Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

On Sat, 26 Nov 2011, Robert Millan wrote:

> On Fri, Nov 25, 2011 at 11:16:15AM -0700, Warner Losh wrote:
>> Hey Bruce,
>>
>> These sound like good suggestions, but I'd hoped to actually go through all these files with a fine-toothed comb to see which ones were still relevant.  You've found a bunch of good areas to clean up, but I'd like to humbly suggest they be done in a follow-on commit.
>
> Hi,
>
> I'm sending a new patch.  Thanks Bruce for your input.  TTBOMK this corrects
> all the problems you spotted that were introduced by my patch.  It doesn't
> fix pre-existing problems in the files however, except in cases where I had
> to modify that line anyway.
>
> I think it's a good compromise between my initial patch and an exhaustive
> cleanup of those headers (which I'm probably not the most indicate for).

It fixes most style bugs, but not some-pre-existing problems, even in cases
where you had to modify the line anyway.

% Index: sys/cam/scsi/scsi_low.h
% ===================================================================
% --- sys/cam/scsi/scsi_low.h	(revision 227956)
% +++ sys/cam/scsi/scsi_low.h	(working copy)
% _at__at_ -53,10 +53,10 _at__at_
%  #define	SCSI_LOW_INTERFACE_XS
%  #endif	/* __NetBSD__ */
% 
% -#ifdef	__FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
%  #define	SCSI_LOW_INTERFACE_CAM
%  #define	CAM
% -#endif	/* __FreeBSD__ */
% +#endif /* __FreeBSD__ || __FreeBSD_kernel__ */

It still has the whitespace-after tab style change for cam.

% Index: sys/dev/firewire/firewirereg.h
% ===================================================================
% --- sys/dev/firewire/firewirereg.h	(revision 227956)
% +++ sys/dev/firewire/firewirereg.h	(working copy)
% _at__at_ -75,7 +75,8 _at__at_
%  };
% 
%  struct firewire_softc {
% -#if defined(__FreeBSD__) && __FreeBSD_version >= 500000
% +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && \
% +    __FreeBSD_version >= 500000
%  	struct cdev *dev;
%  #endif
%  	struct firewire_comm *fc;

Here is a pre-existing problem that you didn't fix on a line that you
changed.  The __FreeBSD__ ifdef is nonsense here, since __FreeBSD__
being defined has nothing to do with either whether __FreeBSD_version
is defined or whether there is a struct cdev * in the data structure.

Previously:
- defined(__FreeBSD__) means that the compiler is for FreeBSD
- __FreeBSD_version >= 500000 means that FreeBSD <sys/param.h> has
   been included and has defined __FreeBSD_version to a value that
   satisifes this.  It would be a bug for anything else to define
   __FreeBSD_version.  Unfortunately, there is a bogus #undef of
   __FreeBSD_version that breaks detection of other things defining
   it.
- the __FreeBSD__ part of the test has no effect except to break
   compiling this file with a non-gcc compiler.  In particular,
   it doesn't prevent errors for -Wundef -Werror.  But other ifdefs
   in this file use an unguarded __FreeBSD_version.  Thus this file
   never worked with -Wundef -Werror, and the __FreeBSD__ part has
   no effect except the breakage.

Now: as above, except:
- defined(__FreeBSD_kernel__) means that FreeBSD <sys/param.h>
   been included and that this header is new enough to define
   __FreeBSD_kernel__.  This has the same bug with the #undef,
   which I pointed out before (I noticed it for this but not
   for __FreeBSD_version).  And it has a style bug in its name
   which I pointed out before -- 2 underscores in its name.
   __FreeBSD_version doesn't have this style bug.  The definition
   of __FreeBSD_kernel__ has already been committed.  Is it too
   late to fix its name?
- when <sys/param.h> is new enough to define __FreeBSD_kernel__,
   it must be new enough to define __FreeBSD_version >= 500000.
   Thus there is now no -Wundef error.
- the __FreeBSD__ ifdef remains nonsense.  If you just removed it,
   then you wouldn't need the __FreeBSD_kernel__ ifdef (modulo the
   -Wundef error).  You didn't add the __FreeBSD_kernel__ ifdef to
   any of the other lines with the __FreeBSD_kernel__ ifdef in this
   file, apparently because the others don't have the nonsensical
   __FreeBSD__ ifdef.

The nonsense and changes to work around it make the logic for this
ifdef even more convoluted and broken than might first appear.  In
a previous patchset, you included <sys/param.h> to ensure that
__FreeBSD_kernel__ is defined for newer kernel sources (instead of
testing if it is defined).  Ifdefs like the above make <sys/param.h>
a prerequsite for this file anyway, since without knowing
__FreeBSD_version it is impossible to determine if the data structure
has new fields like the cdev in it.  <sys/param.h> is a prerequisite
for almost all kernel .c files, so this prerequisite should be satisfied
automatically for them, but it isn't clear what happens for user .c files.
I think the ifdef should be something like the following to enforce the
prerequisite:

#ifndef _SYS_PARAM_H_
/*
  * Here I don't support __FreeBSD_version__ to be set outside of
  * <sys/param.h> to hack around a missing include of <sys/param.h>.
  * The case where the kernel is so old that __FreeBSD_version__ is
  * not defined should be handled by including <sys/param.h> to see
  * if __FreeBSD_version__ is in fact not defined.
  */
#error "<sys/param.h> is a prerequisite for this file"
#endif
#if __FreeBSD_version >= 500000
/*
  * Add defined(__FreeBSD_version) to the ifdef if you want to fully
  * support -Wundef.  This is unlikely to have any effect here, since
  * <sys/param.h> has been included, and it defines __FreeBSD_version
  * except under very old versions of FreeBSD where -Wundef was even
  * more unusable than now.
  */
 	struct cdev *dev;
#endif

Similarly in most places that test __FreeBSD__ and __FreeBSD_version,
or __FreeBSD__ and DEVICE_POLLING (the latter might need to ensure
that opt_device_polling.h instead of <sys/param.h> is included, so in
userland it reduces to an unconditional #error or hacks since
opt_device_polling.h is a kernel-only non-module-only header).

Bruce