Date: Thu, 1 Mar 2012 13:08:49 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: kib@FreeBSD.org Cc: freebsd-fs@FreeBSD.org Subject: Re: kern/165559: [ufs] [patch] ufsmount.h uses the 'export' keyword as a structure member name Message-ID: <20120301115156.X1922@besplex.bde.org> In-Reply-To: <201202292037.q1TKbJDI072739@freefall.freebsd.org> References: <201202292037.q1TKbJDI072739@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <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.
Here is the entire contents of this header after removing the _KERNEL
parts of it:
% #include <sys/buf.h> /* 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 <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.
%
% /*
% * 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 <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.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120301115156.X1922>