From owner-freebsd-net@FreeBSD.ORG  Tue Mar 30 02:22:10 2010
Return-Path: <owner-freebsd-net@FreeBSD.ORG>
Delivered-To: net@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34])
	by hub.freebsd.org (Postfix) with ESMTP id 44493106566B
	for <net@freebsd.org>; Tue, 30 Mar 2010 02:22:10 +0000 (UTC)
	(envelope-from brde@optusnet.com.au)
Received: from fallbackmx06.syd.optusnet.com.au
	(fallbackmx06.syd.optusnet.com.au [211.29.132.8])
	by mx1.freebsd.org (Postfix) with ESMTP id CF0578FC14
	for <net@freebsd.org>; Tue, 30 Mar 2010 02:22:09 +0000 (UTC)
Received: from mail02.syd.optusnet.com.au (mail02.syd.optusnet.com.au
	[211.29.132.183])
	by fallbackmx06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id
	o2U0Gixt013549 for <net@freebsd.org>; Tue, 30 Mar 2010 11:16:44 +1100
Received: from c122-106-158-90.carlnfd1.nsw.optusnet.com.au
	(c122-106-158-90.carlnfd1.nsw.optusnet.com.au [122.106.158.90])
	by mail02.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id
	o2U0GZ9x030487
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO);
	Tue, 30 Mar 2010 11:16:37 +1100
Date: Tue, 30 Mar 2010 11:16:35 +1100 (EST)
From: Bruce Evans <brde@optusnet.com.au>
X-X-Sender: bde@delplex.bde.org
To: "M. Warner Losh" <imp@bsdimp.com>
In-Reply-To: <20100329.115116.385399974524554540.imp@bsdimp.com>
Message-ID: <20100330104425.E5195@delplex.bde.org>
References: <20100329.115116.385399974524554540.imp@bsdimp.com>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
Cc: net@freebsd.org
Subject: Re: Small patch to ipfilter for arm
X-BeenThere: freebsd-net@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: Networking and TCP/IP with FreeBSD <freebsd-net.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-net>,
	<mailto:freebsd-net-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-net>
List-Post: <mailto:freebsd-net@freebsd.org>
List-Help: <mailto:freebsd-net-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-net>,
	<mailto:freebsd-net-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 30 Mar 2010 02:22:10 -0000

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