Re: [RFC/RFT] calloutng

On Sat, 15 Dec 2012, Garrett Cooper wrote:

> On Dec 15, 2012, at 12:34 PM, Mark Johnston wrote:
>
>> On Sat, Dec 15, 2012 at 06:55:53PM +0200, Alexander Motin wrote:
>>> Hi.
>>>
>>> I'm sorry to interrupt review, but as usual good ideas came during the
>>> final testing, causing another round. :)  Here is updated patch for
>>> HEAD, that includes several new changes:
>>> http://people.freebsd.org/~mav/calloutng_12_15.patch
>>
>> This patch breaks the libprocstat build.
>>
>> Specifically, the OpenSolaris sys/time.h defines the preprocessor
>> symbols gethrestime and gethrestime_sec. These symbols are also defined
>> in cddl/contrib/opensolaris/lib/libzpool/common/sys/zfs_context.h.
>> libprocstat:zfs.c is compiled using include paths that pick up the
>> OpenSolaris time.h, and with this patch _callout.h includes sys/time.h.
>>
>> zfs.c includes taskqueue.h (with _KERNEL defined), which includes
>> _callout.h, so both time.h and zfs_context.h are included in zfs.c, and
>> the symbols are thus defined twice.

Gross namespace pollution.  sys/_callout.h exists so that the full
namespace pollution of sys/callout.h doesn't get included nested.  But
sys/time.h is much more polluted than sys/callout.h.

However, sys/time.h is old standard pollution in sys/param.h, and
sys/callout.h is not so old standard pollution in sys/systm.h.  It is
a bug to not include sys/param.h and sys/systm.h in most kernel source
code, so these nested includes are just style bugs -- they have no
effect for correct kernel source code.

>> The patch below fixes the build for me. Another approach might be to
>> include sys/_task.h instead of taskqueue.h at the beginning of zfs.c.

Good if it works.

> 	I had a patch open once upon a time to cleanup inclusion of sys/time.h all over the tree and deal with the sys/time.h <-> time.h pollution issue, but it got dropped due to lack of interest (20~30 apps/libs were affected IIRC and I only really got assistance in fixing the UFS and bsnmpd pieces, and gave up due to lack of response from maintainers). dtrace/zfs is a definite instigator in this pollution (I remember nasty cddl/... pollution with the compat sys/time.h header).

Please use the unix newline character in mail.  The above is difficult to
quote.

The standard sys/time.h pollution in sys/param.h is only in the kernel,
and there aren't many direct includes of sys/time.h in the kernel.  Userland
is different and many of the direct includes were correct.  But not POSIX
specifies that struct timespec and struct timeval be defined in most places
where they are needed, so the includes of sys/time.h are not necessary
for POSIX or FreeBSD, although FreeBSD man pages still say that they
are necessary.  The sys/time.h <-> time.h pollution issue is also only
for userland.  Many places depend on one including the other, and include
the wrong one themself.

> 	Bottom line: make sure anything new you're defining isn't already defined via POSIX or other OSes, and if so please try to make the implementations match (so that eventual POSIX inclusion might be possible) and when in doubt I suggest consulting standards_at_ / brde_at_.

Bruce