[nfs-utils PATCH 0/4] Improve nfsmount.conf configuration parsing.

[nfs-utils PATCH 0/4] Improve nfsmount.conf configuration parsing.

[Scott Mayhew]

The parsing of the nfsmount.conf configuration file has various issues.

Here's an example config file I've been testing with:

# cat /etc/nfsmount.conf
[ NFSMount_Global_Options ]
	Acl=False
	Ac=False
	Timeo=600
	Background=True

[ Server "nfs.smayhew.test" ]
	Sec=krb5p
	Nfsvers=4
	Acl=True
	Ac=True
	Timeo=300
	Sloppy=True
	Foreground=True

[ Server "rhel7alpha2.smayhew.test" ]
	Sec=krb5i
	Nfsvers=4

[ Server "rhel6u2.smayhew.test" ]
	Nfsvers=4	

[ Server "rhel5u8.smayhew.test" ]
	Nfsvers=3

[ Server "rhel4u8.smayhew.test" ]
	Nfsvers=3

[ MountPoint "/mnt/nfs" ]
	Timeo=150
	Background=False
	Ac=False
	Sloppy=False

I then mount a filesystem using:

# mount nfs.smayhew.test:/export /mnt/nfs
  
# mount | grep smayhew
nfs.smayhew.test:/export on /mnt/nfs type nfs
(rw,timeo=600,bg,sec=krb5p,nfsvers=4,acl,ac,timeo=300,sloppy,fg,sec=krb5p,nfsvers=4,nfsvers=4,nfsvers=4,nfsvers=4,addr=192.168.122.179,clientaddr=192.168.122.202)
# grep smayhew /proc/mounts
nfs.smayhew.test:/export/ /mnt/nfs nfs4
rw,relatime,vers=4,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,port=0,timeo=300,retrans=2,sec=krb5p,clientaddr=192.168.122.202,minorversion=0,local_lock=none,addr=192.168.122.179
0 0

In the output of mount (no arguments) as well as /etc/mtab,
1. nfsvers=4 is shown four times
2. sec=krb5p is shown twice
3. both fg and bg is shown
4. timeo appears twice

In /proc/mounts we can see that we have the wrong value for timeo (it should be
150).

In newer distros where /etc/mtab is a symlink to /proc/mounts, you're unlikely
to see most of these issues unless you're watching the mount command with
something like systemtap, e.g.

# stap -e 'probe syscall.mount { printf("%s %s\n", probefunc(),
# kernel_string($data)) }'
sys_mount
timeo=600,bg,sec=krb5p,nfsvers=4,acl,ac,timeo=300,sloppy,fg,sec=krb5p,nfsvers=4,nfsvers=4,nfsvers=4,nfsvers=4,addr=192.168.122.179,clientaddr=192.168.122.202

Regardless, rather than sending a slew of mount options and letting the kernel
sort it out, we can do a better job of parsing them on the front end.


Scott Mayhew (4):
  mount.nfs: avoid unnecessary duplication of options passed to
    mount(2)
  mount.nfs: avoid sending conflicting mount options
  mount.nfs:  improve handling of MNT_NOARG type options
  mount.nfs:  clean up conf_parse_mntopts()

 support/include/conffile.h |  2 +-
 support/nfs/conffile.c     |  4 ++-
 utils/mount/configfile.c   | 77 +++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 73 insertions(+), 10 deletions(-)

-- 
1.7.11.7

[nfs-utils PATCH 1/4] mount.nfs: avoid unnecessary duplication of options passed to mount(2)

[Scott Mayhew]

The nfsmount.conf file has the following format:

[ section "arg" ]
	tag = value

conf_get_tag_list() currently doesn't check the arg field so we wind up
getting all the options that fall under a particular section value,
instead of just the ones that match the specific "arg" field.  As a
result, we wind up passing options to the mount syscall multiple times.

For example, if we have three different server sections, and each
section has an Nfsvers tag, then the string we pass to the mount syscall
will have three occurrences of the nfsvers option.

Note that the call to conf_get_section() in conf_parse_mntopts() will
ensure that we never pass the wrong value for an option, but we should
still avoid sending duplicate options if it's at all possible.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 support/include/conffile.h | 2 +-
 support/nfs/conffile.c     | 4 +++-
 utils/mount/configfile.c   | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/support/include/conffile.h b/support/include/conffile.h
index ce7aa21..05ea5d2 100644
--- a/support/include/conffile.h
+++ b/support/include/conffile.h
@@ -54,7 +54,7 @@ extern int      conf_end(int, int);
 extern void     conf_free_list(struct conf_list *);
 extern struct sockaddr *conf_get_address(char *, char *);
 extern struct conf_list *conf_get_list(char *, char *);
-extern struct conf_list *conf_get_tag_list(char *);
+extern struct conf_list *conf_get_tag_list(char *, char *);
 extern int      conf_get_num(char *, char *, int);
 extern char    *conf_get_str(char *, char *);
 extern char    *conf_get_section(char *, char *, char *);
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 5015e94..c3434d5 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -565,7 +565,7 @@ cleanup:
 }
 
 struct conf_list *
