pcap icon indicating copy to clipboard operation
pcap copied to clipboard

Make `dump*` functions usable, add `breakLoop`, improve docs, and expand examples

Open ntc2 opened this issue 7 years ago • 6 comments

I can break this up into separate pull requests if prefer. I've used many small commits to make it easy to see what I changed, and the changes are also well summarized in the CHANGELOG.md I added.

My original motivation was to make the dump* functions usable via the provided APIs, i.e. Issue #3.

The documentation and type changes for the loop* functions fix Issue #4.

@bos: I see that there haven't been any commits in this repo since 2012, and there are several outstanding pull requests. Are you still maintaining this project?

ntc2 avatar Jun 30 '17 01:06 ntc2

@bos Bump! In case you are not still maintaining this project, I would be happy to take over for you. It seems pretty low volume, and we're using this library in a project at Galois for the next year or so.

ntc2 avatar Sep 01 '17 03:09 ntc2

@bos: Please merge this or give somebody else the commit bit. I just ran into several issues that this pull request fixed

Gabriella439 avatar Apr 09 '18 22:04 Gabriella439

@ntc2 given that I'm building against your PR, perhaps it makes more sense to ask you rather than @bos , what do you think of providing an unsafe variant of the BS functions? It's a simple enough modification, but converting eg. the Ptr Word8 returned by next into an (uncopied) ByteString seems like a common use-case.

m00ngoose avatar Apr 03 '19 04:04 m00ngoose

what do you think of providing an unsafe variant of the BS functions? It's a simple enough modification, but converting eg. the Ptr Word8 returned by next into an (uncopied) ByteString seems like a common use-case.

@awhtayler Sorry, I don't understand. Can you provide more detail or some example code?

ntc2 avatar Apr 03 '19 20:04 ntc2

loopBS and dispatchBS call wrapBS, nextBS calls toBS.

wrapBS and toBS both have a line that reads essentially bs <- B.create (fromIntegral len) $ \p -> B.memcpy p ptr (fromIntegral len) - the packet data is copied from its location ptr into a newly created ByteString. This is done because the reference is potentially invalidated between calls (see here - "The struct pcap_pkthdr and the packet data are not to be freed by the caller, and are not guaranteed to be valid after the next call to pcap_next_ex(), pcap_next(), pcap_loop(3PCAP), or pcap_dispatch(3PCAP); if the code needs them to remain valid, it must make a copy of them.")

However, in the vein of eg. Conduit's sourceHandleUnsafe, if you're going to process the ByteString in such a way that you need not maintain references to it between calls to nextBS / running the callback provided to loopBS/dispatchBS, then there's no need to make a copy, and the line may as well instead read bs <- BU.unsafePackCStringLen (castPtr ptr, fromIntegral len) - treat the memory location pointed to by ptr as a ByteString directly.

m00ngoose avatar Apr 03 '19 22:04 m00ngoose

@awhtayler thanks for explaining. I don't think it makes sense to add this unsafe functions as part of this PR. But you could create another PR that builds on top of this one, that adds those unsafe functions.

ntc2 avatar Apr 17 '19 21:04 ntc2