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

[Chuck Lever <chuck.lever@oracle.com>]

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