-conf_get_tag_list(char *section)
+conf_get_tag_list(char *section, char *arg)
 {
 	struct conf_list *list = 0;
 	struct conf_list_node *node;
@@ -579,6 +579,8 @@ conf_get_tag_list(char *section)
 	cb = LIST_FIRST(&conf_bindings[conf_hash (section)]);
 	for (; cb; cb = LIST_NEXT(cb, link)) {
 		if (strcasecmp (section, cb->section) == 0) {
+			if (arg != NULL && strcasecmp(arg, cb->arg) != 0)
+				continue;
 			list->cnt++;
 			node = calloc(1, sizeof *node);
 			if (!node)
diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
index 6f2ee75..1f1b6e7 100644
--- a/utils/mount/configfile.c
+++ b/utils/mount/configfile.c
@@ -286,7 +286,7 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
 	char *nvalue, *ptr;
 	int argtype;
 
-	list = conf_get_tag_list(section);
+	list = conf_get_tag_list(section, arg);
 	TAILQ_FOREACH(node, &list->fields, link) {
 		/*
 		 * Do not overwrite options if already exists 
-- 
1.7.11.7

Re: [nfs-utils PATCH 1/4] mount.nfs: avoid unnecessary duplication of options passed to mount(2)

[Chuck Lever]

On Aug 13, 2013, at 3:20 PM, Scott Mayhew <smayhew@redhat.com> wrote:

> The nfsmount.conf file has the following format:
> 
> [ section "arg" ]
> 	tag = value
> 
> conf_get_tag_list() currently doesn't check the arg field so we wind up
> getting all the options that fall under a particular section value,
> instead of just the ones that match the specific "arg" field.  As a
> result, we wind up passing options to the mount syscall multiple times.
> 
> For example, if we have three different server sections, and each
> section has an Nfsvers tag, then the string we pass to the mount syscall
> will have three occurrences of the nfsvers option.
> 
> Note that the call to conf_get_section() in conf_parse_mntopts() will
> ensure that we never pass the wrong value for an option, but we should
> still avoid sending duplicate options if it's at all possible.

You haven't convinced me that passing duplicate options is incorrect behavior.

The idea was to do this kind of data reduction in one place.  It's either done in the kernel's NFS mount option parser, or in mount.nfs's legacy mount option parser.  There should be one authority on what setting takes effect.

Do your changes preserve the "right-most setting wins" behavior?  This is why the config file logic sends duplicate options: it's easier to simply stack the options onto a list, with the system specific options first, then the server specific, then the mount point specific options.  The kernel's option parser then sorts it out and applies the correct setting.

If the right-most setting of an option is not taking effect, that's a bug.


> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
> support/include/conffile.h | 2 +-
> support/nfs/conffile.c     | 4 +++-
> utils/mount/configfile.c   | 2 +-
> 3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/support/include/conffile.h b/support/include/conffile.h
> index ce7aa21..05ea5d2 100644
> --- a/support/include/conffile.h
> +++ b/support/include/conffile.h
> @@ -54,7 +54,7 @@ extern int      conf_end(int, int);
> extern void     conf_free_list(struct conf_list *);
> extern struct sockaddr *conf_get_address(char *, char *);
> extern struct conf_list *conf_get_list(char *, char *);
> -extern struct conf_list *conf_get_tag_list(char *);
> +extern struct conf_list *conf_get_tag_list(char *, char *);
> extern int      conf_get_num(char *, char *, int);
> extern char    *conf_get_str(char *, char *);
> extern char    *conf_get_section(char *, char *, char *);
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index 5015e94..c3434d5 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -565,7 +565,7 @@ cleanup:
> }
> 
> struct conf_list *
> -conf_get_tag_list(char *section)
> +conf_get_tag_list(char *section, char *arg)
> {
> 	struct conf_list *list = 0;
> 	struct conf_list_node *node;
> @@ -579,6 +579,8 @@ conf_get_tag_list(char *section)
> 	cb = LIST_FIRST(&conf_bindings[conf_hash (section)]);
> 	for (; cb; cb = LIST_NEXT(cb, link)) {
> 		if (strcasecmp (section, cb->section) == 0) {
> +			if (arg != NULL && strcasecmp(arg, cb->arg) != 0)
> +				continue;
> 			list->cnt++;
> 			node = calloc(1, sizeof *node);
> 			if (!node)
> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> index 6f2ee75..1f1b6e7 100644
> --- a/utils/mount/configfile.c
> +++ b/utils/mount/configfile.c
> @@ -286,7 +286,7 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
> 	char *nvalue, *ptr;
> 	int argtype;
> 
> -	list = conf_get_tag_list(section);
> +	list = conf_get_tag_list(section, arg);
> 	TAILQ_FOREACH(node, &list->fields, link) {
> 		/*
> 		 * Do not overwrite options if already exists 
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [nfs-utils PATCH 1/4] mount.nfs: avoid unnecessary duplication of options passed to mount(2)

[Scott Mayhew]

On Tue, 13 Aug 2013, Chuck Lever wrote:

> 
> On Aug 13, 2013, at 3:20 PM, Scott Mayhew <smayhew@redhat.com> wrote:
> 
> > The nfsmount.conf file has the following format:
> > 
> > [ section "arg" ]
> > 	tag = value
> > 
> > conf_get_tag_list() currently doesn't check the arg field so we wind up
> > getting all the options that fall under a particular section value,
> > instead of just the ones that match the specific "arg" field.  As a
> > result, we wind up passing options to the mount syscall multiple times.
> > 
> > For example, if we have three different server sections, and each
> > section has an Nfsvers tag, then the string we pass to the mount syscall
> > will have three occurrences of the nfsvers option.
> > 
> > Note that the call to conf_get_section() in conf_parse_mntopts() will
> > ensure that we never pass the wrong value for an option, but we should
> > still avoid sending duplicate options if it's at all possible.
> 
> You haven't convinced me that passing duplicate options is incorrect behavior.
> 
> The idea was to do this kind of data reduction in one place.  It's either done in the kernel's NFS mount option parser, or in mount.nfs's legacy mount option parser.  There should be one authority on what setting takes effect.
> 
> Do your changes preserve the "right-most setting wins" behavior? 

Yes, they do.

> This is why the config file logic sends duplicate options: it's easier to simply stack the options onto a list, with the system specific options first, then the server specific, then the mount point specific options.  The kernel's option parser then sorts it out and applies the correct setting.
> 

I would expect to see a particular mount option at most 3 times then
(once from the global options, once from the server specific options,
and once from the mountpoint-specific options).  That's fine (although
that's what patch 2 was changing). 

The problem is that we're adding that option to the list based on config
file settings that aren't even relevant to the mount operation we happen
to be performing.  For instance if your nfsmount.conf has 100 server
sections, and each of those (along with your global and
mountpoint-specific sections) has an nfsvers option, then you could wind
up sending the nfsvers option 102 times.

If you have enough entries in your config file that the option string
exceeds one page then chances are an option is going to be lopped off
somewhere in the middle and the mount will likely fail with -EINVAL (and
even if that doesn't happen the rightmost options will most likely to be
lost).

-Scott

> If the right-most setting of an option is not taking effect, that's a bug.
> 
> 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> > support/include/conffile.h | 2 +-
> > support/nfs/conffile.c     | 4 +++-
> > utils/mount/configfile.c   | 2 +-
> > 3 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/support/include/conffile.h b/support/include/conffile.h
> > index ce7aa21..05ea5d2 100644
> > --- a/support/include/conffile.h
> > +++ b/support/include/conffile.h
> > @@ -54,7 +54,7 @@ extern int      conf_end(int, int);
> > extern void     conf_free_list(struct conf_list *);
> > extern struct sockaddr *conf_get_address(char *, char *);
> > extern struct conf_list *conf_get_list(char *, char *);
> > -extern struct conf_list *conf_get_tag_list(char *);
> > +extern struct conf_list *conf_get_tag_list(char *, char *);
> > extern int      conf_get_num(char *, char *, int);
> > extern char    *conf_get_str(char *, char *);
> > extern char    *conf_get_section(char *, char *, char *);
> > diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> > index 5015e94..c3434d5 100644
> > --- a/support/nfs/conffile.c
> > +++ b/support/nfs/conffile.c
> > @@ -565,7 +565,7 @@ cleanup:
> > }
> > 
> > struct conf_list *
> > -conf_get_tag_list(char *section)
> > +conf_get_tag_list(char *section, char *arg)
> > {
> > 	struct conf_list *list = 0;
> > 	struct conf_list_node *node;
> > @@ -579,6 +579,8 @@ conf_get_tag_list(char *section)
> > 	cb = LIST_FIRST(&conf_bindings[conf_hash (section)]);
> > 	for (; cb; cb = LIST_NEXT(cb, link)) {
> > 		if (strcasecmp (section, cb->section) == 0) {
> > +			if (arg != NULL && strcasecmp(arg, cb->arg) != 0)
> > +				continue;
> > 			list->cnt++;
> > 			node = calloc(1, sizeof *node);
> > 			if (!node)
> > diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> > index 6f2ee75..1f1b6e7 100644
> > --- a/utils/mount/configfile.c
> > +++ b/utils/mount/configfile.c
> > @@ -286,7 +286,7 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
> > 	char *nvalue, *ptr;
> > 	int argtype;
> > 
> > -	list = conf_get_tag_list(section);
> > +	list = conf_get_tag_list(section, arg);
> > 	TAILQ_FOREACH(node, &list->fields, link) {
> > 		/*
> > 		 * Do not overwrite options if already exists 
> > -- 
> > 1.7.11.7
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

Re: [nfs-utils PATCH 1/4] mount.nfs: avoid unnecessary duplication of options passed to mount(2)

[Chuck Lever]

On Aug 13, 2013, at 6:39 PM, Scott Mayhew <smayhew@redhat.com> wrote:

> On Tue, 13 Aug 2013, Chuck Lever wrote:
> 
>> 
>> On Aug 13, 2013, at 3:20 PM, Scott Mayhew <smayhew@redhat.com> wrote:
>> 
>>> The nfsmount.conf file has the following format:
>>> 
>>> [ section "arg" ]
>>> 	tag = value
>>> 
>>> conf_get_tag_list() currently doesn't check the arg field so we wind up
>>> getting all the options that fall under a particular section value,
>>> instead of just the ones that match the specific "arg" field.  As a
>>> result, we wind up passing options to the mount syscall multiple times.
>>> 
>>> For example, if we have three different server sections, and each
>>> section has an Nfsvers tag, then the string we pass to the mount syscall
>>> will have three occurrences of the nfsvers option.
>>> 
>>> Note that the call to conf_get_section() in conf_parse_mntopts() will
>>> ensure that we never pass the wrong value for an option, but we should
>>> still avoid sending duplicate options if it's at all possible.
>> 
>> You haven't convinced me that passing duplicate options is incorrect behavior.
>> 
>> The idea was to do this kind of data reduction in one place.  It's either done in the kernel's NFS mount option parser, or in mount.nfs's legacy mount option parser.  There should be one authority on what setting takes effect.
>> 
>> Do your changes preserve the "right-most setting wins" behavior? 
> 
> Yes, they do.
> 
>> This is why the config file logic sends duplicate options: it's easier to simply stack the options onto a list, with the system specific options first, then the server specific, then the mount point specific options.  The kernel's option parser then sorts it out and applies the correct setting.
>> 
> 
> I would expect to see a particular mount option at most 3 times then
> (once from the global options, once from the server specific options,
> and once from the mountpoint-specific options).  That's fine (although
> that's what patch 2 was changing).

(Well, to be precise, possibly 4 times: once for the system section, once for server-specific section, once for mount-specific options, and once in the command line mount options).

> The problem is that we're adding that option to the list based on config
> file settings that aren't even relevant to the mount operation we happen
> to be performing.  For instance if your nfsmount.conf has 100 server
> sections, and each of those (along with your global and
> mountpoint-specific sections) has an nfsvers option, then you could wind
> up sending the nfsvers option 102 times.

OK, that's clearly broken.  The config file logic should not include mount options from sections that are not relevant.  If that's all that is addressed in 1/4, that's probably a good fix.

> If you have enough entries in your config file that the option string
> exceeds one page then chances are an option is going to be lopped off
> somewhere in the middle and the mount will likely fail with -EINVAL (and
> even if that doesn't happen the rightmost options will most likely to be
> lost).

I agree, buffer overrun is a valid risk.  I might also suggest that EINVAL is not a terribly descriptive error, and the mount.nfs command could do something to report this error more accurately.

However, I disagree with the claim made in your patch description and cover letter.  Duplicate options, in and of themselves, are not a problem, and the mount option parsing stack leverages that in favor of simplicity.

After the irrelevant mount options are excluded, do you think we are still at risk of overrunning the options buffer for any realistic configuration?

> -Scott
> 
>> If the right-most setting of an option is not taking effect, that's a bug.
>> 
>> 
>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>> ---
>>> support/include/conffile.h | 2 +-
>>> support/nfs/conffile.c     | 4 +++-
>>> utils/mount/configfile.c   | 2 +-
>>> 3 files changed, 5 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/support/include/conffile.h b/support/include/conffile.h
>>> index ce7aa21..05ea5d2 100644
>>> --- a/support/include/conffile.h
>>> +++ b/support/include/conffile.h
>>> @@ -54,7 +54,7 @@ extern int      conf_end(int, int);
>>> extern void     conf_free_list(struct conf_list *);
>>> extern struct sockaddr *conf_get_address(char *, char *);
>>> extern struct conf_list *conf_get_list(char *, char *);
>>> -extern struct conf_list *conf_get_tag_list(char *);
>>> +extern struct conf_list *conf_get_tag_list(char *, char *);
>>> extern int      conf_get_num(char *, char *, int);
>>> extern char    *conf_get_str(char *, char *);
>>> extern char    *conf_get_section(char *, char *, char *);
>>> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
>>> index 5015e94..c3434d5 100644
>>> --- a/support/nfs/conffile.c
>>> +++ b/support/nfs/conffile.c
>>> @@ -565,7 +565,7 @@ cleanup:
>>> }
>>> 
>>> struct conf_list *
>>> -conf_get_tag_list(char *section)
>>> +conf_get_tag_list(char *section, char *arg)
>>> {
>>> 	struct conf_list *list = 0;
>>> 	struct conf_list_node *node;
>>> @@ -579,6 +579,8 @@ conf_get_tag_list(char *section)
>>> 	cb = LIST_FIRST(&conf_bindings[conf_hash (section)]);
>>> 	for (; cb; cb = LIST_NEXT(cb, link)) {
>>> 		if (strcasecmp (section, cb->section) == 0) {
>>> +			if (arg != NULL && strcasecmp(arg, cb->arg) != 0)
>>> +				continue;
>>> 			list->cnt++;
>>> 			node = calloc(1, sizeof *node);
>>> 			if (!node)
>>> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
>>> index 6f2ee75..1f1b6e7 100644
>>> --- a/utils/mount/configfile.c
>>> +++ b/utils/mount/configfile.c
>>> @@ -286,7 +286,7 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
>>> 	char *nvalue, *ptr;
>>> 	int argtype;
>>> 
>>> -	list = conf_get_tag_list(section);
>>> +	list = conf_get_tag_list(section, arg);
>>> 	TAILQ_FOREACH(node, &list->fields, link) {
>>> 		/*
>>> 		 * Do not overwrite options if already exists 
>>> -- 
>>> 1.7.11.7
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> -- 
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
>> 
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [nfs-utils PATCH 1/4] mount.nfs: avoid unnecessary duplication of options passed to mount(2)

[Scott Mayhew]

On Tue, 13 Aug 2013, Chuck Lever wrote:

> 
> On Aug 13, 2013, at 6:39 PM, Scott Mayhew <smayhew@redhat.com> wrote:
> 
> > On Tue, 13 Aug 2013, Chuck Lever wrote:
> > 
> >> 
> >> On Aug 13, 2013, at 3:20 PM, Scott Mayhew <smayhew@redhat.com> wrote:
> >> 
> >>> The nfsmount.conf file has the following format:
> >>> 
> >>> [ section "arg" ]
> >>> 	tag = value
> >>> 
> >>> conf_get_tag_list() currently doesn't check the arg field so we wind up
> >>> getting all the options that fall under a particular section value,
> >>> instead of just the ones that match the specific "arg" field.  As a
> >>> result, we wind up passing options to the mount syscall multiple times.
> >>> 
> >>> For example, if we have three different server sections, and each
> >>> section has an Nfsvers tag, then the string we pass to the mount syscall
> >>> will have three occurrences of the nfsvers option.
> >>> 
> >>> Note that the call to conf_get_section() in conf_parse_mntopts() will
> >>> ensure that we never pass the wrong value for an option, but we should
> >>> still avoid sending duplicate options if it's at all possible.
> >> 
> >> You haven't convinced me that passing duplicate options is incorrect behavior.
> >> 
> >> The idea was to do this kind of data reduction in one place.  It's either done in the kernel's NFS mount option parser, or in mount.nfs's legacy mount option parser.  There should be one authority on what setting takes effect.
> >> 
> >> Do your changes preserve the "right-most setting wins" behavior? 
> > 
> > Yes, they do.
> > 
> >> This is why the config file logic sends duplicate options: it's easier to simply stack the options onto a list, with the system specific options first, then the server specific, then the mount point specific options.  The kernel's option parser then sorts it out and applies the correct setting.
> >> 
> > 
> > I would expect to see a particular mount option at most 3 times then
> > (once from the global options, once from the server specific options,
> > and once from the mountpoint-specific options).  That's fine (although
> > that's what patch 2 was changing).
> 
> (Well, to be precise, possibly 4 times: once for the system section, once for server-specific section, once for mount-specific options, and once in the command line mount options).
> 
> > The problem is that we're adding that option to the list based on config
> > file settings that aren't even relevant to the mount operation we happen
> > to be performing.  For instance if your nfsmount.conf has 100 server
> > sections, and each of those (along with your global and
> > mountpoint-specific sections) has an nfsvers option, then you could wind
> > up sending the nfsvers option 102 times.
> 
> OK, that's clearly broken.  The config file logic should not include mount options from sections that are not relevant.  If that's all that is addressed in 1/4, that's probably a good fix.
> 
> > If you have enough entries in your config file that the option string
> > exceeds one page then chances are an option is going to be lopped off
> > somewhere in the middle and the mount will likely fail with -EINVAL (and
> > even if that doesn't happen the rightmost options will most likely to be
> > lost).
> 
> I agree, buffer overrun is a valid risk.  I might also suggest that EINVAL is not a terribly descriptive error, and the mount.nfs command could do something to report this error more accurately.
> 
> However, I disagree with the claim made in your patch description and cover letter.  Duplicate options, in and of themselves, are not a problem, and the mount option parsing stack leverages that in favor of simplicity.

Okay, I'll fix the patch description and resend it as a standalone
patch.

> 
> After the irrelevant mount options are excluded, do you think we are still at risk of overrunning the options buffer for any realistic configuration?

No.
> 
> > -Scott
> > 
> >> If the right-most setting of an option is not taking effect, that's a bug.
> >> 
> >> 
> >>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> >>> ---
> >>> support/include/conffile.h | 2 +-
> >>> support/nfs/conffile.c     | 4 +++-
> >>> utils/mount/configfile.c   | 2 +-
> >>> 3 files changed, 5 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/support/include/conffile.h b/support/include/conffile.h
> >>> index ce7aa21..05ea5d2 100644
> >>> --- a/support/include/conffile.h
> >>> +++ b/support/include/conffile.h
> >>> @@ -54,7 +54,7 @@ extern int      conf_end(int, int);
> >>> extern void     conf_free_list(struct conf_list *);
> >>> extern struct sockaddr *conf_get_address(char *, char *);
> >>> extern struct conf_list *conf_get_list(char *, char *);
> >>> -extern struct conf_list *conf_get_tag_list(char *);
> >>> +extern struct conf_list *conf_get_tag_list(char *, char *);
> >>> extern int      conf_get_num(char *, char *, int);
> >>> extern char    *conf_get_str(char *, char *);
> >>> extern char    *conf_get_section(char *, char *, char *);
> >>> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> >>> index 5015e94..c3434d5 100644
> >>> --- a/support/nfs/conffile.c
> >>> +++ b/support/nfs/conffile.c
> >>> @@ -565,7 +565,7 @@ cleanup:
> >>> }
> >>> 
> >>> struct conf_list *
> >>> -conf_get_tag_list(char *section)
> >>> +conf_get_tag_list(char *section, char *arg)
> >>> {
> >>> 	struct conf_list *list = 0;
> >>> 	struct conf_list_node *node;
> >>> @@ -579,6 +579,8 @@ conf_get_tag_list(char *section)
> >>> 	cb = LIST_FIRST(&conf_bindings[conf_hash (section)]);
> >>> 	for (; cb; cb = LIST_NEXT(cb, link)) {
> >>> 		if (strcasecmp (section, cb->section) == 0) {
> >>> +			if (arg != NULL && strcasecmp(arg, cb->arg) != 0)
> >>> +				continue;
> >>> 			list->cnt++;
> >>> 			node = calloc(1, sizeof *node);
> >>> 			if (!node)
> >>> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> >>> index 6f2ee75..1f1b6e7 100644
> >>> --- a/utils/mount/configfile.c
> >>> +++ b/utils/mount/configfile.c
> >>> @@ -286,7 +286,7 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
> >>> 	char *nvalue, *ptr;
> >>> 	int argtype;
> >>> 
> >>> -	list = conf_get_tag_list(section);
> >>> +	list = conf_get_tag_list(section, arg);
> >>> 	TAILQ_FOREACH(node, &list->fields, link) {
> >>> 		/*
> >>> 		 * Do not overwrite options if already exists 
> >>> -- 
> >>> 1.7.11.7
> >>> 
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> 
> >> -- 
> >> Chuck Lever
> >> chuck[dot]lever[at]oracle[dot]com
> >> 
> >> 
> >> 
> >> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 

[nfs-utils PATCH 2/4] mount.nfs: avoid sending conflicting mount options

[Scott Mayhew]

conf_parse_mntopts() calls lookup_entry() in an attempt to avoid
"overwriting" of mount options (actually it tries to avoid appending
conflicting options to the string passed to mount(2)).  Currently this doesn't
do much of anything.  For example, if you have Timeo=600 in the global
section, Timeo=300 in the server-specific section, and Timeo=150 in the
mountpoint-specific section, then the string that is passed to the mount
syscall will contain "timeo=600,timeo=300,timeo=150".

Note that the ordering of the conflicting options ensures that the
correct option will ultimately be applied, but we should still avoid sending
the conflicting options in the first place if possible.

This patch fixes all but the MNT_NOARG type options (fg, bg, and sloppy).

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 utils/mount/configfile.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
index 1f1b6e7..221436f 100644
--- a/utils/mount/configfile.c
+++ b/utils/mount/configfile.c
@@ -170,10 +170,16 @@ char *lookup_entry(char *opt)
 {
 	struct entry *entry;
 
-	SLIST_FOREACH(entry, &head, entries) {
-		if (strcasecmp(entry->opt, opt) == 0)
-			return opt;
-	}
+	if (strlen(opt) > 0 && opt[strlen(opt) - 1] == '=')
+		SLIST_FOREACH(entry, &head, entries) {
+			if (strncasecmp(entry->opt, opt, strlen(opt)) == 0)
+				return opt;
+		}
+	else
+		SLIST_FOREACH(entry, &head, entries) {
+			if (strcasecmp(entry->opt, opt) == 0)
+				return opt;
+		}
 	return NULL;
 }
 /*
@@ -300,13 +306,21 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
 		if (opts && check_vers(opts, node->field))
 			continue;
 
-		if (lookup_entry(node->field) != NULL)
+		if (lookup_entry(buf) != NULL)
 			continue;
 		buf[0] = '\0';
 		value = conf_get_section(section, arg, node->field);
 		if (value == NULL)
 			continue;
 		field = mountopts_alias(node->field, &argtype);
+		if (lookup_entry(field) != NULL)
+			continue;
+		if (argtype != MNT_NOARG) {
+			snprintf(buf, BUFSIZ, "no%s", field);
+			if (lookup_entry(buf) != NULL)
+				continue;
+			buf[0] = '\0';
+		}
 		if (strcasecmp(value, "false") == 0) {
 			if (argtype != MNT_NOARG)
 				snprintf(buf, BUFSIZ, "no%s", field);
-- 
1.7.11.7

[nfs-utils PATCH 3/4] mount.nfs: improve handling of MNT_NOARG type options

[Scott Mayhew]

conf_parse_mntopts() maintains a linked list of options that will
ultimately be passed in the data field of the mount() syscall in order
to avoid unnecessary duplicates and/or conflicts.  This doesn't work
very well for MNT_NOARG type options, since setting any of these to
false in the nfsmount.conf doesn't correspond to any 'real' mount option
(i.e. there are no such options 'nobg', 'nofg', and 'nosloppy').

This patch adds a set of internal variables for the three real MNT_NOARG
options (bg, fg, sloppy) along with their pseudo options (nobg, nofg,
nosloppy), and a helper function to track which of these options has
been previously seen and to determine whether or not to append an option
to the linked list.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 utils/mount/configfile.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
index 221436f..623c886 100644
--- a/utils/mount/configfile.c
+++ b/utils/mount/configfile.c
@@ -73,6 +73,8 @@ struct mnt_alias {
 };
 int mnt_alias_sz = (sizeof(mnt_alias_tab)/sizeof(mnt_alias_tab[0]));
 
+static int bg, nobg, fg, nofg, sloppy, nosloppy;
+
 /*
  * See if the option is an alias, if so return the 
  * real mount option along with the argument type.
@@ -278,6 +280,46 @@ default_value(char *mopt)
 
 	return 1;
 }
+
+int
+should_add_noarg_opt(char *opt, char *val)
+{
+	int ret = 0;
+
+	if (strcasecmp(opt, "bg") == 0) {
+		if (strcasecmp(val, "true") == 0) {
+			if (bg == 0 && nobg == 0 && fg == 0) {
+				bg = 1;
+				ret = 1;
+			}
+		} else if (strcasecmp(val, "false") == 0) {
+			if (bg == 0 && nobg == 0)
+				nobg = 1;
+		}
+	} else if (strcasecmp(opt, "fg") == 0) {
+		if (strcasecmp(val, "true") == 0) {
+			if (fg == 0 && nofg == 0 && bg == 0) {
+				fg = 1;
+				ret = 1;
+			}
+		} else if (strcasecmp(val, "false") == 0) {
+			if (fg == 0 && nofg == 0)
+				nofg = 1;
+		}
+	} else if (strcasecmp(opt, "sloppy") == 0) {
+		if (sloppy == 0 && nosloppy == 0) {
+			if(strcasecmp(val, "true") == 0) {
+				sloppy = 1;
+				ret = 1;
+			} else
+				nosloppy = 1;
+		}
+	} else
+		xlog_warn("Invalid MNT_NOARG option: '%s'", opt);
+
+	return ret;
+}
+
 /*
  * Parse the given section of the configuration 
  * file to if there are any mount options set.
@@ -315,6 +357,13 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
 		field = mountopts_alias(node->field, &argtype);
 		if (lookup_entry(field) != NULL)
 			continue;
+		if (argtype == MNT_NOARG) {
+			if (should_add_noarg_opt(field, value)) {
+				list_size += strlen(field) + 1;
+				add_entry(field);
+			}
+			continue;
+		}
 		if (argtype != MNT_NOARG) {
 			snprintf(buf, BUFSIZ, "no%s", field);
 			if (lookup_entry(buf) != NULL)
@@ -359,6 +408,7 @@ char *conf_get_mntopts(char *spec, char *mount_point,
 	char *ptr, *server, *config_opts;
 	int optlen = 0;
 
+	bg = nobg = fg = nofg = sloppy = nosloppy = 0;
 	SLIST_INIT(&head);
 	list_size = 0;
 	/*
-- 
1.7.11.7

Re: [nfs-utils PATCH 3/4] mount.nfs: improve handling of MNT_NOARG type options

[Chuck Lever]

Hi Scott-

On Aug 13, 2013, at 3:20 PM, Scott Mayhew <smayhew@redhat.com> wrote:

> conf_parse_mntopts() maintains a linked list of options that will
> ultimately be passed in the data field of the mount() syscall in order
> to avoid unnecessary duplicates and/or conflicts.  This doesn't work
> very well for MNT_NOARG type options, since setting any of these to
> false in the nfsmount.conf doesn't correspond to any 'real' mount option
> (i.e. there are no such options 'nobg', 'nofg', and 'nosloppy').

What's broken here, exactly?

> 
> This patch adds a set of internal variables for the three real MNT_NOARG
> options (bg, fg, sloppy) along with their pseudo options (nobg, nofg,
> nosloppy), and a helper function to track which of these options has
> been previously seen and to determine whether or not to append an option
> to the linked list.
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
> utils/mount/configfile.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
> 
> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> index 221436f..623c886 100644
> --- a/utils/mount/configfile.c
> +++ b/utils/mount/configfile.c
> @@ -73,6 +73,8 @@ struct mnt_alias {
> };
> int mnt_alias_sz = (sizeof(mnt_alias_tab)/sizeof(mnt_alias_tab[0]));
> 
> +static int bg, nobg, fg, nofg, sloppy, nosloppy;
> +
> /*
>  * See if the option is an alias, if so return the 
>  * real mount option along with the argument type.
> @@ -278,6 +280,46 @@ default_value(char *mopt)
> 
> 	return 1;
> }
> +
> +int
> +should_add_noarg_opt(char *opt, char *val)
> +{
> +	int ret = 0;
> +
> +	if (strcasecmp(opt, "bg") == 0) {
> +		if (strcasecmp(val, "true") == 0) {
> +			if (bg == 0 && nobg == 0 && fg == 0) {
> +				bg = 1;
> +				ret = 1;
> +			}
> +		} else if (strcasecmp(val, "false") == 0) {
> +			if (bg == 0 && nobg == 0)
> +				nobg = 1;
> +		}
> +	} else if (strcasecmp(opt, "fg") == 0) {
> +		if (strcasecmp(val, "true") == 0) {
> +			if (fg == 0 && nofg == 0 && bg == 0) {
> +				fg = 1;
> +				ret = 1;
> +			}
> +		} else if (strcasecmp(val, "false") == 0) {
> +			if (fg == 0 && nofg == 0)
> +				nofg = 1;
> +		}
> +	} else if (strcasecmp(opt, "sloppy") == 0) {
> +		if (sloppy == 0 && nosloppy == 0) {
> +			if(strcasecmp(val, "true") == 0) {
> +				sloppy = 1;
> +				ret = 1;
> +			} else
> +				nosloppy = 1;
> +		}
> +	} else
> +		xlog_warn("Invalid MNT_NOARG option: '%s'", opt);
> +
> +	return ret;
> +}
> +
> /*
>  * Parse the given section of the configuration 
>  * file to if there are any mount options set.
> @@ -315,6 +357,13 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
> 		field = mountopts_alias(node->field, &argtype);
> 		if (lookup_entry(field) != NULL)
> 			continue;
> +		if (argtype == MNT_NOARG) {
> +			if (should_add_noarg_opt(field, value)) {
> +				list_size += strlen(field) + 1;
> +				add_entry(field);
> +			}
> +			continue;
> +		}
> 		if (argtype != MNT_NOARG) {
> 			snprintf(buf, BUFSIZ, "no%s", field);
> 			if (lookup_entry(buf) != NULL)
> @@ -359,6 +408,7 @@ char *conf_get_mntopts(char *spec, char *mount_point,
> 	char *ptr, *server, *config_opts;
> 	int optlen = 0;
> 
> +	bg = nobg = fg = nofg = sloppy = nosloppy = 0;
> 	SLIST_INIT(&head);
> 	list_size = 0;
> 	/*
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [nfs-utils PATCH 3/4] mount.nfs: improve handling of MNT_NOARG type options

[Scott Mayhew]

On Tue, 13 Aug 2013, Chuck Lever wrote:

> Hi Scott-
> 
> On Aug 13, 2013, at 3:20 PM, Scott Mayhew <smayhew@redhat.com> wrote:
> 
> > conf_parse_mntopts() maintains a linked list of options that will
> > ultimately be passed in the data field of the mount() syscall in order
> > to avoid unnecessary duplicates and/or conflicts.  This doesn't work
> > very well for MNT_NOARG type options, since setting any of these to
> > false in the nfsmount.conf doesn't correspond to any 'real' mount option
> > (i.e. there are no such options 'nobg', 'nofg', and 'nosloppy').
> 
> What's broken here, exactly?

The problem here is that specifying something like bg=false, fg=false, or 
sloppy=false doesn't perform any negation if those options appeared
somewhere in an earlier section.  Take for example a simple config that 
ooks like this:

[ NFSMount_Global_Options ]
        Nfsvers=4

[ Server "nfs.smayhew.test" ]
        Background=True

[ MountPoint "/mnt" ]
        Background=False

If I try to perform a mount using that server and mountpoint, and the
server happens to be unresponsive, then what should happen is that the
mount should time out after 2 minutes.  What actually happens is that
we'll keep retrying for 10000 minutes.

-Scott
> 
> > 
> > This patch adds a set of internal variables for the three real MNT_NOARG
> > options (bg, fg, sloppy) along with their pseudo options (nobg, nofg,
> > nosloppy), and a helper function to track which of these options has
> > been previously seen and to determine whether or not to append an option
> > to the linked list.
> > 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> > utils/mount/configfile.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> > 
> > diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> > index 221436f..623c886 100644
> > --- a/utils/mount/configfile.c
> > +++ b/utils/mount/configfile.c
> > @@ -73,6 +73,8 @@ struct mnt_alias {
> > };
> > int mnt_alias_sz = (sizeof(mnt_alias_tab)/sizeof(mnt_alias_tab[0]));
> > 
> > +static int bg, nobg, fg, nofg, sloppy, nosloppy;
> > +
> > /*
> >  * See if the option is an alias, if so return the 
> >  * real mount option along with the argument type.
> > @@ -278,6 +280,46 @@ default_value(char *mopt)
> > 
> > 	return 1;
> > }
> > +
> > +int
> > +should_add_noarg_opt(char *opt, char *val)
> > +{
> > +	int ret = 0;
> > +
> > +	if (strcasecmp(opt, "bg") == 0) {
> > +		if (strcasecmp(val, "true") == 0) {
> > +			if (bg == 0 && nobg == 0 && fg == 0) {
> > +				bg = 1;
> > +				ret = 1;
> > +			}
> > +		} else if (strcasecmp(val, "false") == 0) {
> > +			if (bg == 0 && nobg == 0)
> > +				nobg = 1;
> > +		}
> > +	} else if (strcasecmp(opt, "fg") == 0) {
> > +		if (strcasecmp(val, "true") == 0) {
> > +			if (fg == 0 && nofg == 0 && bg == 0) {
> > +				fg = 1;
> > +				ret = 1;
> > +			}
> > +		} else if (strcasecmp(val, "false") == 0) {
> > +			if (fg == 0 && nofg == 0)
> > +				nofg = 1;
> > +		}
> > +	} else if (strcasecmp(opt, "sloppy") == 0) {
> > +		if (sloppy == 0 && nosloppy == 0) {
> > +			if(strcasecmp(val, "true") == 0) {
> > +				sloppy = 1;
> > +				ret = 1;
> > +			} else
> > +				nosloppy = 1;
> > +		}
> > +	} else
> > +		xlog_warn("Invalid MNT_NOARG option: '%s'", opt);
> > +
> > +	return ret;
> > +}
> > +
> > /*
> >  * Parse the given section of the configuration 
> >  * file to if there are any mount options set.
> > @@ -315,6 +357,13 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
> > 		field = mountopts_alias(node->field, &argtype);
> > 		if (lookup_entry(field) != NULL)
> > 			continue;
> > +		if (argtype == MNT_NOARG) {
> > +			if (should_add_noarg_opt(field, value)) {
> > +				list_size += strlen(field) + 1;
> > +				add_entry(field);
> > +			}
> > +			continue;
> > +		}
> > 		if (argtype != MNT_NOARG) {
> > 			snprintf(buf, BUFSIZ, "no%s", field);
> > 			if (lookup_entry(buf) != NULL)
> > @@ -359,6 +408,7 @@ char *conf_get_mntopts(char *spec, char *mount_point,
> > 	char *ptr, *server, *config_opts;
> > 	int optlen = 0;
> > 
> > +	bg = nobg = fg = nofg = sloppy = nosloppy = 0;
> > 	SLIST_INIT(&head);
> > 	list_size = 0;
> > 	/*
> > -- 
> > 1.7.11.7
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

-- 
Scott Mayhew, RHCE
Software Maintenance Engineer
Red Hat Global Support Services
smayhew@redhat.com
1-888-REDHAT1 x43741

Re: [nfs-utils PATCH 3/4] mount.nfs: improve handling of MNT_NOARG type options

[Chuck Lever]

On Aug 13, 2013, at 6:45 PM, Scott Mayhew <smayhew@redhat.com> wrote:

> On Tue, 13 Aug 2013, Chuck Lever wrote:
> 
>> Hi Scott-
>> 
>> On Aug 13, 2013, at 3:20 PM, Scott Mayhew <smayhew@redhat.com> wrote:
>> 
>>> conf_parse_mntopts() maintains a linked list of options that will
>>> ultimately be passed in the data field of the mount() syscall in order
>>> to avoid unnecessary duplicates and/or conflicts.  This doesn't work
>>> very well for MNT_NOARG type options, since setting any of these to
>>> false in the nfsmount.conf doesn't correspond to any 'real' mount option
>>> (i.e. there are no such options 'nobg', 'nofg', and 'nosloppy').
>> 
>> What's broken here, exactly?
> 
> The problem here is that specifying something like bg=false, fg=false, or 
> sloppy=false doesn't perform any negation if those options appeared
> somewhere in an earlier section.  Take for example a simple config that 
> ooks like this:
> 
> [ NFSMount_Global_Options ]
>        Nfsvers=4
> 
> [ Server "nfs.smayhew.test" ]
>        Background=True
> 
> [ MountPoint "/mnt" ]
>        Background=False
> 
> If I try to perform a mount using that server and mountpoint, and the
> server happens to be unresponsive, then what should happen is that the
> mount should time out after 2 minutes.  What actually happens is that
> we'll keep retrying for 10000 minutes.

"bg" and "fg" negate each other.  "Background=True" should mean "bg" and "Background=False" should mean "fg."

"sloppy" is another matter… It was originally not a mount option, but a command line option on the mount.nfs command.  When mount option parsing was moved into the kernel, we had to make "sloppy" a real mount option.

It probably requires the approach you took.  However, that means that the mount command line cannot negate "sloppy" set in the config file.


> -Scott
>> 
>>> 
>>> This patch adds a set of internal variables for the three real MNT_NOARG
>>> options (bg, fg, sloppy) along with their pseudo options (nobg, nofg,
>>> nosloppy), and a helper function to track which of these options has
>>> been previously seen and to determine whether or not to append an option
>>> to the linked list.
>>> 
>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>> ---
>>> utils/mount/configfile.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 50 insertions(+)
>>> 
>>> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
>>> index 221436f..623c886 100644
>>> --- a/utils/mount/configfile.c
>>> +++ b/utils/mount/configfile.c
>>> @@ -73,6 +73,8 @@ struct mnt_alias {
>>> };
>>> int mnt_alias_sz = (sizeof(mnt_alias_tab)/sizeof(mnt_alias_tab[0]));
>>> 
>>> +static int bg, nobg, fg, nofg, sloppy, nosloppy;
>>> +
>>> /*
>>> * See if the option is an alias, if so return the 
>>> * real mount option along with the argument type.
>>> @@ -278,6 +280,46 @@ default_value(char *mopt)
>>> 
>>> 	return 1;
>>> }
>>> +
>>> +int
>>> +should_add_noarg_opt(char *opt, char *val)
>>> +{
>>> +	int ret = 0;
>>> +
>>> +	if (strcasecmp(opt, "bg") == 0) {
>>> +		if (strcasecmp(val, "true") == 0) {
>>> +			if (bg == 0 && nobg == 0 && fg == 0) {
>>> +				bg = 1;
>>> +				ret = 1;
>>> +			}
>>> +		} else if (strcasecmp(val, "false") == 0) {
>>> +			if (bg == 0 && nobg == 0)
>>> +				nobg = 1;
>>> +		}
>>> +	} else if (strcasecmp(opt, "fg") == 0) {
>>> +		if (strcasecmp(val, "true") == 0) {
>>> +			if (fg == 0 && nofg == 0 && bg == 0) {
>>> +				fg = 1;
>>> +				ret = 1;
>>> +			}
>>> +		} else if (strcasecmp(val, "false") == 0) {
>>> +			if (fg == 0 && nofg == 0)
>>> +				nofg = 1;
>>> +		}
>>> +	} else if (strcasecmp(opt, "sloppy") == 0) {
>>> +		if (sloppy == 0 && nosloppy == 0) {
>>> +			if(strcasecmp(val, "true") == 0) {
>>> +				sloppy = 1;
>>> +				ret = 1;
>>> +			} else
>>> +				nosloppy = 1;
>>> +		}
>>> +	} else
>>> +		xlog_warn("Invalid MNT_NOARG option: '%s'", opt);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> /*
>>> * Parse the given section of the configuration 
>>> * file to if there are any mount options set.
>>> @@ -315,6 +357,13 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
>>> 		field = mountopts_alias(node->field, &argtype);
>>> 		if (lookup_entry(field) != NULL)
>>> 			continue;
>>> +		if (argtype == MNT_NOARG) {
>>> +			if (should_add_noarg_opt(field, value)) {
>>> +				list_size += strlen(field) + 1;
>>> +				add_entry(field);
>>> +			}
>>> +			continue;
>>> +		}
>>> 		if (argtype != MNT_NOARG) {
>>> 			snprintf(buf, BUFSIZ, "no%s", field);
>>> 			if (lookup_entry(buf) != NULL)
>>> @@ -359,6 +408,7 @@ char *conf_get_mntopts(char *spec, char *mount_point,
>>> 	char *ptr, *server, *config_opts;
>>> 	int optlen = 0;
>>> 
>>> +	bg = nobg = fg = nofg = sloppy = nosloppy = 0;
>>> 	SLIST_INIT(&head);
>>> 	list_size = 0;
>>> 	/*
>>> -- 
>>> 1.7.11.7
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> -- 
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
>> 
>> 
> 
> -- 
> Scott Mayhew, RHCE
> Software Maintenance Engineer
> Red Hat Global Support Services
> smayhew@redhat.com
> 1-888-REDHAT1 x43741
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[nfs-utils PATCH 4/4] mount.nfs: clean up conf_parse_mntopts()

[Scott Mayhew]

It's no longer necessary for conf_parse_mntopts to explicitly check for
argtype != MNT_NOARG after calling should_add_noarg_opt().

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 utils/mount/configfile.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
index 623c886..8f68a6e 100644
--- a/utils/mount/configfile.c
+++ b/utils/mount/configfile.c
@@ -364,15 +364,12 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
 			}
 			continue;
 		}
-		if (argtype != MNT_NOARG) {
-			snprintf(buf, BUFSIZ, "no%s", field);
-			if (lookup_entry(buf) != NULL)
-				continue;
-			buf[0] = '\0';
-		}
+		snprintf(buf, BUFSIZ, "no%s", field);
+		if (lookup_entry(buf) != NULL)
+			continue;
+		buf[0] = '\0';
 		if (strcasecmp(value, "false") == 0) {
-			if (argtype != MNT_NOARG)
-				snprintf(buf, BUFSIZ, "no%s", field);
+			snprintf(buf, BUFSIZ, "no%s", field);
 		} else if (strcasecmp(value, "true") == 0) {
 			snprintf(buf, BUFSIZ, "%s", field);
 		} else {
-- 
1.7.11.7