-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix sr() on multiple interfaces and libpcap L3 sockets #4474
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4474 +/- ##
==========================================
- Coverage 81.68% 81.67% -0.01%
==========================================
Files 355 355
Lines 84706 84754 +48
==========================================
+ Hits 69190 69225 +35
- Misses 15516 15529 +13
|
6cfa194 to
1faafa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool PR! I did not test it but it looks good.
| raise | ||
|
|
||
| @staticmethod | ||
| def select(sockets, remain=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use a generic select()method instead of duplicating it in all sockets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that's clearer, considering the types change in each function.
We would also require a dependency to another L3Socket or something, and I think sockets are already abstracted enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A classmethod could do the trick. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like a LOT of refactoring work that I honnestly don't really want to do :(
1faafa8 to
0f68903
Compare
|
Your call!
|
|
You are right though. How about we let it pass today, but I promise I'll come back to it eventually, or whenever we touch it again :) |
Currently you can
send()on multiple interfaces, but notsr().This PR:
sron multiple interfaces by allowingL3socket*to recursively spawn sockets on other interfaces if necessary