Date: Fri, 25 Nov 2011 16:54:54 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Robert Millan <rmh@FreeBSD.org> Cc: Kostik Belousov <kostikbel@gmail.com>, Adrian Chadd <adrian@FreeBSD.org>, freebsd-current@FreeBSD.org, Bruce Evans <brde@optusnet.com.au>, freebsd-arch@FreeBSD.org Subject: Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2) Message-ID: <20111125150324.G1018@besplex.bde.org> In-Reply-To: <CAOfDtXMaHCW4rfEhzfhThGn6kY0=%2B209VhqFSBq9EF03fZLBhw@mail.gmail.com> References: <20111123070036.GA29952@thorin> <20111124141821.O932@besplex.bde.org> <CAOfDtXMaHCW4rfEhzfhThGn6kY0=%2B209VhqFSBq9EF03fZLBhw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 24 Nov 2011, Robert Millan wrote:
> 2011/11/24 Bruce Evans <brde@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.
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.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111125150324.G1018>