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. BruceReceived on Fri Nov 25 2011 - 04:55:01 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:21 UTC