From owner-freebsd-fs@FreeBSD.ORG Thu Mar 1 02:08:54 2012 Return-Path: Delivered-To: freebsd-fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 25CD8106564A; Thu, 1 Mar 2012 02:08:54 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id B94D88FC14; Thu, 1 Mar 2012 02:08:53 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q2128nX3022888 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 1 Mar 2012 13:08:51 +1100 Date: Thu, 1 Mar 2012 13:08:49 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: kib@FreeBSD.org In-Reply-To: <201202292037.q1TKbJDI072739@freefall.freebsd.org> Message-ID: <20120301115156.X1922@besplex.bde.org> References: <201202292037.q1TKbJDI072739@freefall.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-fs@FreeBSD.org Subject: Re: kern/165559: [ufs] [patch] ufsmount.h uses the 'export' keyword as a structure member name X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Mar 2012 02:08:54 -0000 On Wed, 29 Feb 2012 kib@FreeBSD.org wrote: > Synopsis: [ufs] [patch] ufsmount.h uses the 'export' keyword as a structure member name > > The supplied patch is obviously wrong, or rather incomplete. > The in-kernel uses of the export member must be corrected. The kernel should change to support applications that abuse kernel headers. libufs uses ufsmount.h, but I consider the existence of libufs to be another bug, and it's not clear whey it uses this header. Even mount(8) no longer uses this header. It used to use it, but now uses nmount(2) so everything is done using string options and mount args structs are never used. I don't like nmount(), but it seems that anything new enough to be tripping over the export keyword should also be new enough to not use old mount args structs. df(1) still uses this header, since it hasn't been converted to use nmount(). But if nmount() is good for anything it is good here. df only uses mount() to run report results for unmounted file systems. But it only understands ffs. Actually, it blindly assumes ffs. It could do better using nmount(), except the details of creating options are even more difficult using nmount(), and it is unreasonable fot df to create options more complicated than "-o ro ", which are not enough in many cases. So it shouldn't do any of this. It could reasonably fork mount(8) to do whatever can be done using /etc/fsatab. Either way, it wouldn't use this header. Here is the entire contents of this header after removing the _KERNEL parts of it: % #include /* XXX For struct workhead. */ This supplies gross namespace pollution. Perhaps parts of userland that are abusing this header actually want this pollution. sys/buf.h is another header that should be kernel-only, but it has some _KERNEL ifdefs in it, and removing the _KERNEL parts in it shows gross namespace pollution that is not under these ifdefs. It begins by includinf , , and and gets whatever pollution these supply. Then it soon declares a kernel variable (extern struct... bioops). Otherwise, it is realtively clean and only defines kernel types and macros. % % /* % * Arguments to mount UFS-based filesystems % */ % struct ufs_args { % char *fspec; /* block special device to mount */ % struct oexport_args export; /* network export information */ % }; There is not much here. libufs of course doesn't use this struct or anything in it. It seems to build perfectly after removing all includes of ufsmount.h in it (5 in .c files). This shows that none of the other ufs headers that libufs includes depend on this header, and that nothing depends on the namespace pollution in this header. libufs also says that applications must include this header in the SYNOPSIS of all 5 of its man pages. This seems to be wrong too. The man pages also says that applications must include 2 other ffs headers. These headers are actually needed, to declare types for libufs's header. Actually, the `export' variable should be fixed because it has style bugs. Struct members should have a fairly unique prefix related to the struct. The style bugs are missing in , so it wouldn't have this bug if it had a struct member near `export'. It actually doesn't have any names near `export', but has struct export args and uses the prefix ex_ for all struct members in it. I only searched for includes of this header in an old version of FreeBSD. Apart from the above, they are in - amd: it still uses mount() AFAIK - sysinstall: too old to use nmount() - mountd: now the use is almost reasonable, but it has XXX comments about this assuming that all mount args structs look like ffs's, and I think they must indeed start like ffs's - mountd in -current: mountd uses nmount() and no longer uses the ufs header, but it does use the corresponding nfs header, which, since it lmust look like ffs's header, has the same bugs (no prefixes in names, and the second member is named `export'). So the scope of this bug includes all mount args structs in the kernel. - dump/main.c: ufs utilities can use ufs headers without abusing them, but like libufs, this file includes this header, apparently without actually using it (the include is grouped with the other 2 as in libufs, except it is only unsorted in ufs. - newfs/newfs.c: same as in libufs, with different unsorting - tunefs/tunefs.c: this actually uses the header, since it needs to remount and hasn't been converted to nmount(). - fsck_ffs/main.c: legit use. Even uses `export', but only to zero it. - ffsinfo/ffsinfo.c: like dump/main.c - mksnap/mksnap_ffs.c: used to use args.fspec a lot. Has been converted to nmount, so no longer uses the header (?), but still includes it. (I only checked about 3/4 of the files in this list for conversion to nmount()). - kernel uses of this header: there are a few outside of ffs. To summarise, even ffs utilities should be using this header. There are 1 or 2 ffs utilities that can reasonably use it, and a few non-ffs utilities that use since they haven't been converted to nmount(). amd is the only significant remaining one. Bruce