Make sure statd gets started when 'string options' are in use.

The code for checking and starting statd was only in the binary-options branch of the code.
This moves it into common code.
---

utils/mount/mount.c    |   22 ++++++++++++++++++++--
utils/mount/nfsmount.c |   11 -----------
2 files changed, 20 insertions(+), 13 deletions(-)

utils/mount/mount.c +2 -1

index 5076468..0036caa 100644
@@ -334,7 +334,8 @@ static void parse_opt(const char *opt, int  
 *mask, char *extra_opts, int len)
 * standard options (indicated by MS_ bits), and output parameter
 * "@extra_opts" gets all the filesystem-specific options.
 */
-static void parse_opts(const char *options, int *flags, char  
 **extra_opts)
+static void parse_opts(const char *options, int *flags, char  
 **extra_opts,
+		       int *lock)

{
	if (options != NULL) {
		char *opts = xstrdup(options);

utils/mount/mount.c +4 -0

@@ -358,6 +359,10 @@ static void parse_opts(const char *options, int  
 *flags, char **extra_opts)
 			/* end of option item or last item */
 			if (*p == '\0' || *(p + 1) == '\0') {
 				parse_opt(opt, flags, *extra_opts, len);
+				if (strcmp(opt, "lock") == 0)
+					*lock = 1;
+				if (strcmp(opt, "nolock") == 0)
+					*lock = 0;
 				opt = NULL;
 			}

		}

utils/mount/mount.c +4 -1

@@ -421,6 +426,7 @@ int main(int argc, char *argv[])
 	char *spec, *mount_point, *fs_type = "nfs";
 	char *extra_opts = NULL, *mount_opts = NULL;
 	uid_t uid = getuid();
+	int lock = 1;
 
 	progname = basename(argv[0]);
 

@@ -531,7 +537,7 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
-	parse_opts(mount_opts, &flags, &extra_opts);
+	parse_opts(mount_opts, &flags, &extra_opts, &lock);
 
 	if (uid != 0) {
 		if (!(flags & (MS_USERS|MS_USER))) {

@@ -546,6 +552,18 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
+	if (!fake && lock) {
+		if (!start_statd()) {

utils/mount/mount.c +0 -0

 	mnt_err = try_mount(spec, mount_point, flags, fs_type, &extra_opts,
 				mount_opts, fake, nomtab, FOREGROUND);
 	if (mnt_err == EX_BG) {

utils/mount/nfsmount.c +0 -11

index 6c0c365..e6ed019 100644
@@ -851,17 +851,6 @@ noauth_flavors:
 
 	*extra_opts = xstrdup(new_opts);
 
-	if (!fake && !(data.flags & NFS_MOUNT_NONLM)) {
-		if (!start_statd()) {
-			nfs_error(_("%s: rpc.statd is not running but is "
-				"required for remote locking.\n"
-				"   Either use '-o nolock' to keep "
-				"locks local, or start statd."),
-					progname);
-			goto fail;
-		}
-	}
-
 	if (!fake) {
 		if (mount(spec, node, "nfs",
 				flags & ~(MS_USER|MS_USERS), &data)) {

Signature · 1 line

1.5.5.1

Quoted 59 lines · Neil Brown

The code for checking and starting statd was only in the binary- options
branch of the code.
This moves it into common code.
---

utils/mount/mount.c    |   22 ++++++++++++++++++++--
utils/mount/nfsmount.c |   11 -----------
2 files changed, 20 insertions(+), 13 deletions(-)

utils/mount/mount.c +2 -1

index 5076468..0036caa 100644
@@ -334,7 +334,8 @@ static void parse_opt(const char *opt, int  
 *mask, char *extra_opts, int len)
 * standard options (indicated by MS_ bits), and output parameter
 * "@extra_opts" gets all the filesystem-specific options.
 */
-static void parse_opts(const char *options, int *flags, char  
 **extra_opts)
+static void parse_opts(const char *options, int *flags, char  
 **extra_opts,
+		       int *lock)

{
	if (options != NULL) {
		char *opts = xstrdup(options);

utils/mount/mount.c +4 -0

@@ -358,6 +359,10 @@ static void parse_opts(const char *options, int  
 *flags, char **extra_opts)
 			/* end of option item or last item */
 			if (*p == '\0' || *(p + 1) == '\0') {
 				parse_opt(opt, flags, *extra_opts, len);
+				if (strcmp(opt, "lock") == 0)
+					*lock = 1;
+				if (strcmp(opt, "nolock") == 0)
+					*lock = 0;
 				opt = NULL;
 			}

		}

utils/mount/mount.c +2 -1

@@ -421,6 +426,7 @@ int main(int argc, char *argv[])
 	char *spec, *mount_point, *fs_type = "nfs";
 	char *extra_opts = NULL, *mount_opts = NULL;
 	uid_t uid = getuid();
+	int lock = 1;
 
 	progname = basename(argv[0]);
 

@@ -531,7 +537,7 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
-	parse_opts(mount_opts, &flags, &extra_opts);
+	parse_opts(mount_opts, &flags, &extra_opts, &lock);
 
 	if (uid != 0) {
 		if (!(flags & (MS_USERS|MS_USER))) {

@@ -546,6 +552,18 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 

utils/mount/mount.c int main(int argc, char *argv[]) · line 566

Quoted 12 lines

utils/mount/mount.c +12 -0

+	if (!fake && lock) {
+		if (!start_statd()) {
+			nfs_error(_("%s: rpc.statd is not running but is "
+				"required for remote locking.\n"
+				"   Either use '-o nolock' to keep "
+				"locks local, or start statd."),
+					progname);
+			mnt_err = EX_FAIL;
+			goto out;
+		}
+	}
+

You probably want to gate this based on NFS version (ie do the checking only for NFSv2/v3 mounts, and not for NFSv4 mounts)

There is already a function in stropts.c (called set_mandatory_options) that might be a better place to put the special option parsing, as you can use the nice po_* calls, and it already has a switch for detecting which NFS version is being requested.

Since the common part of this code (start_statd) is already in a helper, I would leave the existing logic in nfsmount.c, and just add a handful of lines in stropts.c:set_mandatory_options that worries about "lock/nolock" for NFSv2/v3 mounts.

Quoted 60 lines · On Tuesday May 6, [email protected] wrote:

Quoted 59 lines · Neil Brown

The code for checking and starting statd was only in the binary- options
branch of the code.
This moves it into common code.
---

utils/mount/mount.c    |   22 ++++++++++++++++++++--
utils/mount/nfsmount.c |   11 -----------
2 files changed, 20 insertions(+), 13 deletions(-)

utils/mount/mount.c +2 -1

index 5076468..0036caa 100644
@@ -334,7 +334,8 @@ static void parse_opt(const char *opt, int
 *mask, char *extra_opts, int len)
 * standard options (indicated by MS_ bits), and output parameter
 * "@extra_opts" gets all the filesystem-specific options.
 */
-static void parse_opts(const char *options, int *flags, char
 **extra_opts)
+static void parse_opts(const char *options, int *flags, char
 **extra_opts,
+		       int *lock)

{
	if (options != NULL) {
		char *opts = xstrdup(options);

utils/mount/mount.c +4 -0

@@ -358,6 +359,10 @@ static void parse_opts(const char *options, int
 *flags, char **extra_opts)
 			/* end of option item or last item */
 			if (*p == '\0' || *(p + 1) == '\0') {
 				parse_opt(opt, flags, *extra_opts, len);
+				if (strcmp(opt, "lock") == 0)
+					*lock = 1;
+				if (strcmp(opt, "nolock") == 0)
+					*lock = 0;
 				opt = NULL;
 			}

		}

utils/mount/mount.c +2 -1

@@ -421,6 +426,7 @@ int main(int argc, char *argv[])
 	char *spec, *mount_point, *fs_type = "nfs";
 	char *extra_opts = NULL, *mount_opts = NULL;
 	uid_t uid = getuid();
+	int lock = 1;
 
 	progname = basename(argv[0]);
 

@@ -531,7 +537,7 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
-	parse_opts(mount_opts, &flags, &extra_opts);
+	parse_opts(mount_opts, &flags, &extra_opts, &lock);
 
 	if (uid != 0) {
 		if (!(flags & (MS_USERS|MS_USER))) {

@@ -546,6 +552,18 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 

Quoted 15 lines

utils/mount/mount.c int main(int argc, char *argv[]) · line 566

Quoted 12 lines

utils/mount/mount.c +12 -0

+	if (!fake && lock) {
+		if (!start_statd()) {
+			nfs_error(_("%s: rpc.statd is not running but is "
+				"required for remote locking.\n"
+				"   Either use '-o nolock' to keep "
+				"locks local, or start statd."),
+					progname);
+			mnt_err = EX_FAIL;
+			goto out;
+		}
+	}
+

You probably want to gate this based on NFS version (ie do the checking only for NFSv2/v3 mounts, and not for NFSv4 mounts)

Yes, of course :-(
I remember think about that but must have forgotten when I actually cut the patch.

Quoted 1 line

Quoted 4 lines

There is already a function in stropts.c (called set_mandatory_options) that might be a better place to put the special option parsing, as you can use the nice po_* calls, and it already has a switch for detecting which NFS version is being requested.

I cannot bring myself to put code in a function called "set_mandatory_options" which does not, in fact, set any options, whether mandatory or not.

Quoted 4 lines

Since the common part of this code (start_statd) is already in a helper, I would leave the existing logic in nfsmount.c, and just add a handful of lines in stropts.c:set_mandatory_options that worries about "lock/nolock" for NFSv2/v3 mounts.

So how about the following?

Thanks for the feedback.

NeilBrown

utils/mount/stropts.c +11 -0

index cdd610e..be77d17 100644
@@ -694,6 +694,17 @@ int nfsmount_string(const char *spec, const  
 char *node, const char *type,
 	if (!set_mandatory_options(type, &saddr, options))
 		goto out;
 
+	if (!fake && strcmp(type, "nfs4") != 0 &&
+	    po_rightmost(options, "nolock", "lock") != PO_KEY1_RIGHTMOST)
+		if (!start_statd()) {
+			nfs_error(_("%s: rpc.statd is not running but is "
+				    "required for remote locking.\n"
+				    "   Either use '-o nolock' to keep "
+				    "locks local, or start statd."),
+				  progname);
+			goto out;
+		}
+
 	if (po_rightmost(options, "bg", "fg") == PO_KEY1_RIGHTMOST)
 		retval = nfsmount_bg(spec, node, type, hostname, flags,

					options, fake, child, extra_opts);

Quoted 4 lines · Neil Brown

I cannot bring myself to put code in a function called "set_mandatory_options" which does not, in fact, set any options, whether mandatory or not.

/me makes the Scooby noise... (or is it the Tim Allen noise?)

Huh? set_mandatory_options sets up both "addr=" and "clientaddr=". Both of these are required by the kernel, thus they are mandatory options. So I don't understand your objection.

That function is the designated place to do specific option processing, and even has a specific check for which type of file system is being mounted. Your fix would be simpler (it would add less redundant logic) if it were added in set_mandatory_options().

It's easy enough to change the name of the function if you think that would help make it more clear.

Quoted 7 lines · On Thursday May 8, [email protected] wrote:

Quoted 4 lines · Neil Brown

I cannot bring myself to put code in a function called "set_mandatory_options" which does not, in fact, set any options, whether mandatory or not.

/me makes the Scooby noise... (or is it the Tim Allen noise?)

Great impersonation!! (or is that an 'imdogonation' ??)

Quoted 3 lines

Huh? set_mandatory_options sets up both "addr=" and "clientaddr=". Both of these are required by the kernel, thus they are mandatory options. So I don't understand your objection.

Ah.. I see how I could be read differently from how I meant it. What if I change it to:

I cannot bring myself to put code which does not set any options into a function called "set_mandatory_options".

??

Quoted 7 lines

That function is the designated place to do specific option processing, and even has a specific check for which type of file system is being mounted. Your fix would be simpler (it would add less redundant logic) if it were added in set_mandatory_options().

It's easy enough to change the name of the function if you think that would help make it more clear.

Good. How about this?

Thanks,
NeilBrown

utils/mount/stropts.c +18 -3

index cdd610e..2de77a8 100644
@@ -218,14 +218,29 @@ static int fix_mounthost_option(struct  
 mount_options *options)
 	return 0;
 }
 
+static int verify_lock_option(struct mount_options *options)
+{
+	if (po_rightmost(options, "nolock", "lock") != PO_KEY1_RIGHTMOST
+	    && !start_statd()) {
+		nfs_error(_("%s: rpc.statd is not running but is "
+			    "required for remote locking.\n"
+			    "   Either use '-o nolock' to keep "
+			    "locks local, or start statd."),
+			  progname);
+		return 0;
+	}
+	return 1;
+}
+
 /*
 * Set up mandatory mount options.
 *
 * Returns 1 if successful; otherwise zero.
 */
-static int set_mandatory_options(const char *type,
-				 struct sockaddr_in *saddr,
-				 struct mount_options *options)
+static int validate_options(const char *type,
+			    struct sockaddr_in *saddr,
+			    struct mount_options *options,
+			    int fake)
 {
 	if (!append_addr_option(saddr, options))

		return 0;

utils/mount/stropts.c +3 -1

@@ -236,6 +251,8 @@ static int set_mandatory_options(const char *type,
 	} else {
 		if (!fix_mounthost_option(options))
 			return 0;
+		if (!fake && verify_lock_option(options))
+			return 0;
 	}
 
 	return 1;

@@ -691,7 +708,7 @@ int nfsmount_string(const char *spec, const char  
 *node, const char *type,
 		goto fail;
 	}
 
-	if (!set_mandatory_options(type, &saddr, options))
+	if (!validate_options(type, &saddr, options, fake))
 		goto out;
 

	if (po_rightmost(options, "bg", "fg") == PO_KEY1_RIGHTMOST)