From owner-freebsd-fs@FreeBSD.ORG Thu Mar 1 08:49:02 2012 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 835191065670 for ; Thu, 1 Mar 2012 08:49:02 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id BA6888FC12 for ; Thu, 1 Mar 2012 08:49:01 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q218m1SO032188; Thu, 1 Mar 2012 10:48:01 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q218m1ug013137; Thu, 1 Mar 2012 10:48:01 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q218m1Iu013136; Thu, 1 Mar 2012 10:48:01 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 1 Mar 2012 10:48:01 +0200 From: Konstantin Belousov To: Bruce Evans Message-ID: <20120301084801.GP55074@deviant.kiev.zoral.com.ua> References: <201202292037.q1TKbJDI072739@freefall.freebsd.org> <20120301115156.X1922@besplex.bde.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ic1EQBzFNokAMJj2" Content-Disposition: inline In-Reply-To: <20120301115156.X1922@besplex.bde.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua 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 08:49:02 -0000 --ic1EQBzFNokAMJj2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 01, 2012 at 01:08:49PM +1100, Bruce Evans wrote: > On Wed, 29 Feb 2012 kib@FreeBSD.org wrote: >=20 > >Synopsis: [ufs] [patch] ufsmount.h uses the 'export' keyword as a=20 > >structure member name > > > >The supplied patch is obviously wrong, or rather incomplete. > >The in-kernel uses of the export member must be corrected. >=20 > The kernel should change to support applications that abuse kernel > headers. >=20 > 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. >=20 > 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. >=20 > 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. There is more uses of ufs_args, in particular, in automounter. >=20 > Here is the entire contents of this header after removing the _KERNEL > parts of it: >=20 > % #include /* XXX For struct workhead. */ >=20 > 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. >=20 > %=20 > % /* > % * Arguments to mount UFS-based filesystems > % */ > % struct ufs_args { > % char *fspec; /* block special device to mount */ > % struct oexport_args export; /* network export information */ > % }; >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 Yes. I expect the bug submitter to finish the work and provide a complete patch for renaming. FWIW, the export_ is ugly, some reasonable name should be used. > Bruce --ic1EQBzFNokAMJj2 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAk9PN8EACgkQC3+MBN1Mb4hEEACfV1Ae1mHTp66A13+3E8bLNr7+ NxoAoJf9DO7egn6mW3Oo1ygeSGTi/2MU =J90p -----END PGP SIGNATURE----- --ic1EQBzFNokAMJj2--