Small patch to ipfilter for arm

[Bruce Evans]

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