Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

From: Warner Losh <imp_at_bsdimp.com>
Date: Fri, 25 Nov 2011 11:16:15 -0700
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:
> 
>> 2011/11/24 Bruce Evans <brde_at_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
> % _at__at_ -53,7 +53,7 _at__at_
> %  #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.
> 
> % _at__at_ -64,7 +64,7 _at__at_
> %  #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
> % _at__at_ -29,7 +29,7 _at__at_
> %  #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 */
> % _at__at_ -51,7 +51,7 _at__at_
> %  	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
> % _at__at_ -30,7 +30,7 _at__at_
> %  #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
> % _at__at_ -75,7 +75,7 _at__at_
> %  };
> % %  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
> % _at__at_ -984,7 +984,7 _at__at_
> %  #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.
> 
> % _at__at_ -1035,7 +1035,7 _at__at_
> %  #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.
> 
> % _at__at_ -1068,7 +1068,7 _at__at_
> %   */
> %  #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.
> 
> % _at__at_ -1151,7 +1151,7 _at__at_
> %  # 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.
> 
> % _at__at_ -1428,7 +1428,7 _at__at_
> %  #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.
> 
> % _at__at_ -1531,7 +1531,7 _at__at_
> %  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.
> 
> % _at__at_ -1570,7 +1570,7 _at__at_
> %  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.
> 
> % _at__at_ -1638,7 +1638,7 _at__at_
> %  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
> % _at__at_ -1020,7 +1020,7 _at__at_
> %  #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
> % _at__at_ -240,7 +240,7 _at__at_
> %  	 * 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;
> % _at__at_ -632,7 +632,7 _at__at_
> %      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
> % _at__at_ -108,7 +108,7 _at__at_
> %  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
> % _at__at_ -1056,7 +1056,7 _at__at_
> % %  #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
> _______________________________________________
> freebsd-arch_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe_at_freebsd.org"
> 
> 
Received on Fri Nov 25 2011 - 17:25:27 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:21 UTC