Tidy up the stream module

rust-pcap / pcap

Rust language pcap library

Apache License 2.0

620 stars 144 forks source link

Tidy up the stream module #200

Closed axelf4 closed 2 years ago

axelf4 commented 2 years ago

Remove unused direct dependency on mio.
Inline the Capture::next_noblock method into stream module.
Use newtype wrapper of Capture that implements AsRawFd with pcap_get_selectable_fd(), instead of new data type SelectableFd that carries around the fd;
...in order to use AsyncFdReadyMutGuard::try_io which abstracts away the details previously explicit in Capture::next_noblock.
(Drop the PacketCodec requirement of being Unpin.)
Add PacketStream::inner_mut to make it possible to inject packets in-between awaiting packets.

This is a breaking change since it removes stream::SelectableFd and stream::PacketStream::new from the public API.

Closes #192

Wojtek242 commented 2 years ago

From a quick I like this PR. It does appear much cleaner. I would like to take a closer look at it first though.

However, for that can I ask you to do two things: 1) Since we don't have tests covering this feature, could you please add an example of the issue you were trying to solve in #192. 2) Can you make sure the CI passes - that way in case it takes me a bit too long to get around to reviewing I won't have to ping you for some minor fixes.

Edit: There are a few examples in the examples directory already - doesn't have to be complicated. It will also need to be added to Cargo.toml.

axelf4 commented 2 years ago

Hmm, I do not see how the recent Windows build failure would be caused by this PR.

Wojtek242 commented 2 years ago

@axelf4 thanks for the ping. The windows CI has been lately failing to download winpcap occasionally and needs to be given a kick to run again.

Looks like all tests pass.

Thanks for the PR and the example! I'll have a look at the PR with your example some time soon.

Wojtek242 commented 2 years ago

Thanks for the PR. Code is nice and your example seems to work for me! I'll merge straight since I took my time and I don't want to block again, but if you have time, I'd appreciate if you could add a CHANGELOG entry for this PR.