From owner-svn-src-all@FreeBSD.ORG Fri Jul 27 10:57:16 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 21059106566C; Fri, 27 Jul 2012 10:57:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id AD2DD8FC08; Fri, 27 Jul 2012 10:57:15 +0000 (UTC) Received: from c122-106-171-246.carlnfd1.nsw.optusnet.com.au (c122-106-171-246.carlnfd1.nsw.optusnet.com.au [122.106.171.246]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q6RAvC6Z031169 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 27 Jul 2012 20:57:13 +1000 Date: Fri, 27 Jul 2012 20:57:12 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Smirnoff In-Reply-To: <201207270916.q6R9Gm23086648@svn.freebsd.org> Message-ID: <20120727201736.O6703@besplex.bde.org> References: <201207270916.q6R9Gm23086648@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r238828 - head/sys/sys X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Jul 2012 10:57:16 -0000 On Fri, 27 Jul 2012, Gleb Smirnoff wrote: > Log: > Add assertion for refcount overflow. > > Submitted by: Andrey Zonov > 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 New namespace pollution. > #include 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 This is misplacted. In the kernel, it is a standard part of sys/systm.h, which is included later. % % #ifdef _KERNEL % #include 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 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