Date: Tue, 30 Mar 2010 11:16:35 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "M. Warner Losh" <imp@bsdimp.com> Cc: net@freebsd.org Subject: Re: Small patch to ipfilter for arm Message-ID: <20100330104425.E5195@delplex.bde.org> In-Reply-To: <20100329.115116.385399974524554540.imp@bsdimp.com> References: <20100329.115116.385399974524554540.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 29 Mar 2010, M. Warner Losh wrote: > OK. I'd like to propose the following patch for ipfilter: > > Index: sys/contrib/ipfilter/netinet/ip_compat.h > =================================================================== > --- sys/contrib/ipfilter/netinet/ip_compat.h (revision 205838) > +++ sys/contrib/ipfilter/netinet/ip_compat.h (working copy) > @@ -975,7 +975,6 @@ > # define SPL_NET(x) ; > # define SPL_IMP(x) ; > # define SPL_SCHED(x) ; > -extern int in_cksum __P((struct mbuf *, int)); > # else > # define SPL_SCHED(x) x = splhigh() > # endif /* __FreeBSD_version >= 500043 */ > > This declaration is wrong, and it prevents arm from building ipfilter. Quite likely. It even uses __P(()), whose use is supposed to have gone away ~10 years ago. But this file's purpose is to provide compat cruft like that. Also, the null SPL's in the above have bogus semicolons. > Why is it wrong? Because we have: > > # if (__FreeBSD_version >= 500002) > # include <netinet/in_systm.h> > # include <netinet/ip.h> > # include <machine/in_cksum.h> > # endif > > # if (__FreeBSD_version >= 500043) > ... > <the above code> > ... > # endif > > So, we have in_cksum.h being included *AND* we're defining this > function. However, in_cksum.h is supposed to do this. > > Why don't we see problems today? No architecture except arm has an > assembler in_cksum in the tree. All the other architectures have > > #define in_cksum(a, b) in_cksum_skip(a, b, 0) > > in their headers. Since the above extern uses __P to hide the args, > in_cksum doesn't expand the macro, so we don't see any problems or > conflicts. Not quite. __P(()) does expand args, except for K&R compilers whose use is supposed to have gone away ~20 years ago. However, the macro has precedence over the declaration, so the declaration has no effect (if there is a macro). The ordering of the includes has to be delicate to get the function declared before the macro, else the declaration would be a syntax error. On arm, where we define in_cksum() correctly to return > u_short, there's a conflict. > > So, it would best if we just dropped this one line from ip_compat.h, > since it was always wrong anyway. I agree. This line is only for non-old FreeBSD systems. It can never have had any good effect on these systems, since even if it were correct then it would have forced failure due to -Wredundant-decls (since the delicate include ordering requires including the system header at the correct point and this include is forced in ip_compat.h itself); thus the system definition is always visible and any private declaration gives a redundant-decl). FreeBSD uses -Wredundant-decls to inhibit use of private decls like this. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100330104425.E5195>