Fixed build fail when NO_PROTOCHAIN specified#852
Conversation
|
Apart from build problem dir seems not used in any meaningful way anymore here. |
| @@ -6363,20 +6363,17 @@ gen_check_802_11_data_frame(compiler_state_t *cstate) | |||
| * against Q_IP and Q_IPV6. | |||
| */ | |||
| static struct block * | |||
| gen_proto(compiler_state_t *cstate, bpf_u_int32 v, int proto, int dir) | |||
There was a problem hiding this comment.
So I think that we included the direction in order to provide future proofing against BPF code that would be able to distinguish between incoming and outgoing packets.
CHASE_CHAIN is not correlated with the build process, it has been undefined since it was introduced in commit 7fe3c11 in 1999, possibly as a leftover from earlier prototypes. If defined, the code still compiles fine, but quietly changes the behaviour in meaningful ways, which is a wrong thing to do. Remove the macro and the associated dead code. A practical side effect of this change is that gen_protochain() now can be conditional solely on NO_PROTOCHAIN, which simplifies getting the latter right to solve the build problem stated in GH #852.
The NO_PROTOCHAIN macro is properly managed by both of the current build systems, but it looks like noone has exercised the option in a long time, as the source code compiles only with the default "enable protochain" setting and the other branch fails due to a compiler error: ./configure --disable-protochain && make [...] ./gencode.c: In function 'gen_protochain': ./gencode.c:6077:9: error: too few arguments to function 'gen_proto' 6077 | return gen_proto(cstate, v, proto); Drop the offending call to gen_proto() because it is out of place anyway and make both the declarations and the invocations of gen_protochain() conditional on NO_PROTOCHAIN to emphasize that when the macro is defined, the code is not reachable because the parser rejects the "protochain" token at an earlier step. ./configure --disable-protochain && make [...] $ tcpdump -y EN10MB -d 'ip protochain \tcp' tcpdump: protochain not supported
|
Another way to reproduce the problem is using This way, at a glance the following change would be a much simpler and more consistent way to fix the error: --- a/gencode.c
+++ b/gencode.c
@@ -6083,7 +6083,7 @@ static struct block *
gen_protochain(compiler_state_t *cstate, bpf_u_int32 v, int proto)
{
#ifdef NO_PROTOCHAIN
- return gen_proto(cstate, v, proto);
+ return gen_proto(cstate, v, proto, Q_DEFAULT);
#else
struct block *b0, *b;
struct slist *s[100];However, it means "instead of chasing the protocol chain just test that the outermost protocol number of the packet matches the protocol number given in the expression", which does not look right: what the filter expression looks should be what it actually does, so long as it manages to compile. However, upon a closer look it is possible to notice that with #ifdef NO_PROTOCHAIN
bpf_set_error(cstate, "protochain not supported");
YYABORT;
#else
QSET($$.q, $1, Q_DEFAULT, Q_PROTOCHAIN);
#endifThis actually works, which is good because the library rejects the filter immediately with an error and the user can tell the reason: However, this was not quite exactly the case because two calls to After some time studying the matter I found it better to remove |
|
It looks as if the intent of If we could make protocol chasing work everywhere, including in the kernel, and support optimization of filters with backwards branches - i.e., the only cost of protocol chasing would be a little CPU time in the filter, not "this has to be done in userland" or "filter code isn't optimized"" - that might be a nice simplification (although the place where protocol chasing seems to be most important is with VLAN traffic, but we don't support any way to chase through VLAN headers). But, as we don't, I don't see much point in keeping the |
CHASE_CHAIN is not correlated with the build process, it has been undefined since it was introduced in commit 7fe3c11 in 1999, possibly as a leftover from earlier prototypes. If defined, the code still compiles fine, but quietly changes the behaviour in meaningful ways, which is a wrong thing to do. Remove the macro and the associated dead code. A practical side effect of this change is that gen_protochain() now can be conditional solely on NO_PROTOCHAIN, which simplifies getting the latter right to solve the build problem stated in GH #852. (cherry picked from commit e6129d5)
The NO_PROTOCHAIN macro is properly managed by both of the current build systems, but it looks like noone has exercised the option in a long time, as the source code compiles only with the default "enable protochain" setting and the other branch fails due to a compiler error: ./configure --disable-protochain && make [...] ./gencode.c: In function 'gen_protochain': ./gencode.c:6077:9: error: too few arguments to function 'gen_proto' 6077 | return gen_proto(cstate, v, proto); Drop the offending call to gen_proto() because it is out of place anyway and make both the declarations and the invocations of gen_protochain() conditional on NO_PROTOCHAIN to emphasize that when the macro is defined, the code is not reachable because the parser rejects the "protochain" token at an earlier step. ./configure --disable-protochain && make [...] $ tcpdump -y EN10MB -d 'ip protochain \tcp' tcpdump: protochain not supported (cherry picked from commit bc594f1)
CHASE_CHAIN is not correlated with the build process, it has been undefined since it was introduced in commit 7fe3c11 in 1999, possibly as a leftover from earlier prototypes. If defined, the code still compiles fine, but quietly changes the behaviour in meaningful ways, which is a wrong thing to do. Remove the macro and the associated dead code. A practical side effect of this change is that gen_protochain() now can be conditional solely on NO_PROTOCHAIN, which simplifies getting the latter right to solve the build problem stated in GH the-tcpdump-group#852.
The NO_PROTOCHAIN macro is properly managed by both of the current build systems, but it looks like noone has exercised the option in a long time, as the source code compiles only with the default "enable protochain" setting and the other branch fails due to a compiler error: ./configure --disable-protochain && make [...] ./gencode.c: In function 'gen_protochain': ./gencode.c:6077:9: error: too few arguments to function 'gen_proto' 6077 | return gen_proto(cstate, v, proto); Drop the offending call to gen_proto() because it is out of place anyway and make both the declarations and the invocations of gen_protochain() conditional on NO_PROTOCHAIN to emphasize that when the macro is defined, the code is not reachable because the parser rejects the "protochain" token at an earlier step. ./configure --disable-protochain && make [...] $ tcpdump -y EN10MB -d 'ip protochain \tcp' tcpdump: protochain not supported
Build on latest master:
Line gencode.c:6043
Error
Scanning dependencies of target SerializeTarget [ 2%] Generating grammar.c, grammar.h [ 5%] Generating scanner.c, scanner.h /home/pbrzezinski/open-source-contrib/libpcap/grammar.y:40.1-7: warning: POSIX Yacc does not support %expect [-Wyacc] %expect 38 ^~~~~~~ [ 5%] Built target SerializeTarget Scanning dependencies of target pcap_static [ 8%] Building C object CMakeFiles/pcap_static.dir/bpf_filter.c.o [ 11%] Building C object CMakeFiles/pcap_static.dir/bpf_dump.c.o [ 14%] Building C object CMakeFiles/pcap_static.dir/gencode.c.o [ 17%] Building C object CMakeFiles/pcap_static.dir/bpf_image.c.o [ 20%] Building C object CMakeFiles/pcap_static.dir/etherent.c.o [ 22%] Building C object CMakeFiles/pcap_static.dir/fmtutils.c.o [ 25%] Building C object CMakeFiles/pcap_static.dir/optimize.c.o /home/pbrzezinski/open-source-contrib/libpcap/gencode.c: In function ‘gen_protochain’: /home/pbrzezinski/open-source-contrib/libpcap/gencode.c:6043:9: error: too few arguments to function ‘gen_proto’ return gen_proto(cstate, v, proto); ^~~~~~~~~ /home/pbrzezinski/open-source-contrib/libpcap/gencode.c:571:22: note: declared here static struct block *gen_proto(compiler_state_t *, bpf_u_int32, int, int); ^~~~~~~~~ [ 28%] Building C object CMakeFiles/pcap_static.dir/nametoaddr.c.o make[2]: *** [CMakeFiles/pcap_static.dir/build.make:142: CMakeFiles/pcap_static.dir/gencode.c.o] Error 1 make[2]: *** Waiting for unfinished jobs.... [ 31%] Building C object CMakeFiles/pcap_static.dir/pcap.c.o [ 34%] Building C object CMakeFiles/pcap_static.dir/pcap-common.c.o make[1]: *** [CMakeFiles/Makefile2:73: CMakeFiles/pcap_static.dir/all] Error 2 make: *** [Makefile:130: all] Error 2How I build :