Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow .igmpize() on IGMPv3 packet types #2537

Merged
merged 1 commit into from Mar 25, 2020
Merged

Conversation

Radvendii
Copy link
Contributor

This is marked as a draft because I'm having trouble adding tests. When I run test/run_tests it doens't seem to run any of the contrib tests. Is that by design? Can I turn those on?

Also, the guidelines say no unnecessary lists. I think value in [item1, item2, item3, item4] is a whole lot more readable / maintainable / etc than value == item1 or value == item2 or value == item3 or value == item4 but I can change it if that's really better.

This PR allows you to run .igmpize() on IGMPv3 packets that are a type that didn't exist in IGMP. It shouldn't affect too much. I think there may still be some issues in IGMPv3 that I'll have to fix in a separate pull request.

fixes #2536

@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #2537 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2537      +/-   ##
==========================================
- Coverage   87.72%   87.72%   -0.01%     
==========================================
  Files         242      242              
  Lines       51089    51111      +22     
==========================================
+ Hits        44818    44836      +18     
- Misses       6271     6275       +4     
Impacted Files Coverage Δ
scapy/contrib/igmp.py 100.00% <100.00%> (ø)
scapy/layers/http.py 84.54% <0.00%> (-1.08%) ⬇️
scapy/arch/linux.py 75.60% <0.00%> (-0.88%) ⬇️
scapy/arch/bpf/supersocket.py 76.41% <0.00%> (-0.58%) ⬇️
scapy/layers/tftp.py 89.39% <0.00%> (-0.29%) ⬇️
scapy/layers/inet6.py 83.73% <0.00%> (+0.05%) ⬆️
scapy/contrib/isotp.py 90.36% <0.00%> (+0.08%) ⬆️
scapy/sendrecv.py 85.57% <0.00%> (+0.16%) ⬆️
scapy/contrib/igmpv3.py 91.37% <0.00%> (+1.72%) ⬆️
scapy/autorun.py 72.85% <0.00%> (+3.57%) ⬆️

@gpotter2
Copy link
Member

Hi & thanks for the PR.
You can add your tests to https://github.com/secdev/scapy/blob/master/test/contrib/igmpv3.uts
To test them, you could simply use tox, or you can use ./run_tests -t test/contrib/.../igmpv3.uts. You can ./run_tests -h to see all available options.

@Radvendii
Copy link
Contributor Author

Okay, thanks. I made sure my test succeeds (but fails if my changes aren't applied).

Earlier, the Travis CI build was failling, but I couldn't figure out why. Or rather, it seemed unrelated to my change. It says ###(012)=[failed] Test DTC scan
Any help on that front?

@guedou
Copy link
Member

guedou commented Mar 24, 2020

It looks good to me. Could you remove the draft when you are ready?

@Radvendii Radvendii marked this pull request as ready for review March 24, 2020 15:58
@Radvendii
Copy link
Contributor Author

Sure. Some tests still seem to be failing, but it's a different test than it was before. I'm guessing this is one of those things where you eventually learn when you can safely ignore the big red X 😛

@guedou guedou merged commit f2fde56 into secdev:master Mar 25, 2020
@guedou
Copy link
Member

guedou commented Mar 25, 2020

The only red mark is from the code coverage tool. Its result could slightly change depending on the network packets received during the tests. Here the difference is really small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IGMPv3 igmpize broken
3 participants