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
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
