[PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)
Robert Millan
Here we go again :-)
Out of the kernel headers that are installed in /usr/include/ hierracy, there
are some which include support multiple operating systems (usually FreeBSD and
other *BSD flavours).
This patch adds support to detect GNU/kFreeBSD as well. In all cases, we
match the same declarations as FreeBSD does (which is to be expected in kernel
headers, since both systems share the same kernel).
--
Robert Millan
Bruce Evans
Now it adds lots of namespace pollution (all of <sys/param.h>, including
all of its namespace pollution), just to get 1 new symbol defined.
% Index: sys/cam/scsi/scsi_low.h
% ===================================================================
% --- sys/cam/scsi/scsi_low.h (revision 227831)
% +++ sys/cam/scsi/scsi_low.h (working copy)
% @@ -44,6 +44,8 @@
% #ifndef _SCSI_LOW_H_
% #define _SCSI_LOW_H_
%
% +#include <sys/param.h>
% +
% /*================================================
% * Scsi low OSDEP
% * (All os depend structures should be here!)
%
% [... 22 more headers polluted]
All the affected headers are poorly implemented ones. Mostly kernel
headers which escaped to userland.
Bruce
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"
Robert Millan
2011/11/24 Bruce Evans <br...@optusnet.com.au>:
> Now it adds lots of namespace pollution (all of <sys/param.h>, including
> all of its namespace pollution), just to get 1 new symbol defined.
Well, my initial patch (see mail with same subject modulo "v2") didn't
have this problem. Now that __FreeBSD_kernel__ is defined, many
#ifdefs can be simplified, but maybe it's not desireable for all of
them. At least not until we can rely on the compiler to define this
macro.
So in this particular case maybe it's better to use the other approach?
See attachment.
Bruce Evans
That is clean enough, except for some style bugs. (I thought of worse
ways like duplicating the logic of <sys/param.h>, or directing
<sys/param.h> to only declare version macros, or putting version macros
in a little separate param header and including that. The latter would
be cleanest, but gives even more includes, and not worth it for this,
but it would have been better for __FreeBSD_version. I don't like
having to recompile half the universe according to dependencies on
<sys/param.h> because only __FreeBSD_version__ in it changed. Basic
headers rarely change apart from that. BTW, a recent discussion in
the POSIX mailing list says that standardized generation of depenedencies
should not generate dependencies on system headers. This would break
the effect of putting mistakes like __FreeBSD_version__ in any system
header :-).)
% diff -ur sys.old/cam/scsi/scsi_low.h sys/cam/scsi/scsi_low.h
% --- sys.old/cam/scsi/scsi_low.h 2007-12-25 18:52:02.000000000 +0100
% +++ sys/cam/scsi/scsi_low.h 2011-11-13 14:12:41.121908380 +0100
% @@ -53,7 +53,7 @@
% #define SCSI_LOW_INTERFACE_XS
% #endif /* __NetBSD__ */
%
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
% #define SCSI_LOW_INTERFACE_CAM
% #define CAM
% #endif /* __FreeBSD__ */
This also fixes some style bugs (tab instead of space after `#ifdef').
But it doesn't fix others (tab instead of space after `#ifdef', and
comment on a short ifdef). And it introduces a new one (the comment
on the ifdef now doesn't even match the code).
cam has a highly non-KNF style, so it may require all of these style
bugs except the comment not matching the code. This makes it hard
for non-cam programmers to maintain. According to grep, it prefers
a tab to a space after `#ifdef' by a ratio of 89:38 in a version
checked out a year or two ago. But in 9.0-BETA1, the counts have
blown out and the ratio has reduced to 254:221. The counts are
more than doubled because the first version is a cvs checkout and
the second version is a svn checkout, and it is too hard to filter
out the svn duplicates. I guess the ratio changed because the new
ata subsystem is not bug for bug compatible with cam style. Anywyay,
there never was a consistent cam style to match.
% @@ -64,7 +64,7 @@
% #include <dev/isa/ccbque.h>
% #endif /* __NetBSD__ */
%
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
% #include <sys/device_port.h>
% #include <sys/kdb.h>
% #include <cam/cam.h>
Same problems, but now the ifdef is larger (but not large enough to
need a comment on its endif), so the inconsistent comment is not
visible in the patch.
% [... similarly throught cam]
% diff -ur sys.old/contrib/altq/altq/if_altq.h sys/contrib/altq/altq/if_altq.h
% --- sys.old/contrib/altq/altq/if_altq.h 2011-03-10 19:49:15.000000000 +0100
% +++ sys/contrib/altq/altq/if_altq.h 2011-11-13 14:12:41.119907128 +0100
% @@ -29,7 +29,7 @@
% #ifndef _ALTQ_IF_ALTQ_H_
% #define _ALTQ_IF_ALTQ_H_
%
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
% #include <sys/lock.h> /* XXX */
% #include <sys/mutex.h> /* XXX */
% #include <sys/event.h> /* XXX */
% @@ -51,7 +51,7 @@
% int ifq_len;
% int ifq_maxlen;
% int ifq_drops;
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
% struct mtx ifq_mtx;
% #endif
%
No new problems, but I wonder how this even compiles when the ifdefs
are not satisfed. Here we are exporting mounds of kernel data structures
to userland. There is a similar mess in <net/if_var.h>. There it has
no ifdefs at all for the lock, mutex and event headers there, and you
didn't touch it. <net/if_var.h> is unfortunately actually needed in
userland. The mutexes in its data structures cannot simply be left
out, since then the data structures become incompatible with the actual
ones. I don't see how the above can work with the mutex left out.
By "not even compiles", I meant the header itself, but there should be
no problems there because the second ifdef should kill the only use of
all the headers. And userland should compile since it shouldn't use
the ifdefed out (kernel) parts of the data struct. But leaving out
the data substructures changes the ABI, so how could any application that
actually uses the full structure work? And if nothing uses it, it
shouldn't be exported.
Exporting the full pollution of sys/lock.h, sys/mutex.h and sys/event.h
to userland is probably an implementation bug. This is partially fixed
in my version if <net/if_var.h> by including these headers only for
the _KERNEL case. sys/_lock.h and sys/_mutex.h are enough for declaring
the mutex, and sys/event*.h isn't even used in the userland parts of
<net/if_var.h>. But this probably wouldn't help you much, since the
underscored mutex headers are just as unavailable as the non-underscored
ones on non-FreeBSD systems.
I checked that this file doesn't have any comments on the changed ifdefs
to get out of sync (it just has a misformatted one for the endif for
_KERNEL and a backwards one for the one for the idempotency endif).
I didn't check this for other files.
% diff -ur sys.old/contrib/pf/net/if_pflog.h sys/contrib/pf/net/if_pflog.h
% --- sys.old/contrib/pf/net/if_pflog.h 2011-06-28 13:57:25.000000000 +0200
% +++ sys/contrib/pf/net/if_pflog.h 2011-11-13 14:12:41.130906469 +0100
% @@ -30,7 +30,7 @@
% #define PFLOGIFS_MAX 16
%
% struct pflog_softc {
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
% struct ifnet *sc_ifp; /* the interface pointer */
% #else
% struct ifnet sc_if; /* the interface */
Another very fundamental ABI difference in a clearer form. It this only
used in the kernel, and if so, why is it exported to userland? Wouldn't
a _KERNEL ifdef work better for avoiding the latter than a
__FreeBSD_kernel__ ifdef? (It would be with && instead of ||.)
[... similarly for all the pf headers]
% diff -ur sys.old/dev/firewire/firewirereg.h sys/dev/firewire/firewirereg.h
% --- sys.old/dev/firewire/firewirereg.h 2007-07-20 05:42:57.000000000 +0200
% +++ sys/dev/firewire/firewirereg.h 2011-11-13 14:12:41.122907609 +0100
% @@ -75,7 +75,7 @@
% };
%
% 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;
The line is now too long.
This change seems to be wrong. I think the __FreeBSD__ ifdef is only
there because of broken compilers. The correct code is simply `#if
__FreeBSD_version >= 500000', where with non-broken compilers
__FreeBSD_version is replaced by 0 if it is not defined. But broken
compilers warn when undefined identifiers are used in arithmetic cpp
expressions. Some users like to break their compilers using -Wundef
to give the warnings; -Werror then gives full breakage (C compilers
are permitted to give spurious diagnostics but not to fail to compile
C code.) This is normally worked around by checking if the identifier
is defined before it is used in an arithmetic expression. But here a
different identifier is checked for being defined. FreeBSD gcc defines
both __FreeBSD__ and __FreeBSD_version__ together, so this works under
FreeBSD. I think it only works accidentally under FreeBSD, and it
still doesn't work with your changed ifdef under non-FreeBSD.
grepping for `FreeBSD.*FreeBSD_version' in $(find /sys -name *.h) shows
very few lines, and even fewer lines with this bug. Most matching
lines do the unsurprising check that __FreeBSD_version__ is defined
before using it. The exceptions are just the above, 2 lines in
if_lmc.h and about 5 lines in ipfilter (some on comments on endifs
for ipfilter. Even ipfilter does the unsurprising comparison in
4 lines).
grepping for `FreeBSD_version' in $(find /sys -name *.h) shows many more
lines than the above. It follows that many headers cannot be compiled
by broken compilers, and little would be lost, at least in the kernel,
by removing all the checks that __FreeBSD_version__ is defined before
using it. But since this inconsistency intersects with your changes,
perhaps the checks are there mainly for cases that escape to userland,
where the broken compilers are more common.
% diff -ur sys.old/dev/lmc/if_lmc.h sys/dev/lmc/if_lmc.h
% --- sys.old/dev/lmc/if_lmc.h 2009-11-19 19:21:51.000000000 +0100
% +++ sys/dev/lmc/if_lmc.h 2011-11-13 14:12:41.124908302 +0100
% @@ -984,7 +984,7 @@
% #endif
% u_int32_t address1; /* buffer1 bus address */
% u_int32_t address2; /* buffer2 bus address */
% -#if (defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__))
% +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__NetBSD__) || defined(__OpenBSD__))
% bus_dmamap_t map; /* bus dmamap for this descriptor */
% # define TLP_BUS_DSL_VAL (sizeof(bus_dmamap_t) & TLP_BUS_DSL)
% #else
The line is now too long. ABI differences when __FreeBSD__ etc. is not
defined indicated that the declaration probably doesn't actually work
without the definitions, so the whole struct probably shouldn't be
exported.
% @@ -1035,7 +1035,7 @@
% #elif BSD
% struct mbuf *head; /* tail-queue of mbufs */
% struct mbuf *tail;
% -# if (defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__))
% +# if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__NetBSD__) || defined(__OpenBSD__))
% bus_dma_tag_t tag; /* bus_dma tag for desc array */
% bus_dmamap_t map; /* bus_dma map for desc array */
% bus_dma_segment_t segs[2]; /* bus_dmamap_load() or bus_dmamem_alloc() */
As above.
% @@ -1068,7 +1068,7 @@
% */
% #define IOREF_CSR 1 /* access Tulip CSRs with IO cycles if 1 */
%
% -#if (defined(__FreeBSD__) && defined(DEVICE_POLLING))
% +#if ((defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && defined(DEVICE_POLLING))
% # define DEV_POLL 1
% #else
% # define DEV_POLL 0
As above, plus DEVICE_POLLING is only defined in a kernel options header,
so there shouldn't be ifdefs on it in header files, even in the kernel.
% @@ -1151,7 +1151,7 @@
% # endif
% #endif
%
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
% struct callout callout; /* watchdog needs this */
% struct device *dev; /* base device pointer */
% bus_space_tag_t csr_tag; /* bus_space needs this */
But I may misunderstand how __FreeBSD_kernel__ works. Data structures
containing kernel things like the kernel watchdog callout shouldn't be
exported. Given that they are, they should be exported with the full
kernel mess so that their layout for the non-kernel parts doesn't
depend on ifdefs. The __FreeBSD__ ifdef was bogus, but had no effect
under FreeBSD since FreeBSD gcc always defines it. A _KERNEL ifdef
would have given a wrong ABI in userland. Your __FreeBSD_kernel__
ifdef seems to be equivalent to a _KERNEL ifdef for non-FreeBSD. Thus
it gives a changed ABI for non-FreeBSD. I don't see how the changed
ABI can work unless it is not used. But if it is not used, then the
whold ABI should be ifdefed out. This may be beyond the scope of
your changes.
% @@ -1428,7 +1428,7 @@
% #endif
%
% #if (defined(__bsdi__) || /* unconditionally */ \
% - (defined(__FreeBSD__) && (__FreeBSD_version < 503000)) || \
% + ((defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && (__FreeBSD_version < 503000)) || \
% (defined(__NetBSD__) && (__NetBSD_Version__ < 106000000)) || \
% (defined(__OpenBSD__) && ( OpenBSD < 200111)))
% # define IFQ_ENQUEUE(ifq, m, pa, err) \
Another long line. The /* unconditionally */ comment is now even more
grossly mismatched with the code.
% @@ -1531,7 +1531,7 @@
% static int t1_ioctl(softc_t *, struct ioctl *);
%
% #if IFNET
% -# if ((defined(__FreeBSD__) && (__FreeBSD_version < 500000)) ||\
% +# if (((defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && __FreeBSD_version < 500000) ||\
% defined(__NetBSD__) || defined(__OpenBSD__) || defined(__bsdi__))
% static void netisr_dispatch(int, struct mbuf *);
% # endif
Another long line. There are many old style bugs in these ifdefs, starting
with __bsdi__ being first in the previous ifdef and last in this one.
% @@ -1570,7 +1570,7 @@
% static void core_interrupt(void *, int);
% static void user_interrupt(softc_t *, int);
% #if BSD
% -# if (defined(__FreeBSD__) && defined(DEVICE_POLLING))
% +# if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && defined(DEVICE_POLLING)
% static int fbsd_poll(struct ifnet *, enum poll_cmd, int);
% # endif
% static intr_return_t bsd_interrupt(void *);
Another long line. Now the declaration is of a kernel function, so the
condition for if is clearly nonsense. Why not just `#ifdef DEVICE_POLLING'
or no ifdef at all. The function is static in a .c file, so declaring it
as static in a public header is worse than its ifdefs. Better yet, the
ifdef for it here didn't match the ifdef for it in the .c file, and is
now further from matching (the C file has an ifdef on BSD, __FreeBSD__
and DEVICE_POLLING, while the header file had an ifdef on _KERNEL,
KERNEL, __KERNEL__, __FreeBSD__ and DEVICE_POLLING). Fixing mistakes in
DEVICE_POLLING is clearly beyond the scope of your changes.
% @@ -1638,7 +1638,7 @@
% static int attach_card(softc_t *, const char *);
% static void detach_card(softc_t *);
%
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
% static int fbsd_probe(device_t);
% static int fbsd_detach(device_t);
% static int fbsd_shutdown(device_t);
This file is especially bad.
% diff -ur sys.old/netinet/sctp_constants.h sys/netinet/sctp_constants.h
% --- sys.old/netinet/sctp_constants.h 2011-09-17 10:50:29.000000000 +0200
% +++ sys/netinet/sctp_constants.h 2011-11-13 14:12:41.145908872 +0100
% @@ -1020,7 +1020,7 @@
% #define SCTP_GETTIME_TIMEVAL(x) (getmicrouptime(x))
% #define SCTP_GETPTIME_TIMEVAL(x) (microuptime(x))
% #endif
% -/*#if defined(__FreeBSD__) || defined(__APPLE__)*/
% +/*#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__APPLE__)*/
% /*#define SCTP_GETTIME_TIMEVAL(x) { \*/
% /* (x)->tv_sec = ticks / 1000; \*/
% /* (x)->tv_usec = (ticks % 1000) * 1000; \*/
Line too long.
% diff -ur sys.old/netinet/sctp_pcb.h sys/netinet/sctp_pcb.h
% --- sys.old/netinet/sctp_pcb.h 2011-09-14 10:15:21.000000000 +0200
% +++ sys/netinet/sctp_pcb.h 2011-11-13 14:12:41.148909632 +0100
% @@ -240,7 +240,7 @@
% * All static structures that anchor the system must be here.
% */
% struct sctp_epinfo sctppcbinfo;
% -#if defined(__FreeBSD__) && defined(SMP) && defined(SCTP_USE_PERCPU_STAT)
% +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && defined(SMP) && defined(SCTP_USE_PERCPU_STAT)
% struct sctpstat *sctpstat;
% #else
% struct sctpstat sctpstat;
% @@ -632,7 +632,7 @@
% struct sctp_inpcb *,
% uint8_t co_off);
%
% -#if defined(__FreeBSD__) && defined(SCTP_MCORE_INPUT) && defined(SMP)
% +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && defined(SCTP_MCORE_INPUT) && defined(SMP)
% void
% sctp_queue_to_mcore(struct mbuf *m, int off, int cpu_to_use);
%
Lines too long. How would the kernel options be defined for non-FreeBSD?
I.e., why have any __FreeBSD* ifdefs at all here?
% diff -ur sys.old/netinet/sctp_structs.h sys/netinet/sctp_structs.h
% --- sys.old/netinet/sctp_structs.h 2011-10-10 18:31:18.000000000 +0200
% +++ sys/netinet/sctp_structs.h 2011-11-13 14:12:41.150907531 +0100
% @@ -108,7 +108,7 @@
% typedef int (*inp_func) (struct sctp_inpcb *, void *ptr, uint32_t val);
% typedef void (*end_func) (void *ptr, uint32_t val);
%
% -#if defined(__FreeBSD__) && defined(SCTP_MCORE_INPUT) && defined(SMP)
% +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && defined(SCTP_MCORE_INPUT) && defined(SMP)
% /* whats on the mcore control struct */
% struct sctp_mcore_queue {
% TAILQ_ENTRY(sctp_mcore_queue) next;
Line too long.
% diff -ur sys.old/netinet/sctp_uio.h sys/netinet/sctp_uio.h
% --- sys.old/netinet/sctp_uio.h 2011-08-14 22:55:32.000000000 +0200
% +++ sys/netinet/sctp_uio.h 2011-11-13 14:12:41.152905989 +0100
% @@ -1056,7 +1056,7 @@
%
% #define SCTP_STAT_INCR(_x) SCTP_STAT_INCR_BY(_x,1)
% #define SCTP_STAT_DECR(_x) SCTP_STAT_DECR_BY(_x,1)
% -#if defined(__FreeBSD__) && defined(SMP) && defined(SCTP_USE_PERCPU_STAT)
% +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && defined(SMP) && defined(SCTP_USE_PERCPU_STAT)
% #define SCTP_STAT_INCR_BY(_x,_d) (SCTP_BASE_STATS[PCPU_GET(cpuid)]._x += _d)
% #define SCTP_STAT_DECR_BY(_x,_d) (SCTP_BASE_STATS[PCPU_GET(cpuid)]._x -= _d)
% #else
As usual.
Warner Losh
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.
Warner
> freebs...@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch...@freebsd.org"
Robert Millan
> 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).
--
Robert Millan
Warner Losh
Warner
On Nov 26, 2011, at 1:07 PM, Robert Millan wrote:
> <gnu-kfreebsd_headers.diff>
Bruce Evans
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
Robert Millan
2011/11/27 Bruce Evans <br...@optusnet.com.au>:
> % 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.
My initial patch didn't have it. Precisely I thought that you
requested this in your first mail. Did I missunderstand?
> % 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.
Yes. I didn't say I would fix all pre-existing problems in lines that
need to be modified. In some cases I did, and in some cases I opted
to preserve the status quo.
> __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,
Earlier you asked not to include <sys/param.h>. If <sys/param.h> is a
prerequisite, why not just include it then?