Date: Sun, 27 Nov 2011 14:16:20 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Robert Millan <rmh@freebsd.org> Cc: Adrian Chadd <adrian@freebsd.org>, freebsd-current@freebsd.org, Bruce Evans <brde@optusnet.com.au>, freebsd-arch@freebsd.org, Kostik Belousov <kostikbel@gmail.com>, Warner Losh <imp@bsdimp.com> Subject: Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2) Message-ID: <20111127131422.D802@besplex.bde.org> In-Reply-To: <20111126200749.GA72056@thorin> References: <20111123070036.GA29952@thorin> <20111124141821.O932@besplex.bde.org> <CAOfDtXMaHCW4rfEhzfhThGn6kY0=%2B209VhqFSBq9EF03fZLBhw@mail.gmail.com> <20111125150324.G1018@besplex.bde.org> <FBFD9FCA-1B01-4B66-8350-16F0410DE0F9@bsdimp.com> <20111126200749.GA72056@thorin>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
% @@ -53,10 +53,10 @@
% #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)
% @@ -75,7 +75,8 @@
% };
%
% 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111127131422.D802>