Date: Fri, 25 Nov 2011 11:16:15 -0700 From: Warner Losh <imp@bsdimp.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Kostik Belousov <kostikbel@gmail.com>, freebsd-arch@freebsd.org, Adrian Chadd <adrian@freebsd.org>, freebsd-current@freebsd.org, Robert Millan <rmh@freebsd.org> Subject: Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2) Message-ID: <FBFD9FCA-1B01-4B66-8350-16F0410DE0F9@bsdimp.com> In-Reply-To: <20111125150324.G1018@besplex.bde.org> References: <20111123070036.GA29952@thorin> <20111124141821.O932@besplex.bde.org> <CAOfDtXMaHCW4rfEhzfhThGn6kY0=%2B209VhqFSBq9EF03fZLBhw@mail.gmail.com> <20111125150324.G1018@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Warner
On Nov 24, 2011, at 10:54 PM, Bruce Evans wrote:
> On Thu, 24 Nov 2011, Robert Millan wrote:
>=20
>> 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.
>>=20
>> 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.
>>=20
>> So in this particular case maybe it's better to use the other =
approach?
>>=20
>> See attachment.
>=20
> 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 :-).)
>=20
> % 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__ */
>=20
> 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).
>=20
> 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.
>=20
> % @@ -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>
>=20
> 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.
>=20
> % [... similarly throught cam]
>=20
> % 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
> %
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> % 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 */
>=20
> 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 ||.)
>=20
> [... similarly for all the pf headers]
>=20
> % 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 >=3D 500000
> % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && =
__FreeBSD_version >=3D 500000
> % struct cdev *dev;
> % #endif
> % struct firewire_comm *fc;
>=20
> The line is now too long.
>=20
> 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 >=3D 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.
>=20
> 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).
>=20
> 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.
>=20
> % 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
>=20
> 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.
>=20
> % @@ -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() */
>=20
> As above.
>=20
> % @@ -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
>=20
> 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.
>=20
> % @@ -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 =
*/
>=20
> 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.
>=20
> % @@ -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) \
>=20
> Another long line. The /* unconditionally */ comment is now even more
> grossly mismatched with the code.
>=20
> % @@ -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
>=20
> 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.
>=20
> % @@ -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 *);
>=20
> 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.
>=20
> % @@ -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);
>=20
> This file is especially bad.
>=20
> % 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 =3D ticks / 1000; \*/
> % /* (x)->tv_usec =3D (ticks % 1000) * 1000; \*/
>=20
> Line too long.
>=20
> % 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);
> %
>=20
> Lines too long. How would the kernel options be defined for =
non-FreeBSD?
> I.e., why have any __FreeBSD* ifdefs at all here?
>=20
>=20
> % 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;
>=20
> Line too long.
>=20
> % 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 +=3D _d)
> % #define SCTP_STAT_DECR_BY(_x,_d) =
(SCTP_BASE_STATS[PCPU_GET(cpuid)]._x -=3D _d)
> % #else
>=20
> As usual.
>=20
> Bruce
> _______________________________________________
> freebsd-arch@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to =
"freebsd-arch-unsubscribe@freebsd.org"
>=20
>=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?FBFD9FCA-1B01-4B66-8350-16F0410DE0F9>