svn commit: r349151 - in head: lib/libufs stand/libsa sys/conf sys/dev/iscsi sys/dev/iscsi_initiator sys/dev/liquidio sys/dev/usb/net sys/fs/ext2fs sys/fs/nandfs sys/geom/part sys/geom/raid sys/ker...

[Konstantin Belousov]

On Sat, Jun 22, 2019 at 09:13:05PM -0700, Xin Li wrote:
> On 6/22/19 17:15, Conrad Meyer wrote:
> > Hi Xin Li,
> > 
> > On Mon, Jun 17, 2019 at 12:49 PM Xin LI <delphij at freebsd.org> wrote:
> >>
> >> Author: delphij
> >> Date: Mon Jun 17 19:49:08 2019
> >> New Revision: 349151
> >> URL: https://svnweb.freebsd.org/changeset/base/349151
> >>
> >> Log:
> >>   Separate kernel crc32() implementation to its own header (gsb_crc32.h) and
> >>   rename the source to gsb_crc32.c.
> >>
> >>   This is a prerequisite of unifying kernel zlib instances.
> >>
> >> ...
> >> Modified: head/sys/libkern/x86/crc32_sse42.c
> >> ==============================================================================
> >> --- head/sys/libkern/x86/crc32_sse42.c  Mon Jun 17 17:35:55 2019        (r349150)
> >> +++ head/sys/libkern/x86/crc32_sse42.c  Mon Jun 17 19:49:08 2019        (r349151)
> >> @@ -29,14 +29,14 @@ __FBSDID("$FreeBSD$");
> >>  /*
> >>   * This file is compiled in userspace in order to run ATF unit tests.
> >>   */
> >> -#ifdef USERSPACE_TESTING
> >> +#ifndef _KERNEL
> > 
> > This change and following identical changes revert a request by kib@
> > in https://reviews.freebsd.org/D9342 .  (When this revision was
> > initially proposed in  , it was '#ifndef _KERNEL' — kib@ requested the
> > use of a different preprocessor macro.)
> 
> Thanks for pointing this out -- admittedly, I haven't read the reasoning
> before.
> 
> But after reading more about the original review context, I don't quite
> agree with Kostik's reasoning because the code actually works in
> userland regardless of the original intention.  It's true that the code
> is at the time only used in the test program, but they do enabled usage
> in userland, and they by themselves do not serve purely test purposed
> task (these are not mock interfaces, nor test cases, etc.), and the
> common practice in sys/libkern is to wrap these with #if[n]def _KERNEL.
> 
> I don't insist but I still think it's more reasonable to use _KERNEL
> because it's more consistent with the surrounding code.

#ifndef _KERNEL is used when you provide generally useful code which must
be limited to userspace.  There, the conditionally compiled code is very
specific, it is only for testing.  Being explicit would allow to avoid
questions, e.g. why userspace requires constructors.

And still see my confusion in the referenced review.