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

Really need to include pcap-int.h after system includes #6

Closed
wants to merge 1 commit into from
Closed

Conversation

leres
Copy link

@leres leres commented May 15, 2020

or else libpcap's portability.h causes things to go off the rails.

On FreeBSD 11.3-RELEASE the conflict is with string.h:

In file included from ./search.c:38:
/usr/include/string.h:92:9: error: expected ')'
size_t   strlcat(char * __restrict, const char * __restrict, size_t);
         ^
/usr/src/contrib/libpcap/portability.h:86:24: note: expanded from macro 'strlcat'
        strncat((x), (y), (z) - strlen((x)) - 1)
                              ^
/usr/include/string.h:92:9: note: to match this '('
/usr/src/contrib/libpcap/portability.h:86:9: note: expanded from macro 'strlcat'
        strncat((x), (y), (z) - strlen((x)) - 1)
               ^
In file included from ./search.c:38:
/usr/include/string.h:99:7: error: conflicting types for 'strncat'
char    *strncat(char * __restrict, const char * __restrict, size_t);
         ^
/usr/include/string.h:92:9: note: previous declaration is here
size_t   strlcat(char * __restrict, const char * __restrict, size_t);
         ^
/usr/src/contrib/libpcap/portability.h:86:2: note: expanded from macro 'strlcat'
        strncat((x), (y), (z) - strlen((x)) - 1)
        ^
2 errors generated.
*** [search.o] Error code 1

make[1]: stopped in /wrkdirs/usr/ports/net/tcpslice/work/tcpslice-2837b72
1 error

make[1]: stopped in /wrkdirs/usr/ports/net/tcpslice/work/tcpslice-2837b72
===> Compilation failed unexpectedly.
Try to set MAKE_JOBS_UNSAFE=yes and rebuild before reporting the failure to
the maintainer.
*** Error code 1

Stop.
make: stopped in /usr/ports/net/tcpslice

libpcap's portability.h goes off the rails.
@guyharris
Copy link
Member

pcap-int isn't a public header, so it shouldn't include it, nor should it be including portability.h, which is also not a public header.

Instead, given that pcap files are not platform-dependent, it should define the pcap file data structures itself, as per the pcap-savefile man page or the RFC-style pcap file format specification (I plan to ask Van Jacobson and Steve McCanne if they want their names on it).

I'll do that.

guyharris added a commit that referenced this pull request May 15, 2020
It's not something we need to pick up by pulling in an internal libpcap
header; it is *always* the same - if it's not a 32-bit time-in-seconds,
a 32-bit microseconds-part-of-the-time, a 32-bit captured length, and a
32-bit on-the-network length, it's not a valid pcap file.

This should address GitHub issue #6.
@guyharris
Copy link
Member

I'll do that.

Done, along with a bunch of other modernizations largely stolen from modernizations done to tcpdump. It builds on my FreeBSD 11.2 VM.

@guyharris guyharris closed this May 15, 2020
@leres
Copy link
Author

leres commented May 15, 2020

pcap-int isn't a public header, so it shouldn't include it, nor should it be including portability.h, which is also not a public header.

I don't disagree with you, this was the minimal change that allowed the FreeBSD port to build without patching.

Instead, given that pcap files are not platform-dependent, it should define the pcap file data structures itself, as per the pcap-savefile man page or the RFC-style pcap file format specification (I plan to ask Van Jacobson and Steve McCanne if they want their names on it).

I don't know when dependency on pcap-int.h appeared but I just looked and it is not present in the original Network Research Group rcs repo version of search.c. There are only commits by Vern, Steve, and I ending around 2000. And I don't think we had any 64-bit systems at that point.

Rewriting the code to eliminate the dependency is clearly the best solution.

@infrastation
Copy link
Member

For posterity, there is now a FAQ entry about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants