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