[PATCH 03/17] mount.nfs: Remove support for "-t" option

[PATCH 03/17] mount.nfs: Remove support for "-t" option

[Chuck Lever]

/bin/mount will never pass "-t" to a mount helper, since it passes the
fs-type in the name of the program it is executing.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/mount/mount.c |   64 ++++++++++++++++++++++-----------------------------
 1 files changed, 27 insertions(+), 37 deletions(-)

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index 5a7bd1c..337d7cc 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -57,7 +57,6 @@ static struct option longopts[] = {
   { "read-write", 0, 0, 'w' },
   { "rw", 0, 0, 'w' },
   { "options", 1, 0, 'o' },
-  { "nfsvers", 1, 0, 't' },
   { "bind", 0, 0, 128 },
   { "replace", 0, 0, 129 },
   { "after", 0, 0, 130 },
@@ -217,17 +216,18 @@ int do_mount_syscall(char *spec, char *node, char *type, int flags, void *data)
 
 void mount_usage()
 {
-	printf("usage: %s remotetarget dir [-rvVwfnh] [-t version] [-o nfsoptions]\n", progname);
-	printf("options:\n\t-r\t\tMount file system readonly\n");
+	printf("usage: %s remotetarget dir [-rvVwfnh] [-o nfsoptions]\n",
+		progname);
+	printf("options:\n");
+	printf("\t-r\t\tMount file system readonly\n");
 	printf("\t-v\t\tVerbose\n");
 	printf("\t-V\t\tPrint version\n");
 	printf("\t-w\t\tMount file system read-write\n");
-	printf("\t-f\t\tFake mount, don't actually mount\n");
+	printf("\t-f\t\tFake mount, do not actually mount\n");
 	printf("\t-n\t\tDo not update /etc/mtab\n");
 	printf("\t-s\t\tTolerate sloppy mount options rather than failing.\n");
 	printf("\t-h\t\tPrint this help\n");
-	printf("\tversion\t\tnfs4 - NFS version 4, nfs - older NFS version supported\n");
-	printf("\tnfsoptions\tRefer mount.nfs(8) or nfs(5)\n\n");
+	printf("\tnfsoptions\tRefer to mount.nfs(8) or nfs(5)\n\n");
 }
 
 static inline void
@@ -381,9 +381,9 @@ static int start_statd()
 
 int main(int argc, char *argv[])
 {
-	int c, flags = 0, nfs_mount_vers = 0, mnt_err = 1, fake = 0;
-	char *spec, *mount_point, *extra_opts = NULL;
-	char *mount_opts = NULL;
+	int c, flags = 0, mnt_err = 1, fake = 0;
+	char *spec, *mount_point, *fs_type = "nfs";
+	char *extra_opts = NULL, *mount_opts = NULL;
 	uid_t uid = getuid();
 
 	progname = basename(argv[0]);
@@ -413,23 +413,12 @@ int main(int argc, char *argv[])
 	mount_point = argv[2];
 
 	argv[2] = argv[0]; /* so that getopt error messages are correct */
-	while ((c = getopt_long (argc - 2, argv + 2, "rt:vVwfno:hs",
+	while ((c = getopt_long (argc - 2, argv + 2, "r:vVwfno:hs",
 				longopts, NULL)) != -1) {
 		switch (c) {
 		case 'r':
 			flags |= MS_RDONLY;
 			break;
-		case 't':
-			if (strcmp(optarg, "nfs4") == 0)
-				nfs_mount_vers = 4;
-			else if (strcmp(optarg, "nfs") == 0)
-				nfs_mount_vers = 0;
-			else {
-				fprintf(stderr, "%s: unknown filesystem type: %s\n",
-					progname, optarg);
-				exit(1);
-			}
-			break;
 		case 'v':
 			++verbose;
 			break;
@@ -488,26 +477,29 @@ int main(int argc, char *argv[])
 	}
 
 	if (strcmp(progname, "mount.nfs4") == 0)
-		nfs_mount_vers = 4;
+		fs_type = "nfs4";
 
+	/*
+	 * If a non-root user is attempting to mount, make sure the
+	 * user's requested options match the options specified in
+	 * /etc/fstab; otherwise, don't allow the mount.
+	 */
 	if (uid != 0) {
-		/* don't even think about it unless options exactly
-		 * match fstab
-		 */
 		struct mntentchn *mc;
 
 		if ((mc = getfsfile(mount_point)) == NULL ||
-		    strcmp(mc->m.mnt_fsname, spec) != 0 ||
-		    strcmp(mc->m.mnt_type, (nfs_mount_vers == 4 ? "nfs4":"nfs")) != 0
-		    ) {
-			fprintf(stderr, "%s: permission died - no match for fstab\n",
-				progname);
+		    !strcmp(mc->m.mnt_fsname, spec) ||
+		    !strcmp(mc->m.mnt_type, fs_type)) {
+			fprintf(stderr, "%s: permission denied: no match for %s "
+				"found in /etc/fstab\n", progname, mount_point);
 			exit(1);
 		}
-		/* 'mount' munges the options from fstab before passing them
+
+		/*
+		 * 'mount' munges the options from fstab before passing them
 		 * to us, so it is non-trivial to test that we have the correct
 		 * set of options and we don't want to trust what the user
-		 * gave us, so just take whatever is in fstab
+		 * gave us, so just take whatever is in /etc/fstab.
 		 */
 		mount_opts = strdup(mc->m.mnt_opts);
 		mounttype = 0;
@@ -533,7 +525,7 @@ int main(int argc, char *argv[])
 	if (chk_mountpoint(mount_point))
 		exit(EX_FAIL);
 
-	if (nfs_mount_vers == 4)
+	if (!strcmp(fs_type, "nfs4"))
 		mnt_err = nfs4mount(spec, mount_point, &flags, &extra_opts, &mount_opts, 0);
 	else {
 		int need_statd = 0;
@@ -557,8 +549,7 @@ int main(int argc, char *argv[])
 		exit(EX_FAIL);
 
 	if (!fake) {
-		mnt_err = do_mount_syscall(spec, mount_point,
-					   nfs_mount_vers == 4 ? "nfs4" : "nfs",
+		mnt_err = do_mount_syscall(spec, mount_point, fs_type,
 					   flags & ~(MS_USER|MS_USERS) ,
 					   mount_opts);
 
@@ -569,8 +560,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (!nomtab)
-		add_mtab(spec, mount_point,
-			 nfs_mount_vers == 4 ? "nfs4" : "nfs",
+		add_mtab(spec, mount_point, fs_type,
 			 flags, extra_opts, 0, 0);
 
 	return 0;


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH 03/17] mount.nfs: Remove support for "-t" option

[Neil Brown]

On Sunday July 15, chuck.lever@oracle.com wrote:
> /bin/mount will never pass "-t" to a mount helper, since it passes the
> fs-type in the name of the program it is executing.

Does it hurt to leave it there though?  Maybe it makes unit-testing
easier?
Or maybe not....

> @@ -413,23 +413,12 @@ int main(int argc, char *argv[])
>  	mount_point = argv[2];
>  
>  	argv[2] = argv[0]; /* so that getopt error messages are correct */
> -	while ((c = getopt_long (argc - 2, argv + 2, "rt:vVwfno:hs",
> +	while ((c = getopt_long (argc - 2, argv + 2, "r:vVwfno:hs",
                                                      ^^

Ooops.  -r takes an argument now?

>  		if ((mc = getfsfile(mount_point)) == NULL ||
> -		    strcmp(mc->m.mnt_fsname, spec) != 0 ||
> -		    strcmp(mc->m.mnt_type, (nfs_mount_vers == 4 ? "nfs4":"nfs")) != 0
> -		    ) {
> -			fprintf(stderr, "%s: permission died - no match for fstab\n",
> -				progname);
> +		    !strcmp(mc->m.mnt_fsname, spec) ||
                   ^^^^

I absolutely LOATHE that construct. The first thing you read is "!"
which means "not" but it is actually a test for "are these things
equal" which makes it confusing (to me).  So I always like to see the
result of strcmp tested with "== 0"  "< 0"  "> 0"  as the relation
given applies exactly to the tested relation between the strings.

Further:
> -		    strcmp(mc->m.mnt_fsname, spec) != 0 ||
and
> +		    !strcmp(mc->m.mnt_fsname, spec) ||

are opposite tests, and it is not clear to me why you inverted the
test.

> @@ -533,7 +525,7 @@ int main(int argc, char *argv[])
>  	if (chk_mountpoint(mount_point))
>  		exit(EX_FAIL);
>  
> -	if (nfs_mount_vers == 4)
> +	if (!strcmp(fs_type, "nfs4"))
            ^
There it is again... HATE HATE HATE HATE .....
It makes me think 'If the fs_type is not "nfs4"' which is not what is
going on at all.

But otherwise there are some fairly useful clean-ups in there.

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH 03/17] mount.nfs: Remove support for "-t" option

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]

Neil Brown wrote:
> On Sunday July 15, chuck.lever@oracle.com wrote:
>> /bin/mount will never pass "-t" to a mount helper, since it passes the
>> fs-type in the name of the program it is executing.
> 
> Does it hurt to leave it there though?  Maybe it makes unit-testing
> easier?
> Or maybe not....

Well, part of these changes remove stuff that just seems confusing to 
anyone trying to understand the code, and yes, this one is quite 
optional.  Honestly, I couldn't think of a case where this could 
possibly be useful, but I'm open to education.

The fact that we can do a "mount.nfs -t nfs4" and that's the same as 
"mount.nfs4" seems a little weird.  The mount command will parse the 
"-t" option and invoke either "mount.nfs" or "mount.nfs4"... extra logic 
here is just going to cloud things.

>> @@ -413,23 +413,12 @@ int main(int argc, char *argv[])
>>  	mount_point = argv[2];
>>  
>>  	argv[2] = argv[0]; /* so that getopt error messages are correct */
>> -	while ((c = getopt_long (argc - 2, argv + 2, "rt:vVwfno:hs",
>> +	while ((c = getopt_long (argc - 2, argv + 2, "r:vVwfno:hs",
>                                                       ^^
> 
> Ooops.  -r takes an argument now?

Hmmm.  I'll fix that too.

>>  		if ((mc = getfsfile(mount_point)) == NULL ||
>> -		    strcmp(mc->m.mnt_fsname, spec) != 0 ||
>> -		    strcmp(mc->m.mnt_type, (nfs_mount_vers == 4 ? "nfs4":"nfs")) != 0
>> -		    ) {
>> -			fprintf(stderr, "%s: permission died - no match for fstab\n",
>> -				progname);
>> +		    !strcmp(mc->m.mnt_fsname, spec) ||
>                    ^^^^
> 
> I absolutely LOATHE that construct. The first thing you read is "!"
> which means "not" but it is actually a test for "are these things
> equal" which makes it confusing (to me).  So I always like to see the
> result of strcmp tested with "== 0"  "< 0"  "> 0"  as the relation
> given applies exactly to the tested relation between the strings.

OK, now that I know that's the preference, I will use "== 0".

> Further:
>> -		    strcmp(mc->m.mnt_fsname, spec) != 0 ||
> and
>> +		    !strcmp(mc->m.mnt_fsname, spec) ||
> 
> are opposite tests, and it is not clear to me why you inverted the
> test.

Simple explanation: because I'm a bone-head.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 290 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs