Skip to content
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

Add some error checking to pcap_dump(). #494

Closed
wants to merge 1 commit into from

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? 😉

@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.

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