From owner-freebsd-net@FreeBSD.ORG Tue Sep 30 01:24:12 2003 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 53F4F16A4B3; Tue, 30 Sep 2003 01:24:12 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id AA83743F93; Tue, 30 Sep 2003 01:24:06 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3p2/8.8.7) with ESMTP id SAA04609; Tue, 30 Sep 2003 18:23:57 +1000 Date: Tue, 30 Sep 2003 18:22:35 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Brooks Davis In-Reply-To: <20030930010128.GA31222@Odin.AC.HMC.Edu> Message-ID: <20030930172536.U3713@gamplex.bde.org> References: <20030930010128.GA31222@Odin.AC.HMC.Edu> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: arch@freebsd.org cc: net@freebsd.org Subject: Re: finishing the if.h/if_var.h split X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Sep 2003 08:24:12 -0000 On Mon, 29 Sep 2003, Brooks Davis wrote: > Six years and eight months ago, net/if.h was split into if.h and > if_var.h. At the time, if_var.h was included at the end if if.h as > follows (this is the current code, but it's equivalent): > > #ifdef _KERNEL > struct thread; > > /* XXX - this should go away soon. */ > #include > #endif > > Unfortunately, "soon" hasn't happened yet and it is now tripping me > up. To add the if_dev member to struct ifnet (see the forthcoming > post on that subject), it is necessary for sys/bus.h to be included in > net/if_var.h That would be namespace pollution, so it is not permitted :-). Requiring all files that include (and especially to include would be interface breakage so it is even less permitted. > which in turn requires that if_var.h NOT be included in > genassym.c. Do you mean in userland? There don't seem to be any immediate problems for genassym.c or any other file in the kernel from including unconditionally in . However, the pollution may be harmful for userland. In fact, including would just not work for userland, since the declaration of device_t is only made in the _KERNEL case, so use of it in struct ifnet (which is exported to userland for some reason) would be a syntax error in userland whether or not is included. Oops, doesn't include in the !_KERNEL case, so the problem is a little different... > Since if.h must be included for nfsdiskless support, this > means we need to finish the job and remove the include if_var.h from > if.h. It involves editing a large number of files, but over all it's > pretty mechanical as it simple includes adding and include of if_var.h > after the if.h include in files that break after the change. Mechanical removal wouldn't help userland. It has already been done for userland, but too mechanically to actually address the problem of abusing kernel interfaces in in userland. E.g., struct ifnet is (ab)used in netstat at least, so struct ifnet is outside of the _KERNEL ifdef in and adding a device_t to struct ifnet would expose even more kernel internals to userland. Since correctly doesn't declare device-t in the !_KERNEL case, clients like netstat would have to become aware of new magic to declare device_t if doesn't do it itself by some means other than including > P.S. The alternative is to add a second typedef of device_t to if_var.h. > It's an ugly solution since it and the definition in sys/bus.h would > have to look like the one below, but it would be a heck of a lot easier. > > #ifndef _DEVICE_T_DECLARED > typedef struct device *device_t; > #define _DEVICE_T_DECLARED > #endif That's one alternative. (Far too) many places already use the simple alternative of just using "struct device *". Grep shows 68 lines containing "struct device" in *.h and 32 in *.c. For "device_t", the numbers are 2140 in *.h and 5089 in *.c. This is in a sys tree with about 1000 matches of "device_t" in generated files. There are non-bogus uses of "struct device" to avoid namespace pollution in . Most other uses are just bogus (modulo the existence of device_t being non-bogus -- its opaqueness is negative since anything that wants to use it must include and thus can see its internals. style(9) says to not use negatively opaque typedefs). exports lots more kernel internals to userland than 6 years ago. It now exports labels, mutexes and locks. I have only fixed part of this: %%% Index: if_var.h =================================================================== RCS file: /home/ncvs/src/sys/net/if_var.h,v retrieving revision 1.58 diff -u -2 -r1.58 if_var.h --- if_var.h 1 Jan 2003 18:48:54 -0000 1.58 +++ if_var.h 7 Aug 2003 16:47:54 -0000 @@ -46,7 +46,8 @@ * received from its medium. * - * Output occurs when the routine if_output is called, with three parameters: + * Output occurs when the routine if_output is called: * (*ifp->if_output)(ifp, m, dst, rt) - * Here m is the mbuf chain to be sent and dst is the destination address. + * Here m is the mbuf chain to be sent, dst is the destination address, + * and rt is XXX. * The output routine encapsulates the supplied datagram if necessary, * and then transmits it on its medium. @@ -63,25 +64,23 @@ */ -#ifdef __STDC__ -/* - * Forward structure declarations for function prototypes [sic]. - */ -struct mbuf; -struct thread; +struct ether_header; struct rtentry; struct rt_addrinfo; struct socket; -struct ether_header; -#endif +struct thread; -#include /* struct label */ -#include /* get TAILQ macros */ +#include /* XXX XXX */ +#include #ifdef _KERNEL -#include -#endif /* _KERNEL */ -#include /* XXX */ -#include /* XXX */ -#include /* XXX */ +#include /* XXX */ +#include /* XXX XXX */ +#include /* XXX XXX */ +#include /* XXX XXX */ +#else +#include /* XXX */ +#include /* XXX */ +#include /* XXX */ +#endif TAILQ_HEAD(ifnethead, ifnet); /* we use TAILQs so that the order of */ @@ -116,5 +115,5 @@ * struct ifnet ac_if; * ... - * } ; + * } ; * ... * }; @@ -125,5 +124,4 @@ * Unfortunately devices' softc are opaque, so we depend on this layout * to locate the struct ifnet from the softc in the generic code. - * */ struct ifnet { %%% This only significantly reduces pollution in the !_KERNEL case. Reducing it in the _KERNEL case is much harder. Bruce