kern/165559: [ufs] [patch] ufsmount.h uses the 'export' keyword as a structure member name

[Konstantin Belousov]

On Thu, Mar 01, 2012 at 01:08:49PM +1100, Bruce Evans wrote:
> On Wed, 29 Feb 2012 kib at 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.
There is more uses of ufs_args, in particular, in automounter.

> 
> 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.
> 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20120301/69b6b729/attachment.pgp