Skip to content

Conversation

shane-kerr
Copy link
Contributor

We noticed that pcap files were corrupted after a full disk event. This patch tries to help a bit.

Note that probably it would be better if pcap_dump() returned an error code, rather than being a void function. The best way to transition to that might be to add a new function that returned an error, and had pcap_dump() just call that and throw away the error code, then declare pcap_dump() deprecated in a future version of the ABI and ultimately removing it.

Who thought a function that doesn't return an error code was a good idea? 😉

@mcr mcr self-assigned this Apr 30, 2019
@mcr mcr added this to the release-after-next milestone Apr 30, 2019
@infrastation
Copy link
Member

This rebases on the current master branch fine after a trivial conflict resolution, but the pull request predates the "push into the contributor's branch" feature, so I cannot update it in place. In any case, the suggested change looks useful.

@guyharris
Copy link
Member

Who thought a function that doesn't return an error code was a good idea?

Who thought that the pcap callback functions for pcap_dispatch() and pcap_loop() shouldn't be able to return an error code, so that those routines could exit the loop and return the error code?

The right fix is to add new routines where the callback can return a status code, and then a dump routine that does return an error code could be added and used with those routines.

That could be done as part of the process of adding full pcapng support.

(Note that changes that modify the API or ABI are not allowed.)

@infrastation
Copy link
Member

This isn't a perfect fix, but it adds a comment with the rationale and an extra ferror() check that is not in pull request #1048, so in order to credit the much earlier effort I am going to rebase and to merge this change in a couple days (unless anyone objects).

@infrastation infrastation assigned infrastation and unassigned mcr Jan 3, 2022
@infrastation
Copy link
Member

Merged as commit e6d1929, thank you.

@infrastation infrastation removed this from the release-after-next milestone May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants