Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 27 Jul 2012 20:57:12 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Gleb Smirnoff <glebius@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org
Subject:   Re: svn commit: r238828 - head/sys/sys
Message-ID:  <20120727201736.O6703@besplex.bde.org>
In-Reply-To: <201207270916.q6R9Gm23086648@svn.freebsd.org>
References:  <201207270916.q6R9Gm23086648@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 27 Jul 2012, Gleb Smirnoff wrote:

> Log:
>  Add assertion for refcount overflow.
>
>  Submitted by:	Andrey Zonov <andrey zonov.org>
>  Reviewed by:	kib
>
> Modified:
>  head/sys/sys/refcount.h
>
> Modified: head/sys/sys/refcount.h
> ==============================================================================
> --- head/sys/sys/refcount.h	Fri Jul 27 08:28:44 2012	(r238827)
> +++ head/sys/sys/refcount.h	Fri Jul 27 09:16:48 2012	(r238828)
> @@ -32,6 +32,7 @@
> #ifndef __SYS_REFCOUNT_H__
> #define __SYS_REFCOUNT_H__
>
> +#include <sys/limits.h>

New namespace pollution.

> #include <machine/atomic.h>

Old namespace pollution.  Worse than the above, since machine/atomic.h
is standard pollution in sys/systm.h.

>
> #ifdef _KERNEL
> @@ -51,6 +52,7 @@ static __inline void
> refcount_acquire(volatile u_int *count)

Despite the old namespace pollution, this file still has prerequisites,
to get u_int declared, and even more prerequisites to get all the
prerequisites for the pollution declated.  These prerequisites may
consist of only sys/types.h, or sys/param.h and all of its standard
pollution.  However, this file is apparently supposed to be useable in
userland, and some of the pollution is sort of needed there.  The
includes are nonsense for the kernel case.

> {
>
> +	KASSERT(*count < UINT_MAX, ("refcount %p overflowed", count));

Spell UINT_MAX as (uint_t)-1 to avoid the new pollution.

> 	atomic_add_acq_int(count, 1);
> }

The patch doesn't show quite enough detail to untangle the pollution.
Here is more:

% #include <machine/atomic.h>

This is misplacted.  In the kernel, it is a standard part of sys/systm.h,
which is included later.

% 
% #ifdef _KERNEL
% #include <sys/systm.h>

Yet more pollution, yet not enough to actually compile standalone.  This
is needed mainly for KASSERT().  All though it has massive internal
pollution, with the machine/cpufunc.h and machine/atomic.h parts very
standard and useful, its still needs its includer to include sys/types.h
before it.  Kernel source files must almost always include both
include sys/param.h and sys/param.h first, since not all kernel headers
are as polluted as this one and include sys/systm.h when they use KASSERT,
and since future ones should not be polluted with thus include but might
grow a use of KASSERT.  There are just a few kernel source files that try
to be too smart about this and include only sys/types.h and not sys/param.h,
or sort the includes with sys/systm.h after other headers that use it.
These tend to break when KASSERTs and other magic are added.

The following headers in <sys> now use KASSERT: buf.h, eventhandler.h,
lock.h, mbuf.h, mount.h, mutex.h, osd.h, proc.h, refcount.h.  Only the
following include sys/systm.h: libkern.h, mbuf.h, pmckern.h, refcount.h.
libkern.h is where the standard pollution of machine/atomic.h comes
from.  This is especially bogus in libkern.h -- libkern.h includes
systm.h, and systm.h includes libkern.h.  The latter is just because
a few C source files, mainly in sys/libkern/, try to be too smart about
minimal includes.  Some include just libkern.h instead of the usual
param.h and systm.h.  libkern.h has complete pollution needed to
compile stand-alone, including types.h, unlike the above.

% #else
% #define	KASSERT(exp, msg)	/* */

Stub for userland.  Your new code is just useless in userland.

% #endif
% 
% static __inline void
% refcount_init(volatile u_int *count, u_int value)

The kernel case shouldn't include anything here.  Require systm.h
as a prerequisite instead of types.h.

The user case doesn't need limits.h, but unfortunately needs atomic.h
to avoid expanding its current bogus prerequisites.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120727201736.O6703>