bin/116980: [patch] [msdosfs] mount_msdosfs(8) resets some flags for 'update' mount

[Bruce Evans]

The following reply was made to PR bin/116980; it has been noted by GNATS.

From: Bruce Evans <brde at optusnet.com.au>
To: EugeneGrosbein at grosbein.pp.ru
Cc: freebsd-gnats-submit at freebsd.org, jkoshy at freebsd.org, dwmalone at freebsd.org
Subject: Re: bin/116980: [patch] [msdosfs] mount_msdosfs(8) resets some flags
 for 'update' mount
Date: Mon, 15 Oct 2007 15:37:43 +1000 (EST)

 On Sun, 14 Oct 2007 EugeneGrosbein at grosbein.pp.ru wrote:
 
 > Subject: Re: bin/116980: [patch] [msdosfs] mount_msdosfs(8) resets some flags for 'update' mount
 
 > Same patch, suitable for 7.0-PRERELEASE:
 
 You should be using -ocurrent.  Without -ocurrent, all flags not
 explicily mentioned in the options list are supposed to be reset.
 However, -ocurrent has apparently never actually worked except for
 generic flags, since the generic level of mount(1) cannot determine
 the current settings of non-generic flags itself and has no way to
 pass the -current option to the fs-specific mount utilities.  This
 also breaks the display of the current settings when no mount point
 is specified.
 
 This problem affects all file systems, and its fix shouldn't involve
 changing the semantics of mount without -ocurrent, so it cannot be
 fixed as in the submitted patch.
 
 > --- sbin/mount_msdosfs/mount_msdosfs.c.orig	2007-01-29 08:49:08.000000000 +0700
 > +++ sbin/mount_msdosfs/mount_msdosfs.c	2007-10-14 18:37:08.000000000 +0800
 > @@ -69,7 +69,7 @@
 >  	struct iovec *iov = NULL;
 >  	int iovlen = 0;
 >  	struct stat sb;
 > -	int c, mntflags, set_gid, set_uid, set_mask, set_dirmask;
 > +	int c, mntflags, set_gid, set_uid, set_mask, set_dirmask, update = 0;
 >  	char *dev, *dir, mntpath[MAXPATHLEN], *csp;
 >  	char fstype[] = "msdosfs";
 >  	char errmsg[255] = {0};
 
 Style bug: initialization in declaration.  Some nearby lines have the same
 style bug, but style bugs on the changed line were limited to disorder.
 
 > @@ -134,6 +134,7 @@
 >  				*p = '\0';
 >  				val = p + 1;
 >  			}
 > +			update = update || !strcmp(optarg, "update");
 >  			build_iovec(&iov, &iovlen, optarg, val, (size_t)-1);
 >  			}
 >  			break;
 
 This and the following changes might be correct with a test of "current"
 instead of "update".
 
 Style bug: boolean comparison of non-boolean `strcmp()'.  This style bug is
 missing for all other instances of strcmp() in this file.
 
 The (old) logic for resetting to defaults also has problems.  Defaults
 for uids, gids and masks are taken from the current values for the mount
 point.  However, previous overrides of the defaults affect the current
 value, so it is difficult to determine the values to reset to.  This
 causes strange behaviour like the following:
 
  	# mount_msdosfs /dev/foo /foo
  	# # mode is now 555 for /foo and for regular files under /foo, say
  	# mount -u -o -M755,-m644 /foo  # set nicer masks
  	# mount -u /foo  # try to reset
  	# # mode is now 755 for /foo and for regular files under /foo
  	# # The -M setting is apparently preserved, but that is an illusion --
  	# # the setting is the default and the default is wrong (it is
  	# # taken from the current value for the mount point but should be
  	# # taken from the current value for the mounted-on point.
  	# # The -m setting is neither reset nor apparently-preserved --
  	# # again, it is the default and the default is wrong (same as for
  	# # -M since the directory perms are used for regular files unless
  	# # -m is specified.
 
 I think I saw problems caused by this for nfs yesterday.  I was toggling
 -T and async, and thought that mount -u doesn't support toggling -T,
 so I was remounting to toggle -T.  Then toggling async using mount -u
 caused mysterious hangs if I forgot to match the current setting of
 -T in the option list for -u.  I think the hangs are new -- apparently,
 toggling -T used to work but now causes hangs.
 
 Bruce