Date: Mon, 6 Oct 2014 15:25:44 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjg@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r272574 - head/sys/sys Message-ID: <20141006145350.S1175@besplex.bde.org> In-Reply-To: <201410052139.s95LdoJw029807@svn.freebsd.org> References: <201410052139.s95LdoJw029807@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 5 Oct 2014, Mateusz Guzik wrote: > Log: > seq_t needs to be visible to userspace > > Pointy hat to: mjg > Reported by: bz > X-MFC: with r272567 The namespace pollution is more disgusting than before. > Modified: head/sys/sys/seq.h > ============================================================================== > --- head/sys/sys/seq.h Sun Oct 5 21:34:56 2014 (r272573) > +++ head/sys/sys/seq.h Sun Oct 5 21:39:50 2014 (r272574) > @@ -29,6 +29,16 @@ > #define _SYS_SEQ_H_ > > #ifdef _KERNEL > +#include <sys/systm.h> > +#endif Namespace pollution. In the kernel, it is the responsibility of .c files to include <sys/systm.h> before any other header except <sys/param.h>, in case bad headers like this one use inline functions with KASSERT() either now or later. > +#include <sys/types.h> Namespace pollution. In the kernel, it is the responsibility of .c files to include <sys/param.h> before any other header, since it has standard namspace pollution that other headers may depend on now or later. This pollution includes <sys/types.h>, even outside the kernel. It is a documented style bug to include both <sys/types.h> and <sys/param.h>. It is a bug to include <sys/types.h> and not <sys/param.h> in the kernel, except in very special cases where only headers with a stable API are included after it (otherwise, the others might grow a need for <sys/param.h> or <sys/systm.h>. Outside of the kernel, the necessary types and no others should be declared in headers. > +/* > + * seq_t may be included in structs visible to userspace > + */ > +typedef uint32_t seq_t; > + > +#ifdef _KERNEL > > /* > * Typical usage: > @@ -54,10 +64,7 @@ > * foo(lobj); > */ > > -typedef uint32_t seq_t; > - > /* A hack to get MPASS macro */ > -#include <sys/systm.h> > #include <sys/lock.h> Now the comment doesn't match the code. The hack is to include both <sys/systm.h> and <sys/lock.h>. MPASS is defined in <sys/lock.h>. <sys/lock.h> has minimal pollution. In particular, it doesn't include <sys/systm.h>. It uses MPASS() with a KASSERT(), but this is in a macro so files that actually use the macro need to include <sys/param.h>. <sys/filedesc.h>'s includers are now such files, but <sys/filedesc.h> polluted instead. This commit moves half of the hack earlier than needed and far from the comment that documents it. > > #include <machine/cpu.h> If you remove the <sys/systm.h> part of the hack, some .c files might break. This is a feature. It detects 2 bugs: - nested pollution. IIRC, <sys/filedesc.h> is now polluted with an include of <sys/seq.h>, so not only files that include <sys/seq.h> are effected - the brokenness of the files that break. <sys/filedesc.h> is included much more, and any .c file that is missing the include of <sys/systm.h>, or includes it miss-sorted in alphabetical or totally disordered after <sys/filedesc.h>. Part of the feature is that you get to fix all such broken .c files, or better implement new headers without namespace pollution. The only easy way to avoid pollution from KASSERT() in headers is to only use it in other macros. Some inline functions are so small that this is clean enough. But ones that use KASSERT() are not so small. Just using it makes them too large to inline (when it is enabled). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141006145350.S1175>