Created attachment 205074 [details] msdosfs_vfsops.patch Currently msdosfs driver doesn't handle duplicate nmount() flags correctly. For example, "shortname" flag doesn't work, while "shortnames" works. The same is for "longname"/"longnames" pair. Proposed patch should fix this.
The patch looks coorect, it has small style(9) issues.
Created attachment 205112 [details] Patch with corrected style Style (9) issues should be corrected in this version.
From a bde@ mail: Since "shortname" and "longname" are undocumented aliases for compatibility and which never actually worked, the correct fix is to remove them. This is just another set of bugs in nmount(). The initial conversion to of msdosfs to nmount() in r138471 on 2004-12-06 misspelled these options (without an "s"), so broke the documented options. Then r152595 on 2005-11-28 restored the correct names and kept the misspelled options for compatibility, but this was apparently not tested since the misspelled options apparently stopped working. r152595 doesn't work due to nmount()'s general mishandling of tri-state logic (default (absent), negative and positive options), and the fix in the PR is wrong too since it gives the misspelled options precedence... In -current, vfs_flagopt() on "shortname" sets MSDOSFSMNT_SHORTNAME if "shortname" is an option and clears the flag otherwise. This is already broken for the negative options "noshortname". Then vfs_flagopt() on "shortnames" sets MSDOSFSMNT_SHORTNAME if "shortnames" is an option, and clears the flag otherwise. So if only "shortname" is an option, then the flag is always clobbered by checking for "shortnames". "shortnames" works accidentally since it is checked last. Negative options for both are broken. In the patch, "shortnames" is not checked for if "shortname" is an option, so negative options that cancel previous positive options are even further from working. E.g., -o shortname,noshortnames is further from working than -o shortname,noshortname. nmount() has some support for negative options, but this is implemented very badly, and is not used for vfs_flagopt() (this does a simple strcmp()). So even standard negative options like "noro" barely work, and for fs- specific ones evey file system has the burden of checking for "no" prefixes. msdosfs is just missing the checks for "noshortname[s]", so these negative option just don't work. Actual testing shows: - FreeBSD-5.2 is not broken by using nmount() or its spelling errors or compatibility cruft for msdosfs, so: - -o shortnames works - -o shortname fails with the error message "msdosfs: -o shortname: option not supported". Everything is handled in userland and this option is just not supported - -o shortnames,noshortnames works (the negative option is supported and is correctly parsed) in userland and correctly cancels the flag in userland. The userland data for this is 3 bytes long (", 0"): { "shortnames", 0, MSDOSFSMNT_SHORTNAME, 1 }, { "longnames", 0, MSDOSFSMNT_LONGNAME, 1 }, { "nowin95", 0, MSDOSFSMNT_NOWIN95, 1 }, The complications for negative logic are in getmntopts(3). This is easy to use, and actually works, unlike nmount() code in the kernel which is many times larger. - double negative ("nono") prefixes are not supported. - -current with mount_msdosfs(8) from FreeBSD-~5.2 and FreeBSD-6 shows various bugs: (I also have mount(8) from FreeBSD-10 which is supposed to be be used for -current; it should exec mount_msdosfs from FreeBSD-6 via a script, but I now see that my scripts are broken for that use: - at boot time, mount(8) is /sbin/mount from ~5.2 and this mostly works - later, mount(8) is a script from $HOME/bin. It uses uname to select a mount(8) than works in more cases - for -current, the script selects mount(8) from FreeBSD-10 - similarly for mount_msdosfs(8), except the script selects mount_msdosfs(8) from FreeBSD-6 - mount(8) uses a sanitizied path (not $PATH which includes $HOME/bin), so it execs /sbin/mount_msdosfs instead of the script and thus finds mount_msdosfs(8) from ~5.2 - there are similar problems finding mount_nfs(8), but these are more noticeable so my scripts already handling them, starting by putting the scripts in /sbin which I try not to change. Now the bugs: - ~5.2 mount_msdosfs uses the parsing utility functions from src/sbin/mount, so option strings -o foo,bar,... work. This is broken in at least FreeBSD-6 mount_msdosfs -- a separate -o flag must be used for each option. The man page still says that the syntax is [-o options] as described in mount(8). - with FreeBSD-6 mount_msdosfs: - o shortname fails (silently has no affect, as described above) - o shortnames works (as described above) - o shortnames,noshortnames fails (is a syntax error, as described above) - o shortnames -o noshortnames fails (noshortnames is silently ignored, as described above) "nowin95" is differently broken. This is a negative option, but is in msdosfs_opts[] as both "win95" and "nowin95". The buggy negative logic apparently allows "noshortnames" to not cause an error although it is mishandled. The same should work for "nowin95", but it is supposed to be a negative option so it should be spelled with "no" in the options table, but then "win95" might be disallowed. It is handled quite differently from "shortnames", using vfs_getopt() instead of vfs_flagopt() and then laborious setting and clearing of a flag. Since the flag has the same negative logic, I don't see why vfs_flagopt() can't handle it equally badly. There may be the common problem that we want the absense of an option to have no effect instead of clearing a flag. In practice, "nowin95" is very broken, but not as feared above: - in ~5.2. The table entry { "nowin95", 0, MSDOSFSMNT_NOWIN95, 1 }, gives a positive option "nowin95", but should give a negative option "win95", and its altflag seems to be garbage too. The correct entry is: { "win95", 1, MSDOSFSMNT_NOWIN95, 1 /* XXX why not 0? */ }, This bug obviously makes "win95" an invalid option. "nowin95" is invalid too, apparently because the options parsing removes leading "no" from "nowin95" and then looks up "win95" in the table and fails to find it. The only option that works is "nonowin95". This allows only limited testing of mount(2) in -current: mount ... -o shortnames,nonowin95 works correctly -- shortnames kills the default of longnames but then nonowin95 restores the default. - with FreeBSD-6 mount_nfs, -o win95,nowin95 is a syntax error, but -o win95 -o nowin95 works correctly and so does -o nowin95 -o win95 and -o nowin95 -o nonowin95. nmount()'s mishandling of negative options is now clearer. For mount(), getmntinfo() has no need for double negatives like "nonoro", and these are only allowed by buggy table entries like the one for "nowin95". Normal entries never start with "no". Option starting with "nono" are converted to negative options starting with "no", but these never match normal entries so are disallowed. nmount() mostly does the opposite. E.g., ffs's tables have "noatime" but not "atime", because nmount() is missing a negative logic flag so has to encode the flag in the name. The "nowin95" table entry is what allows the double negative "nonowin95" to work, while "nonoshortnames" is disallowed. I don't know if the "win95" table entry is needed to allow "win95" to work. ffs doesn't need an "atime" entry to allow "atime" to work -- "atime" is derived from "noatime". But standard options like "noatime" are special -- vfs_mount.c has mounds of special cases to handle them. Individual file systems might need similar mounds of special cases for nonstandard options. Bruce