Re: svn commit: r349459 - head/sys/sys
On Thu, 27 Jun 2019, Andriy Gapon wrote: On 27/06/2019 20:37, Bruce Evans wrote: On Thu, 27 Jun 2019, Andriy Gapon wrote: On 27/06/2019 18:47, Bruce Evans wrote: On Thu, 27 Jun 2019, Andriy Gapon wrote: Log: ??upgrade the warning printf-s in bus accessors to KASSERT-s ??After this change sys/bus.h includes sys/systm.h. This is further namespace pollution.?? sys/systm.h is a prerequiste for all kernel headers except sys/param.h and the headers that that includes, since any kernel header (except sys/param.hm etc.) may have inlines which use features in systm.h, e.g., KASSERT() or an inline function. what do you think about amending style(9) to require that if sys/systm.h is to be included, then it must be included before any other header except for sys/param.h (and possibly sys/cdefs.h) ? It is not a style matter. I know... but style(9) documents sys/param.h for similar reasons. sys/systm.h is just a prerequisite for almost all kernel code.?? Perhaps this can be enforced using #ifdef magic even for headers that don't currently use it.?? If it is to be included nested, then sys/param.h is the place to include it, but I don't like that since it is even less related to parameters than sys/param.h, and this would be a regression from minor depollution of sys/param.h. sys/systm.h is also kernel-only, and as you found, including it nested tends to break applications, so other headers that have the bug of including it tend to have _KERNEL ifdefs to avoid including it in applications.?? This is so fundamental that it doesn't have a "No user-serviceable parts inside" ifdef. I think that there is a trivial fix to my commit: moving the sys/systm.h include to the _KERNEL section of sys/bus.h. That's where its definitions are actually used. But I agree that there should be a better approach. I did a quick check of how many .c files are missing includes of sys/systm.h. In an old version of FreeBSD, my normal kernel has about 1100 object files, and about 89 of these broke when a KASSERT() was added to sys/bus.h without adding the pollution. That is many more than I expected. Many of these errors are in automatically generated bus if.c files. E.g., the include list in acpi_if.c is: #include #include #include #include #include #include #include #include "acpi_if.h" Here sys/bus.h and sys/types.h and the contrib include are in the .m file, and the others are automatically generated. Including sys/types.h is nonsense since sys/param.h already included it and sys/bus.h already used typedefs in it. The generation also misorders sys/queue.h and mangles the formatting of the contrib include (in the .m file, it is separated by spaces). Here the .m file should know sys/bus.h's prerequisites and add them all except sys/param.h and sys/systm.h, or maybe the include of sys/bus.h should be auto-generated too (my test kernel has 25 foo_if.c files and 21 of these include sys/bus.c). Most of the generated if.c files include sys/systm.h by nested pollution anyway. Just not early enough to actually work. E.g., in acpi_if.c, the above 8 includes expand to 45 lines, 144 words and 2679 chars in .depend.acpi_if.o according to wc. The word count of 144 is approximately the number of includes. sys/systm.h is first included deeply nested in contrib//acpi.h. 11 of the 25 generated foo_if.c end up never including sys/systm.h. Of the remaining 64 files that fail to compile, all except 7 including 1 maintained by me (cy_pci.c) *blush* end up including sys/systm.h. The other 57 apparently include it either misordered in the .c file or via nested pollution. After removing all nested includes of sys/systm.h in headers, only another 50 object files fail to build. most of the extras are for libkern functions (20 for str*() alone). libkern was polluted.h relatively recently as part of unimproving the implementation of bcd conversion APIs from macros to inlines with KASSERT()s. The KASSERT()s were a bad fix for missing sanity checks of bcd data in callers. Bruce___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r349459 - head/sys/sys
On 27/06/2019 20:37, Bruce Evans wrote: > On Thu, 27 Jun 2019, Andriy Gapon wrote: > >> On 27/06/2019 18:47, Bruce Evans wrote: >>> On Thu, 27 Jun 2019, Andriy Gapon wrote: >>> Log: upgrade the warning printf-s in bus accessors to KASSERT-s After this change sys/bus.h includes sys/systm.h. >>> >>> This is further namespace pollution. sys/systm.h is a prerequiste for >>> all kernel headers except sys/param.h and the headers that that includes, >>> since any kernel header (except sys/param.hm etc.) may have inlines which >>> use features in systm.h, e.g., KASSERT() or an inline function. >> >> what do you think about amending style(9) to require that if sys/systm.h is >> to >> be included, then it must be included before any other header except for >> sys/param.h (and possibly sys/cdefs.h) ? > > It is not a style matter. I know... but style(9) documents sys/param.h for similar reasons. > sys/systm.h is just a prerequisite for almost > all kernel code. Perhaps this can be enforced using #ifdef magic even > for headers that don't currently use it. If it is to be included nested, > then sys/param.h is the place to include it, but I don't like that since > it is even less related to parameters than sys/param.h, and this would be > a regression from minor depollution of sys/param.h. > > sys/systm.h is also kernel-only, and as you found, including it nested > tends to break applications, so other headers that have the bug of > including it tend to have _KERNEL ifdefs to avoid including it in > applications. This is so fundamental that it doesn't have a "No > user-serviceable parts inside" ifdef. I think that there is a trivial fix to my commit: moving the sys/systm.h include to the _KERNEL section of sys/bus.h. That's where its definitions are actually used. But I agree that there should be a better approach. -- Andriy Gapon ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r349459 - head/sys/sys
On Thu, 27 Jun 2019, Andriy Gapon wrote: On 27/06/2019 18:47, Bruce Evans wrote: On Thu, 27 Jun 2019, Andriy Gapon wrote: Log: ??upgrade the warning printf-s in bus accessors to KASSERT-s ??After this change sys/bus.h includes sys/systm.h. This is further namespace pollution.?? sys/systm.h is a prerequiste for all kernel headers except sys/param.h and the headers that that includes, since any kernel header (except sys/param.hm etc.) may have inlines which use features in systm.h, e.g., KASSERT() or an inline function. what do you think about amending style(9) to require that if sys/systm.h is to be included, then it must be included before any other header except for sys/param.h (and possibly sys/cdefs.h) ? It is not a style matter. sys/systm.h is just a prerequisite for almost all kernel code. Perhaps this can be enforced using #ifdef magic even for headers that don't currently use it. If it is to be included nested, then sys/param.h is the place to include it, but I don't like that since it is even less related to parameters than sys/param.h, and this would be a regression from minor depollution of sys/param.h. sys/systm.h is also kernel-only, and as you found, including it nested tends to break applications, so other headers that have the bug of including it tend to have _KERNEL ifdefs to avoid including it in applications. This is so fundamental that it doesn't have a "No user-serviceable parts inside" ifdef. Bruce___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r349459 - head/sys/sys
On 27/06/2019 18:47, Bruce Evans wrote: > On Thu, 27 Jun 2019, Andriy Gapon wrote: > >> Log: >> upgrade the warning printf-s in bus accessors to KASSERT-s >> >> After this change sys/bus.h includes sys/systm.h. > > This is further namespace pollution. sys/systm.h is a prerequiste for > all kernel headers except sys/param.h and the headers that that includes, > since any kernel header (except sys/param.hm etc.) may have inlines which > use features in systm.h, e.g., KASSERT() or an inline function. Bruce, what do you think about amending style(9) to require that if sys/systm.h is to be included, then it must be included before any other header except for sys/param.h (and possibly sys/cdefs.h) ? -- Andriy Gapon ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r349459 - head/sys/sys
On Thu, 27 Jun 2019, Andriy Gapon wrote:
Log:
upgrade the warning printf-s in bus accessors to KASSERT-s
After this change sys/bus.h includes sys/systm.h.
This is further namespace pollution. sys/systm.h is a prerequiste for
all kernel headers except sys/param.h and the headers that that includes,
since any kernel header (except sys/param.hm etc.) may have inlines which
use features in systm.h, e.g., KASSERT() or an inline function.
Modified: head/sys/sys/bus.h
==
--- head/sys/sys/bus.h Thu Jun 27 14:26:57 2019(r349458)
+++ head/sys/sys/bus.h Thu Jun 27 15:07:06 2019(r349459)
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
This header used to be relatively clean, with no polluting includes.
The following low-quality headers in sys already include sys/systm.h:
- capsicum.h:#include
- fail.h:#include
- fbio.h:#include
- intr.h:#include
- libkern.h:#include
- malloc.h:#include
- mbuf.h:#include
- pmckern.h:#include
- proc.h:#include
- refcount.h:#include
- seqc.h:#include
- sf_buf.h:#include
- zutil.h:#include
This is especially bad in:
- libkern.h. systm.h includes libkern.h, so this recurses. The recursion
is bounded by the reinclude guard. libkern.h was broken by adding
KASSERT()s to it. It is a leaf header, so must be written carefully.
- malloc.h and mbuf.h. I fixed all the include pollution in them. They
are now much more polluted than before I cleaned them
- proc.h. Almost everything includes this. Including sys/systm.h in it
hdes the bug of not including sys/systm.h in the correct place in .c files.
/**
* @defgroup NEWBUS newbus - a generic framework for managing devices
@@ -813,12 +814,9 @@ static __inline type varp ## _get_ ## var(device_t dev
int e; \
e = BUS_READ_IVAR(device_get_parent(dev), dev, \
ivarp ## _IVAR_ ## ivar, ); \
- if (e != 0) { \
- device_printf(dev, "failed to read ivar " \
- __XSTRING(ivarp ## _IVAR_ ## ivar) " on bus %s, " \
- "error = %d\n", \
- device_get_nameunit(device_get_parent(dev)), e);\
- } \
+ KASSERT(e == 0, ("%s failed for %s on bus %s, error = %d",\
+ __func__, device_get_nameunit(dev), \
+ device_get_nameunit(device_get_parent(dev)), e)); \
return ((type) v); \
} \
\
Inline functions in headers generally cause namespace pollution, like here.
Some headers use macros which don't have that problem. mbuf.h used to be
like that. The cleaned version of it in FreeBSD-4 includes only
sys/queue.h. It has rotted to include sys/systm.h, sys/refcount.h,
vm/uma.h. sys/lock.h and sys/sdt.h.
Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r349459 - head/sys/sys
Author: avg
Date: Thu Jun 27 15:07:06 2019
New Revision: 349459
URL: https://svnweb.freebsd.org/changeset/base/349459
Log:
upgrade the warning printf-s in bus accessors to KASSERT-s
After this change sys/bus.h includes sys/systm.h.
Discussed with: cem, imp
MFC after:2 weeks
Modified:
head/sys/sys/bus.h
Modified: head/sys/sys/bus.h
==
--- head/sys/sys/bus.h Thu Jun 27 14:26:57 2019(r349458)
+++ head/sys/sys/bus.h Thu Jun 27 15:07:06 2019(r349459)
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
/**
* @defgroup NEWBUS newbus - a generic framework for managing devices
@@ -813,12 +814,9 @@ static __inline type varp ## _get_ ## var(device_t dev
int e; \
e = BUS_READ_IVAR(device_get_parent(dev), dev, \
ivarp ## _IVAR_ ## ivar, ); \
- if (e != 0) { \
- device_printf(dev, "failed to read ivar " \
- __XSTRING(ivarp ## _IVAR_ ## ivar) " on bus %s, " \
- "error = %d\n", \
- device_get_nameunit(device_get_parent(dev)), e);\
- } \
+ KASSERT(e == 0, ("%s failed for %s on bus %s, error = %d", \
+ __func__, device_get_nameunit(dev), \
+ device_get_nameunit(device_get_parent(dev)), e)); \
return ((type) v); \
} \
\
@@ -828,12 +826,9 @@ static __inline void varp ## _set_ ## var(device_t dev
int e; \
e = BUS_WRITE_IVAR(device_get_parent(dev), dev, \
ivarp ## _IVAR_ ## ivar, v);\
- if (e != 0) { \
- device_printf(dev, "failed to write ivar " \
- __XSTRING(ivarp ## _IVAR_ ## ivar) " on bus %s, " \
- "error = %d\n", \
- device_get_nameunit(device_get_parent(dev)), e);\
- } \
+ KASSERT(e == 0, ("%s failed for %s on bus %s, error = %d", \
+ __func__, device_get_nameunit(dev), \
+ device_get_nameunit(device_get_parent(dev)), e)); \
}
/**
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
