-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Update to enable offline sniff() to use a PacketList #3026
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
9a64e23
to
9eb66c0
Compare
Codecov Report
@@ Coverage Diff @@
## master #3026 +/- ##
==========================================
- Coverage 85.37% 85.35% -0.02%
==========================================
Files 255 256 +1
Lines 53968 54135 +167
==========================================
+ Hits 46075 46209 +134
- Misses 7893 7926 +33
|
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 you add a unit test in this section https://github.com/secdev/scapy/blob/master/test/regression.uts#L2018 ?
elif isinstance(offline, list) and \ | ||
elif (isinstance(offline, list) or | ||
isinstance(offline, PacketList)) and \ | ||
all(isinstance(elt, Packet) for elt in offline): |
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.
We need to find something more efficient than this. We can probably just check the first element and assume the type list is consistent..
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.
Howabout: isinstance(offline, (list, PacketList))
?
%timeit isinstance(pktlist, (list, PacketList))
191 ns ± 1.59 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit isinstance(pktlist, list)
150 ns ± 3.02 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit isinstance(pktlist, list) or isinstance(pktlist, PacketList)
220 ns ± 5.1 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
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 think @gpotter2 was actually talking about the next check (all(...)
)
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 agree with you @gpotter2 but that's not related to this PR. The proposed patch looks good to me.
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.
Ah ok thanks. Agree all() check can be a bit of a delay - Looking at where the call graph goes it ends in calling PcapWriter() whilst it checks for None type one could just add a Packet
type check there...
This minor patch allows for providing a
PacketList
tosniff(offline=my_packet_list)
. The existing behaviour is to fail ("AttributeError: 'list' object has no attribute 'read'") when provided with a PacketList.The test suit runs against it ok.