2008-05-06 04:39:03

by NeilBrown

[permalink] [raw]
Subject: [PATCH - nfs-utils] 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(-)

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index 5076468..0036caa 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -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);
@@ -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;
}
}
@@ -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()) {
+ 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;
+ }
+ }
+
mnt_err = try_mount(spec, mount_point, flags, fs_type, &extra_opts,
mount_opts, fake, nomtab, FOREGROUND);
if (mnt_err == EX_BG) {
diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
index 6c0c365..e6ed019 100644
--- a/utils/mount/nfsmount.c
+++ b/utils/mount/nfsmount.c
@@ -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)) {
--
1.5.5.1



2008-05-06 21:37:25

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH - nfs-utils] Make sure statd gets started when 'string options' are in use.

On May 6, 2008, at 12:38 AM, Neil Brown wrote:
> 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(-)
>
> diff --git a/utils/mount/mount.c b/utils/mount/mount.c
> index 5076468..0036caa 100644
> --- a/utils/mount/mount.c
> +++ b/utils/mount/mount.c
> @@ -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);
> @@ -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;
> }
> }
> @@ -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()) {
> + 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.

> mnt_err = try_mount(spec, mount_point, flags, fs_type, &extra_opts,
> mount_opts, fake, nomtab, FOREGROUND);
> if (mnt_err == EX_BG) {
> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> index 6c0c365..e6ed019 100644
> --- a/utils/mount/nfsmount.c
> +++ b/utils/mount/nfsmount.c
> @@ -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)) {
> --
> 1.5.5.1

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2008-05-08 02:48:26

by NeilBrown

[permalink] [raw]
Subject: Re: [PATCH - nfs-utils] Make sure statd gets started when 'string options' are in use.

On Tuesday May 6, [email protected] wrote:
> On May 6, 2008, at 12:38 AM, Neil Brown wrote:
> > 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(-)
> >
> > diff --git a/utils/mount/mount.c b/utils/mount/mount.c
> > index 5076468..0036caa 100644
> > --- a/utils/mount/mount.c
> > +++ b/utils/mount/mount.c
> > @@ -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);
> > @@ -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;
> > }
> > }
> > @@ -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()) {
> > + 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.

>
> 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.

>
> 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

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index cdd610e..be77d17 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -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);

2008-05-08 14:57:15

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH - nfs-utils] Make sure statd gets started when 'string options' are in use.

On May 7, 2008, at 10:48 PM, Neil Brown wrote:
> On Tuesday May 6, [email protected] wrote:
>> On May 6, 2008, at 12:38 AM, Neil Brown wrote:
>>> 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(-)
>>>
>>> diff --git a/utils/mount/mount.c b/utils/mount/mount.c
>>> index 5076468..0036caa 100644
>>> --- a/utils/mount/mount.c
>>> +++ b/utils/mount/mount.c
>>> @@ -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);
>>> @@ -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;
>>> }
>>> }
>>> @@ -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()) {
>>> + 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.
>
>>
>> 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.

/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.

>> 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
>
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index cdd610e..be77d17 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -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);

That's much closer to what I was thinking of. But again, this belongs
in set_mandatory_options() (or whatever we want to call the function).

It may seem like I'm splitting hairs, but the whole point of
nfsmount_string() is to split the incoming mount option string into an
easy-to-parse data structure, then pass the data structure to helpers
to do the actual work. The nfsmount_string() function is meant to be
free of logic that is specific to a file system type or specific to a
particular mount option (even fg/bg should be moved to a helper... I
think I will construct a patch to do that right now).

The point is to keep each of these functions simple; they should all
operate at the same level of detail, and encapsulate just a few
important decisions at that level.

nfsmount_string() is meant to be the "35,000 foot" entry point. Other
functions, like parse_devname(), are meant to handle simple string
manipulation. Combining dissimilar levels of detail is like a
layering violation, and then over time we end up with unmaintainable
logic such as today's nfsmount.c, which attempts to do nearly
everything in a single giant function.

NFS mounting is so subtle and exception filled it is worth taking a
few moments to factor this correctly.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2008-05-09 00:39:15

by NeilBrown

[permalink] [raw]
Subject: Re: [PATCH - nfs-utils] Make sure statd gets started when 'string options' are in use.

On Thursday May 8, [email protected] wrote:
> On May 7, 2008, at 10:48 PM, Neil Brown wrote:
> >
> > 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' ??)

>
> 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".

??

>
> 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

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index cdd610e..2de77a8 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -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;
@@ -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)