| Message ID | 1376421629-21382-4-git-send-email-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > > > >
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 -- 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
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; /*
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(+)