Date: Thu, 1 Mar 2012 10:48:01 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs@freebsd.org Subject: Re: kern/165559: [ufs] [patch] ufsmount.h uses the 'export' keyword as a structure member name Message-ID: <20120301084801.GP55074@deviant.kiev.zoral.com.ua> In-Reply-To: <20120301115156.X1922@besplex.bde.org> References: <201202292037.q1TKbJDI072739@freefall.freebsd.org> <20120301115156.X1922@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <device> <mountpoint>",
> 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 <sys/buf.h> /* 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 <sys/bufobj.h>, <sys/quque.h>, <sys/lock.h> and
> <sys/lockobj.h> 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 <sys/mount.h>, 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120301084801.GP55074>