Date: Fri, 2 May 2014 18:34:40 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Luiz Otavio O Souza <loos@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r265191 - head/sys/dev/gpio Message-ID: <20140502180115.I1337@besplex.bde.org> In-Reply-To: <201405011409.s41E9mun075016@svn.freebsd.org> References: <201405011409.s41E9mun075016@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 1 May 2014, Luiz Otavio O Souza wrote:
> Log:
> Remove unnecessary headers. Sort out the headers. Add a missing header on
> ofw_gpiobus.c (it was working because of sys/libkern.h).
Do you use /usr/src/tools/tools/kerninclude/kerninclude.sh to find the
unused includes? There are many false positives and negatives which are
hard to find without a lot of testing. kerninclude.sh automates some of
the testing. I think it is rarely used and has rotted. A full universe
build is probably required, but kerninclude.sh is i386-centric and only
tests LINT, GENERIC and GENERIC98 (GENERIC98 last existed in the source
tree in FreeBSD-3, but the script creates it by copying GENERIC and
building with pc98 options).
I prefer my unusedinc script. It is easier to run 1 on file at a time,
but does less hacking to reduce the false positives and negatives.
> Modified: head/sys/dev/gpio/gpiobus.c
> ==============================================================================
> --- head/sys/dev/gpio/gpiobus.c Thu May 1 14:08:19 2014 (r265190)
> +++ head/sys/dev/gpio/gpiobus.c Thu May 1 14:09:47 2014 (r265191)
> @@ -28,21 +28,16 @@
> __FBSDID("$FreeBSD$");
>
> #include <sys/param.h>
> -#include <sys/systm.h>
This unsorts the includes. <sys/systm.h> must be included after
<sys/param.h>, and should be always included, since other headers may
depend on it (for things like KASSERT() in inline functions). Some
broken headers include it nested. This makes it difficult to tell
whether it is used. Some other headers that don't do this may
compile accidentally because they are included after the polluted
ones.
> +#include <sys/bus.h>
> +#include <sys/gpio.h>
> +#include <sys/kernel.h>
> #include <sys/malloc.h>
> #include <sys/module.h>
> -#include <sys/kernel.h>
> -#include <sys/queue.h>
<sys/queue.h> is probably used. It is included nested in many headers,
and this is not considered pollution, unlike for <sys/systm.h>, but it
makes it very hard to determine if <sys/queue.h> is included as needed
in other headers and .c files.
kerninclude.sh attempts to determine if headers like <sys/queue.h> are
needed by doing things like replacing it by an empty header in some
contexts. I suspect this doesn't handle the full complications.
Ideally, the include of <sys/queue.h> in a .c file should be explicit
iff the .c files uses any queue macro.
> -#include <sys/sysctl.h>
> +#include <sys/systm.h>
> #include <sys/types.h>
<sys/types.h> is certainly not needed. It is standard pollution in
<sys/param.h>, and it is a style bug to not depend on that. It is
also usually a style bug (in kernel code) to include <sys/types.h>
and not include <sys.param.h>. Many things depend on the standard
pollution, or might be changed to depend on it.
> Modified: head/sys/dev/gpio/ofw_gpiobus.c
> ==============================================================================
> --- head/sys/dev/gpio/ofw_gpiobus.c Thu May 1 14:08:19 2014 (r265190)
> +++ head/sys/dev/gpio/ofw_gpiobus.c Thu May 1 14:09:47 2014 (r265191)
> @@ -33,17 +33,13 @@ __FBSDID("$FreeBSD$");
> #include <sys/bus.h>
> #include <sys/gpio.h>
> #include <sys/kernel.h>
> -#include <sys/libkern.h>
Correct. <sys/libkern.h> is standard pollution in <sys/systm.h>, and it
is a style bug to include it directly.
> -#include <sys/lock.h>
> #include <sys/module.h>
> -#include <sys/mutex.h>
> +#include <sys/systm.h>
Not using mutexes may indicate missing locking. I think this works because
almost everything in new-bus is Giant-locked and Giant locking hides its
own details very well.
Elsewhere, there are lots of polluting nested includes of <sys/lock.h>
and <sys/mutex.h> (not even of <sys/_lock.h> and <sys/_mutex.h> which
make it very unclear whether explicit includes of these are needed.
The worst cases are in <geom/geom.h>, <sys/buf.h>, <sys/rmlock.h>,
<sys/tty.h>, <sys/vnode.h> and <net/if_var.h>. Only some of these
are relatively easy to fix by including <sys/_mutex.h> in the headers.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140502180115.I1337>