From owner-freebsd-current@FreeBSD.ORG Sun Nov 27 03:16:31 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6AA9D106566B; Sun, 27 Nov 2011 03:16:31 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id E2BF48FC0C; Sun, 27 Nov 2011 03:16:30 +0000 (UTC) Received: from c211-28-227-231.carlnfd1.nsw.optusnet.com.au (c211-28-227-231.carlnfd1.nsw.optusnet.com.au [211.28.227.231]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id pAR3GKwE021619 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 27 Nov 2011 14:16:21 +1100 Date: Sun, 27 Nov 2011 14:16:20 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Robert Millan In-Reply-To: <20111126200749.GA72056@thorin> Message-ID: <20111127131422.D802@besplex.bde.org> References: <20111123070036.GA29952@thorin> <20111124141821.O932@besplex.bde.org> <20111125150324.G1018@besplex.bde.org> <20111126200749.GA72056@thorin> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Mailman-Approved-At: Sun, 27 Nov 2011 04:26:06 +0000 Cc: Adrian Chadd , freebsd-current@freebsd.org, Bruce Evans , freebsd-arch@freebsd.org, Kostik Belousov , Warner Losh Subject: Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 27 Nov 2011 03:16:31 -0000 On Sat, 26 Nov 2011, Robert Millan wrote: > On Fri, Nov 25, 2011 at 11:16:15AM -0700, Warner Losh wrote: >> 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. > > Hi, > > I'm sending a new patch. Thanks Bruce for your input. TTBOMK this corrects > all the problems you spotted that were introduced by my patch. It doesn't > fix pre-existing problems in the files however, except in cases where I had > to modify that line anyway. > > I think it's a good compromise between my initial patch and an exhaustive > cleanup of those headers (which I'm probably not the most indicate for). It fixes most style bugs, but not some-pre-existing problems, even in cases where you had to modify the line anyway. % Index: sys/cam/scsi/scsi_low.h % =================================================================== % --- sys/cam/scsi/scsi_low.h (revision 227956) % +++ sys/cam/scsi/scsi_low.h (working copy) % @@ -53,10 +53,10 @@ % #define SCSI_LOW_INTERFACE_XS % #endif /* __NetBSD__ */ % % -#ifdef __FreeBSD__ % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) % #define SCSI_LOW_INTERFACE_CAM % #define CAM % -#endif /* __FreeBSD__ */ % +#endif /* __FreeBSD__ || __FreeBSD_kernel__ */ It still has the whitespace-after tab style change for cam. % Index: sys/dev/firewire/firewirereg.h % =================================================================== % --- sys/dev/firewire/firewirereg.h (revision 227956) % +++ sys/dev/firewire/firewirereg.h (working copy) % @@ -75,7 +75,8 @@ % }; % % 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; Here is a pre-existing problem that you didn't fix on a line that you changed. The __FreeBSD__ ifdef is nonsense here, since __FreeBSD__ being defined has nothing to do with either whether __FreeBSD_version is defined or whether there is a struct cdev * in the data structure. Previously: - defined(__FreeBSD__) means that the compiler is for FreeBSD - __FreeBSD_version >= 500000 means that FreeBSD has been included and has defined __FreeBSD_version to a value that satisifes this. It would be a bug for anything else to define __FreeBSD_version. Unfortunately, there is a bogus #undef of __FreeBSD_version that breaks detection of other things defining it. - the __FreeBSD__ part of the test has no effect except to break compiling this file with a non-gcc compiler. In particular, it doesn't prevent errors for -Wundef -Werror. But other ifdefs in this file use an unguarded __FreeBSD_version. Thus this file never worked with -Wundef -Werror, and the __FreeBSD__ part has no effect except the breakage. Now: as above, except: - defined(__FreeBSD_kernel__) means that FreeBSD been included and that this header is new enough to define __FreeBSD_kernel__. This has the same bug with the #undef, which I pointed out before (I noticed it for this but not for __FreeBSD_version). And it has a style bug in its name which I pointed out before -- 2 underscores in its name. __FreeBSD_version doesn't have this style bug. The definition of __FreeBSD_kernel__ has already been committed. Is it too late to fix its name? - when is new enough to define __FreeBSD_kernel__, it must be new enough to define __FreeBSD_version >= 500000. Thus there is now no -Wundef error. - the __FreeBSD__ ifdef remains nonsense. If you just removed it, then you wouldn't need the __FreeBSD_kernel__ ifdef (modulo the -Wundef error). You didn't add the __FreeBSD_kernel__ ifdef to any of the other lines with the __FreeBSD_kernel__ ifdef in this file, apparently because the others don't have the nonsensical __FreeBSD__ ifdef. The nonsense and changes to work around it make the logic for this ifdef even more convoluted and broken than might first appear. In a previous patchset, you included to ensure that __FreeBSD_kernel__ is defined for newer kernel sources (instead of testing if it is defined). Ifdefs like the above make a prerequsite for this file anyway, since without knowing __FreeBSD_version it is impossible to determine if the data structure has new fields like the cdev in it. is a prerequisite for almost all kernel .c files, so this prerequisite should be satisfied automatically for them, but it isn't clear what happens for user .c files. I think the ifdef should be something like the following to enforce the prerequisite: #ifndef _SYS_PARAM_H_ /* * Here I don't support __FreeBSD_version__ to be set outside of * to hack around a missing include of . * The case where the kernel is so old that __FreeBSD_version__ is * not defined should be handled by including to see * if __FreeBSD_version__ is in fact not defined. */ #error " is a prerequisite for this file" #endif #if __FreeBSD_version >= 500000 /* * Add defined(__FreeBSD_version) to the ifdef if you want to fully * support -Wundef. This is unlikely to have any effect here, since * has been included, and it defines __FreeBSD_version * except under very old versions of FreeBSD where -Wundef was even * more unusable than now. */ struct cdev *dev; #endif Similarly in most places that test __FreeBSD__ and __FreeBSD_version, or __FreeBSD__ and DEVICE_POLLING (the latter might need to ensure that opt_device_polling.h instead of is included, so in userland it reduces to an unconditional #error or hacks since opt_device_polling.h is a kernel-only non-module-only header). Bruce