Return errors from pcap_dump and pcap_dump_close

[rigtorp]

• Add new functions pcap_dump1, pcap_dump_close1 that returns PCAP_ERROR on error
• Update manpages
• Update CMakeLists.txt to install symlinks for new manpages
• Update Makefile.in to install symlinks for new manpages
• Update CHANGES

Closes #1047

[infrastation]

Thank you for proposing these changes. Regardless of the more substantial aspects, which also need to be reviwed, it would be difficult to remember the difference between pcap_dump1() and pcap_dump() (or pcap_dump2() if there is another variation in future). It would be easier with a name like pcap_dump_reliably(), pcap_dump_or_fail() or some such.

[guyharris]

Thank you for proposing these changes. Regardless of the more substantial aspects, which also need to be reviwed, it would be difficult to remember the difference between pcap_dump1() and pcap_dump() (or pcap_dump2() if there is another variation in future). It would be easier with a name like pcap_dump_reliably(), pcap_dump_or_fail() or some such.

+1

pcap_dump_check_error()/pcap_close_check_error() is another alternative for the names.

[rigtorp]

I don't agree, the action/verb is the same. You wouldn't add _with_error on every function that returns an error of it wasn't to fix a API issue. I marked pcap_dump_close as deprecated, the intention is to use only the new version in new code. The 1 is just there as a reminder that a mistake in ABI/API had to be fixed. Linux kernel takes the same approach with syscalls, Microsoft appends "ex" to their functions, libs like zstd and others appends version number to the replacement functions. If you insist i will of course change it to whatever name you want. I've tested this code with my custom capture tool and it works well.

…

On Wed, Aug 25, 2021, 23:32 Denis Ovsienko ***@***.***> wrote: Thank you for proposing these changes. Regardless of the more substantial aspects, which also need to be reviwed, it would be difficult to remember the difference between pcap_dump1() and pcap_dump() (or pcap_dump2() if there is another variation in future). It would be easier with a name like pcap_dump_reliably(), pcap_dump_or_fail() or some such. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1048 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABLO26SEGEY7JMKHYGVQA3T6VOM5ANCNFSM5CZFVGOQ> .

[rigtorp]

What's the conclusion on the naming of these new functions? I would like to move forward and discuss the implementation changes and hopefully get this merged.

…

On Thu, Aug 26, 2021 at 12:23 AM Erik Rigtorp ***@***.***> wrote: I don't agree, the action/verb is the same. You wouldn't add _with_error on every function that returns an error of it wasn't to fix a API issue. I marked pcap_dump_close as deprecated, the intention is to use only the new version in new code. The 1 is just there as a reminder that a mistake in ABI/API had to be fixed. Linux kernel takes the same approach with syscalls, Microsoft appends "ex" to their functions, libs like zstd and others appends version number to the replacement functions. If you insist i will of course change it to whatever name you want. I've tested this code with my custom capture tool and it works well. On Wed, Aug 25, 2021, 23:32 Denis Ovsienko ***@***.***> wrote: > Thank you for proposing these changes. Regardless of the more substantial > aspects, which also need to be reviwed, it would be difficult to remember > the difference between pcap_dump1() and pcap_dump() (or pcap_dump2() if > there is another variation in future). It would be easier with a name like > pcap_dump_reliably(), pcap_dump_or_fail() or some such. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1048 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AABLO26SEGEY7JMKHYGVQA3T6VOM5ANCNFSM5CZFVGOQ> > . >

[rigtorp]

@guyharris @infrastation any updates regarding the naming?

[rigtorp]

@guyharris @infrastation reminding you about this again.

[infrastation]

Thank you for waiting. Naming consistency within a project usually works better than naming consistency between different projects, so it seems likely that before the new function names get set in stone they should be made to make the most sense to libpcap API users.

A good usage example for the new functions would be in tcpdump source code, this would also make it easier to confirm everything fits as expected. I have made a look at tcpdump.c with this in mind.

The first instance of pcap_dump_close() closes the file before opening the next one. Assuming a single file error should not abort the sequence, the only use for checking the return value of pcap_dump_close_GHPR1048() would be to skip the file compression and to print a warning message. In which case it would be useful to explain the error condition. Which makes me think it might be useful to mention in the man pages that when a new function returns PCAP_ERROR, the calling function can use errno to work the cause out.

The second instance looks similar.

The first instance of pcap_dump() knows it is writing to a file in a sequence, but it does not know how many more packets the current file is going to have, so the above thinking likely does not stand and the only reasonable thing to do on the first failed packet would be to fail right away with an error message. Which, again, would benefit from a meaningful error cause.

The second instance of pcap_dump() knows it is writing a single file, but for consistency and simplicity it seems best to treat all errors as hard errors.

The above changes in tcpdump would need to be conditional depending on HAVE_PCAP_DUMP_CLOSE_GHPR1048 and HAVE_PCAP_DUMP_GHPR1048 or some such, so tcpdump continues to compile with older libpcap versions. Thus the complication is not only in the proposed changes itself, but also in the incurred changes. So it would be best not to rush this before verifying everything.

[rigtorp]

On Thu, Sep 16, 2021 at 4:24 PM Denis Ovsienko ***@***.***> wrote: Thank you for waiting. Naming consistency within a project usually works better than naming consistency between different projects, so it seems likely that before the new function names get set in stone they should be made to make the most sense to libpcap API users.

Any name is fine with me, I only care about the functionality.

The first instance of pcap_dump_close() closes the file before opening the next one. Assuming a single file error should not abort the sequence, the only use for checking the return value of pcap_dump_close_GHPR1048() would be to skip the file compression and to print a warning message. In which case it would be useful to explain the error condition. Which makes me think it might be useful to mention in the man pages that when a new function returns PCAP_ERROR, the calling function can use errno to work the cause out.

errno is going to be EIO, ENOSPC, EDQUOT. Opening the next file will likely fail, but at least logging that the disk is full would be great.

The first instance of pcap_dump() knows it is writing to a file in a sequence, but it does not know how many more packets the current file is going to have, so the above thinking likely does not stand and the only reasonable thing to do on the first failed packet would be to fail right away with an error message. Which, again, would benefit from a meaningful error cause.

This is actually the more important case because if the disk is full, tcpdump will keep on calling pcap_dump() and that data will be silently discarded. If someone then makes more disk space available, writing will start to succeed again. The resulting file is likely to be corrupted because a header + data before the failure was only partially written.

The above changes in tcpdump would need to be conditional depending on HAVE_PCAP_DUMP_CLOSE_GHPR1048 and HAVE_PCAP_DUMP_GHPR1048 or some such, so tcpdump continues to compile with older libpcap versions. Thus the complication is not only in the proposed changes itself, but also in the incurred changes. So it would be best not to rush this before verifying everything.

My capture tool uses the new API to detect write errors and will log and then exit with error code. Systemd will then try to continually restart the capture process. This way I won't get corrupted pcap files saved. Our monitoring will also alert us that a process is failing, but that's kind of useless since ops would have missed the disk is full alerts, oh well.

[mcr]

Any name is fine with me, I only care about the functionality.

Yeah, but names accumulate, get in the way, and confuse people.
I'm trying to assemble some discussion about evolving the API.
Go ahead and work on any code you think makes sense to you, but don't get too attached to names yet.

[infrastation]

pcap_dump1

[infrastation]

Now that pull request #494 has been merged, this change needs to be rebased.

[rigtorp]

@infrastation I fixed the manpage and rebased!