Date: Tue, 30 Sep 2003 18:22:35 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: net@freebsd.org Subject: Re: finishing the if.h/if_var.h split Message-ID: <20030930172536.U3713@gamplex.bde.org> In-Reply-To: <20030930010128.GA31222@Odin.AC.HMC.Edu> References: <20030930010128.GA31222@Odin.AC.HMC.Edu>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <net/if_var.h>
> #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 <sys/if_var.h> (and especially <sys/if.h> to
include <sys/bus.h> 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 <sys/bus.h>
unconditionally in <net/if_var.h>. However, the pollution may be harmful
for userland. In fact, including <sys/bus.h> 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
<sys/bus.h> is included. Oops, <net/if.h> doesn't include <net/if_var.h>
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 <net/if_var.h> in userland. E.g., struct ifnet is
(ab)used in netstat at least, so struct ifnet is outside of the _KERNEL
ifdef in <net/if_var.h> and adding a device_t to struct ifnet would
expose even more kernel internals to userland. Since <sys/bus.h>
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
<net/if_var.h> doesn't do it itself by some means other than including
<sys/bus.h>
> 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 <sys/rman.h>.
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 <sys/bus.h> and thus can see its internals. style(9)
says to not use negatively opaque typedefs).
<net/if_var.h> 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 <sys/_label.h> /* struct label */
-#include <sys/queue.h> /* get TAILQ macros */
+#include <sys/event.h> /* XXX XXX */
+#include <sys/queue.h>
#ifdef _KERNEL
-#include <sys/mbuf.h>
-#endif /* _KERNEL */
-#include <sys/lock.h> /* XXX */
-#include <sys/mutex.h> /* XXX */
-#include <sys/event.h> /* XXX */
+#include <sys/_label.h> /* XXX */
+#include <sys/lock.h> /* XXX XXX */
+#include <sys/mbuf.h> /* XXX XXX */
+#include <sys/mutex.h> /* XXX XXX */
+#else
+#include <sys/_label.h> /* XXX */
+#include <sys/_lock.h> /* XXX */
+#include <sys/_mutex.h> /* XXX */
+#endif
TAILQ_HEAD(ifnethead, ifnet); /* we use TAILQs so that the order of */
@@ -116,5 +115,5 @@
* struct ifnet ac_if;
* ...
- * } <arpcom> ;
+ * } <arpcom>;
* ...
* };
@@ -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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030930172536.U3713>