On Mon, 17 Nov 2014, Steve Dickson wrote:
> Hey Ben,
>
> On 11/17/2014 02:43 PM, Benjamin Coddington wrote:
> > Update nfsmount.conf to allow minor version specification, and rearrange
> > the autonegotiation logic to agreed upon best behavior. Depending upon the
> > combination of values specified within nfsmount.conf and given to mount.nfs,
> > the retry behavior of mount.nfs varies according to the general rule of
> > defaulting to the most specific setting, with some exceptions. A general
> > guide to the expected behavior follows:
> Would you mind breaking this patch up into a more digestible patch
> series defined by functionality. 199 insertions and 150 deletions
> in one patch makes it a bit tough to digest it... at least for me. :-)
Yes, I'll do that.
> >
> > ┌────────────────┐
> > │ nfsmount.conf ├──────────────┬───────────────────┐
> > │ Defaultvers= │ arg option │ attempts: │
> > ├────────────────┼──────────────┼───────────────────┤
> > │ 4.2 │ not set │ v4.2 v4.1 v4.0 v3 │
> > │ 4.2 │ v4 │ v4.2 v4.1 v4.0 │
> > │ 4.1 │ not set │ v4.1 v4.0 v3 │
> > │ 4.1 │ v4 │ v4.1 v4.0 │
> > │ 4 │ not set │ v4.0 v3 │
> > │ 4 │ v4 │ v4.0 │
> > │ 3 │ not set │ v3 │
> > │ any set │ v4.2 │ v4.2 │
> > │ any set │ v4.1 │ v4.1 │
> > │ any set │ v4 │ v4.0 │
> > │ any set │ v3 │ v3 │
> > │ not set │ not set │ v4.0 v3 │
> > └────────────────┴──────────────┴───────────────────┘
> Why is "not set" and "not set" not the same as Defaultvers=4.2?
Oh, because I made a mistake in trying to handle another case. That's
the whole point of this work! I'll fix it.
> >
> > If built without --enable-mountconfig, then the behavior is the same as if
> > nfsmount.conf did not set Defaultvers.
> We probably should switch this around so mountconfig is always enabled and
> let people disable if they want.
I can do that, too.
> >
> > The last case in the above table is a special case of disagreement about how
> > to determine the upper bound for a minor number of the v4 protocol. It
> > could be argued that mount.nfs should start at the highest available
> > protocol version, if that version could be discovered at mount time. For
> > now, the upper bound on the value can be modified by setting the values
> > within nfs_default_version().
> For now lets just hard code the upper bound a 4.2. Minor version don't come
> along very often so the recompiling of mount.nfs will not be that
> big of deal...
Right, that'll get fixed when I fix the case above.
Ben
>
> steved.
>
> >
> > Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> > ---
> > utils/mount/configfile.c | 36 +-----------
> > utils/mount/network.c | 122 +++++++++++++++++++--------------------
> > utils/mount/network.h | 15 +++++-
> > utils/mount/nfsumount.c | 4 +-
> > utils/mount/parse_opt.c | 26 ++++++++-
> > utils/mount/parse_opt.h | 2 +
> > utils/mount/stropts.c | 144 ++++++++++++++++++++++++++++++----------------
> > 7 files changed, 199 insertions(+), 150 deletions(-)
> >
> > diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> > index 39d3741..0a4cc04 100644
> > --- a/utils/mount/configfile.c
> > +++ b/utils/mount/configfile.c
> > @@ -228,37 +228,8 @@ void free_all(void)
> > free(entry);
> > }
> > }
> > -static char *versions[] = {"v2", "v3", "v4", "vers", "nfsvers", NULL};
> > -static int
> > -check_vers(char *mopt, char *field)
> > -{
> > - int i, found=0;
> > -
> > - /*
> > - * First check to see if the config setting is one
> > - * of the many version settings
> > - */
> > - for (i=0; versions[i]; i++) {
> > - if (strcasestr(field, versions[i]) != NULL) {
> > - found++;
> > - break;
> > - }
> > - }
> > - if (!found)
> > - return 0;
> > - /*
> > - * It appears the version is being set, now see
> > - * if the version appears on the command
> > - */
> > - for (i=0; versions[i]; i++) {
> > - if (strcasestr(mopt, versions[i]) != NULL)
> > - return 1;
> > - }
> > -
> > - return 0;
> > -}
> >
> > -unsigned long config_default_vers;
> > +struct nfs_version config_default_vers;
> > unsigned long config_default_proto;
> > extern sa_family_t config_default_family;
> >
> > @@ -331,11 +302,6 @@ conf_parse_mntopts(char *section, char *arg, char *opts)
> > snprintf(buf, BUFSIZ, "%s=", node->field);
> > if (opts && strcasestr(opts, buf) != NULL)
> > continue;
> > - /*
> > - * Protocol verions can be set in a number of ways
> > - */
> > - if (opts && check_vers(opts, node->field))
> > - continue;
> >
> > if (lookup_entry(node->field) != NULL)
> > continue;
> > diff --git a/utils/mount/network.c b/utils/mount/network.c
> > index 4f8c15c..b5ed850 100644
> > --- a/utils/mount/network.c
> > +++ b/utils/mount/network.c
> > @@ -92,9 +92,6 @@ static const char *nfs_version_opttbl[] = {
> > "v4",
> > "vers",
> > "nfsvers",
> > - "v4.0",
> > - "v4.1",
> > - "v4.2",
> > NULL,
> > };
> >
> > @@ -1242,71 +1239,69 @@ nfs_nfs_program(struct mount_options *options, unsigned long *program)
> > * or FALSE if the option was specified with an invalid value.
> > */
> > int
> > -nfs_nfs_version(struct mount_options *options, unsigned long *version)
> > +nfs_nfs_version(struct mount_options *options, struct nfs_version *version)
> > {
> > - long tmp;
> > + char *version_key, *version_val, *cptr;
> > + int i, found = 0;
> >
> > - switch (po_rightmost(options, nfs_version_opttbl)) {
> > - case 0: /* v2 */
> > - *version = 2;
> > - return 1;
> > - case 1: /* v3 */
> > - *version = 3;
> > - return 1;
> > - case 2: /* v4 */
> > - *version = 4;
> > - return 1;
> > - case 3: /* vers */
> > - switch (po_get_numeric(options, "vers", &tmp)) {
> > - case PO_FOUND:
> > - if (tmp >= 2 && tmp <= 4) {
> > - *version = tmp;
> > - return 1;
> > - }
> > - nfs_error(_("%s: parsing error on 'vers=' option\n"),
> > - progname);
> > - return 0;
> > - case PO_NOT_FOUND:
> > - nfs_error(_("%s: parsing error on 'vers=' option\n"),
> > - progname);
> > - return 0;
> > - case PO_BAD_VALUE:
> > - nfs_error(_("%s: invalid value for 'vers=' option"),
> > - progname);
> > - return 0;
> > - }
> > - case 4: /* nfsvers */
> > - switch (po_get_numeric(options, "nfsvers", &tmp)) {
> > - case PO_FOUND:
> > - if (tmp >= 2 && tmp <= 4) {
> > - *version = tmp;
> > - return 1;
> > - }
> > - nfs_error(_("%s: parsing error on 'nfsvers=' option\n"),
> > - progname);
> > - return 0;
> > - case PO_NOT_FOUND:
> > - nfs_error(_("%s: parsing error on 'nfsvers=' option\n"),
> > - progname);
> > - return 0;
> > - case PO_BAD_VALUE:
> > - nfs_error(_("%s: invalid value for 'nfsvers=' option"),
> > - progname);
> > - return 0;
> > + version->v_mode = V_DEFAULT;
> > +
> > + for (i = 0; nfs_version_opttbl[i]; i++) {
> > + if (po_contains_prefix(options, nfs_version_opttbl[i],
> > + &version_key) == PO_FOUND) {
> > + found++;
> > + break;
> > }
> > - case 5: /* v4.0 */
> > - case 6: /* v4.1 */
> > - case 7: /* v4.2 */
> > - *version = 4;
> > + }
> > +
> > + if (!found)
> > return 1;
> > +
> > + if (i <= 2 ) {
> > + /* v2, v3, v4 */
> > + version_val = version_key + 1;
> > + version->v_mode = V_SPECIFIC;
> > + } else if (i > 2 ) {
> > + /* vers=, nfsvers= */
> > + version_val = po_get(options, version_key);
> > }
> >
> > - /*
> > - * NFS version wasn't specified. The pmap version value
> > - * will be filled in later by an rpcbind query in this case.
> > - */
> > - *version = 0;
> > + if (!version_val)
> > + goto ret_error;
> > +
> > + if (!(version->major = strtol(version_val, &cptr, 10)))
> > + goto ret_error;
> > +
> > + if (version->major < 4)
> > + version->v_mode = V_SPECIFIC;
> > +
> > + if (*cptr == '.') {
> > + version_val = ++cptr;
> > + if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val)
> > + goto ret_error;
> > + version->v_mode = V_SPECIFIC;
> > + } else if (version->major > 3 && *cptr == '\0')
> > + version->v_mode = V_GENERAL;
> > +
> > + if (*cptr != '\0')
> > + goto ret_error;
> > +
> > return 1;
> > +
> > +ret_error:
> > + if (i <= 2 ) {
> > + nfs_error(_("%s: parsing error on 'v' option"),
> > + progname);
> > + } else if (i == 3 ) {
> > + nfs_error(_("%s: parsing error on 'vers=' option"),
> > + progname);
> > + } else if (i == 4) {
> > + nfs_error(_("%s: parsing error on 'nfsvers=' option"),
> > + progname);
> > + }
> > + version->v_mode = V_PARSE_ERR;
> > + errno = EINVAL;
> > + return 0;
> > }
> >
> > /*
> > @@ -1625,10 +1620,13 @@ out_err:
> > int nfs_options2pmap(struct mount_options *options,
> > struct pmap *nfs_pmap, struct pmap *mnt_pmap)
> > {
> > + struct nfs_version version;
> > +
> > if (!nfs_nfs_program(options, &nfs_pmap->pm_prog))
> > return 0;
> > - if (!nfs_nfs_version(options, &nfs_pmap->pm_vers))
> > + if (!nfs_nfs_version(options, &version))
> > return 0;
> > + nfs_pmap->pm_vers = version.major;
> > if (!nfs_nfs_protocol(options, &nfs_pmap->pm_prot))
> > return 0;
> > if (!nfs_nfs_port(options, &nfs_pmap->pm_port))
> > diff --git a/utils/mount/network.h b/utils/mount/network.h
> > index d7636d7..9cc5dec 100644
> > --- a/utils/mount/network.h
> > +++ b/utils/mount/network.h
> > @@ -57,9 +57,22 @@ int clnt_ping(struct sockaddr_in *, const unsigned long,
> >
> > struct mount_options;
> >
> > +enum {
> > + V_DEFAULT = 0,
> > + V_GENERAL,
> > + V_SPECIFIC,
> > + V_PARSE_ERR,
> > +};
> > +
> > +struct nfs_version {
> > + unsigned long major;
> > + unsigned long minor;
> > + int v_mode;
> > +};
> > +
> > int nfs_nfs_proto_family(struct mount_options *options, sa_family_t *family);
> > int nfs_mount_proto_family(struct mount_options *options, sa_family_t *family);
> > -int nfs_nfs_version(struct mount_options *options, unsigned long *version);
> > +int nfs_nfs_version(struct mount_options *options, struct nfs_version *version);
> > int nfs_nfs_protocol(struct mount_options *options, unsigned long *protocol);
> >
> > int nfs_options2pmap(struct mount_options *,
> > diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
> > index 3538d88..de284f2 100644
> > --- a/utils/mount/nfsumount.c
> > +++ b/utils/mount/nfsumount.c
> > @@ -179,10 +179,10 @@ static int nfs_umount_is_vers4(const struct mntentchn *mc)
> >
> > options = po_split(pmc->m.mnt_opts);
> > if (options != NULL) {
> > - unsigned long version;
> > + struct nfs_version version;
> > int rc = nfs_nfs_version(options, &version);
> > po_destroy(options);
> > - if (rc && version == 4)
> > + if (rc && version.major == 4)
> > goto out_nfs4;
> > }
> >
> > diff --git a/utils/mount/parse_opt.c b/utils/mount/parse_opt.c
> > index 75a0daa..7ba61c4 100644
> > --- a/utils/mount/parse_opt.c
> > +++ b/utils/mount/parse_opt.c
> > @@ -391,7 +391,7 @@ po_return_t po_append(struct mount_options *options, char *str)
> > }
> >
> > /**
> > - * po_contains - check for presense of an option in a group
> > + * po_contains - check for presence of an option in a group
> > * @options: pointer to mount options
> > * @keyword: pointer to a C string containing option keyword for which to search
> > *
> > @@ -410,6 +410,30 @@ po_found_t po_contains(struct mount_options *options, char *keyword)
> > }
> >
> > /**
> > + * po_contains_prefix - check for presence of an option matching a prefix
> > + * @options: pointer to mount options
> > + * @prefix: pointer to prefix to match against a keyword
> > + * @keyword: pointer to a C string containing the option keyword if found
> > + *
> > + * On success, *keyword contains the pointer of the matching option's keyword.
> > + */
> > +po_found_t po_contains_prefix(struct mount_options *options,
> > + const char *prefix, char **keyword)
> > +{
> > + struct mount_option *option;
> > +
> > + if (options && prefix) {
> > + for (option = options->head; option; option = option->next)
> > + if (strncmp(option->keyword, prefix, strlen(prefix)) == 0) {
> > + *keyword = option->keyword;
> > + return PO_FOUND;
> > + }
> > + }
> > +
> > + return PO_NOT_FOUND;
> > +}
> > +
> > +/**
> > * po_get - return the value of the rightmost instance of an option
> > * @options: pointer to mount options
> > * @keyword: pointer to a C string containing option keyword for which to search
> > diff --git a/utils/mount/parse_opt.h b/utils/mount/parse_opt.h
> > index 5037207..0745e0f 100644
> > --- a/utils/mount/parse_opt.h
> > +++ b/utils/mount/parse_opt.h
> > @@ -45,6 +45,8 @@ po_return_t po_join(struct mount_options *, char **);
> >
> > po_return_t po_append(struct mount_options *, char *);
> > po_found_t po_contains(struct mount_options *, char *);
> > +po_found_t po_contains_prefix(struct mount_options *options,
> > + const char *prefix, char **keyword);
> > char * po_get(struct mount_options *, char *);
> > po_found_t po_get_numeric(struct mount_options *,
> > char *, long *);
> > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> > index 2d72d5b..936f65c 100644
> > --- a/utils/mount/stropts.c
> > +++ b/utils/mount/stropts.c
> > @@ -88,30 +88,44 @@ struct nfsmount_info {
> > struct mount_options *options; /* parsed mount options */
> > char **extra_opts; /* string for /etc/mtab */
> >
> > - unsigned long version; /* NFS version */
> > + struct nfs_version version; /* NFS version */
> > int flags, /* MS_ flags */
> > fake, /* actually do the mount? */
> > child; /* forked bg child? */
> > };
> >
> > -#ifdef MOUNT_CONFIG
> > -static void nfs_default_version(struct nfsmount_info *mi);
> >
> > static void nfs_default_version(struct nfsmount_info *mi)
> > {
> > - extern unsigned long config_default_vers;
> > +#ifdef MOUNT_CONFIG
> > + extern struct nfs_version config_default_vers;
> > /*
> > * Use the default value set in the config file when
> > * the version has not been explicitly set.
> > */
> > - if (mi->version == 0 && config_default_vers) {
> > - if (config_default_vers < 4)
> > - mi->version = config_default_vers;
> > + if (config_default_vers.v_mode == V_PARSE_ERR) {
> > + mi->version.v_mode = V_PARSE_ERR;
> > + return;
> > }
> > -}
> > -#else
> > -inline void nfs_default_version(__attribute__ ((unused)) struct nfsmount_info *mi) {}
> > +
> > + if (mi->version.v_mode == V_DEFAULT &&
> > + config_default_vers.v_mode != V_DEFAULT) {
> > + mi->version.major = config_default_vers.major;
> > + mi->version.minor = config_default_vers.minor;
> > + return;
> > + }
> > +
> > + if (mi->version.v_mode == V_GENERAL &&
> > + config_default_vers.v_mode != V_DEFAULT) {
> > + if (mi->version.major == config_default_vers.major)
> > + mi->version.minor = config_default_vers.minor;
> > + return;
> > + }
> > +
> > #endif /* MOUNT_CONFIG */
> > + mi->version.major = 4;
> > + mi->version.minor = 0;
> > +}
> >
> > /*
> > * Obtain a retry timeout value based on the value of the "retry=" option.
> > @@ -300,7 +314,7 @@ static int nfs_set_version(struct nfsmount_info *mi)
> > return 0;
> >
> > if (strncmp(mi->type, "nfs4", 4) == 0)
> > - mi->version = 4;
> > + mi->version.major = 4;
> >
> > /*
> > * Before 2.6.32, the kernel NFS client didn't
> > @@ -308,28 +322,44 @@ static int nfs_set_version(struct nfsmount_info *mi)
> > * 4 cannot be included when autonegotiating
> > * while running on those kernels.
> > */
> > - if (mi->version == 0 &&
> > - linux_version_code() <= MAKE_VERSION(2, 6, 31))
> > - mi->version = 3;
> > + if (mi->version.v_mode == V_DEFAULT &&
> > + linux_version_code() <= MAKE_VERSION(2, 6, 31)) {
> > + mi->version.major = 3;
> > + mi->version.v_mode = V_SPECIFIC;
> > + }
> >
> > /*
> > * If we still don't know, check for version-specific
> > * mount options.
> > */
> > - if (mi->version == 0) {
> > + if (mi->version.v_mode == V_DEFAULT) {
> > if (po_contains(mi->options, "mounthost") ||
> > po_contains(mi->options, "mountaddr") ||
> > po_contains(mi->options, "mountvers") ||
> > - po_contains(mi->options, "mountproto"))
> > - mi->version = 3;
> > + po_contains(mi->options, "mountproto")) {
> > + mi->version.major = 3;
> > + mi->version.v_mode = V_SPECIFIC;
> > + }
> > }
> >
> > /*
> > * If enabled, see if the default version was
> > * set in the config file
> > */
> > - nfs_default_version(mi);
> > -
> > + if (mi->version.v_mode != V_SPECIFIC) {
> > + nfs_default_version(mi);
> > + /*
> > + * If the version was not specifically set, it will
> > + * be set by autonegotiation later, so remove it now:
> > + */
> > + po_remove_all(mi->options, "v4");
> > + po_remove_all(mi->options, "vers");
> > + po_remove_all(mi->options, "nfsvers");
> > + }
> > +
> > + if (mi->version.v_mode == V_PARSE_ERR)
> > + return 0;
> > +
> > return 1;
> > }
> >
> > @@ -684,6 +714,7 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
> > {
> > struct mount_options *options = po_dup(mi->options);
> > int result = 0;
> > + char version_opt[16];
> > char *extra_opts = NULL;
> >
> > if (!options) {
> > @@ -691,20 +722,24 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
> > return result;
> > }
> >
> > - if (mi->version == 0) {
> > - if (po_contains(options, "mounthost") ||
> > - po_contains(options, "mountaddr") ||
> > - po_contains(options, "mountvers") ||
> > - po_contains(options, "mountproto")) {
> > - /*
> > - * Since these mountd options are set assume version 3
> > - * is wanted so error out with EPROTONOSUPPORT so the
> > - * protocol negation starts with v3.
> > - */
> > - errno = EPROTONOSUPPORT;
> > - goto out_fail;
> > - }
> > - if (po_append(options, "vers=4") == PO_FAILED) {
> > + if (po_contains(options, "mounthost") ||
> > + po_contains(options, "mountaddr") ||
> > + po_contains(options, "mountvers") ||
> > + po_contains(options, "mountproto")) {
> > + /*
> > + * Since these mountd options are set assume version 3
> > + * is wanted so error out with EPROTONOSUPPORT so the
> > + * protocol negation starts with v3.
> > + */
> > + errno = EPROTONOSUPPORT;
> > + goto out_fail;
> > + }
> > +
> > + if (mi->version.v_mode != V_SPECIFIC) {
> > + snprintf(version_opt, sizeof(version_opt) - 1,
> > + "vers=%lu.%lu", mi->version.major, mi->version.minor);
> > +
> > + if (po_append(options, version_opt) == PO_FAILED) {
> > errno = EINVAL;
> > goto out_fail;
> > }
> > @@ -792,14 +827,25 @@ static int nfs_autonegotiate(struct nfsmount_info *mi)
> > int result;
> >
> > result = nfs_try_mount_v4(mi);
> > +check_result:
> > if (result)
> > return result;
> >
> > -check_errno:
> > switch (errno) {
> > case EPROTONOSUPPORT:
> > /* A clear indication that the server or our
> > - * client does not support NFS version 4. */
> > + * client does not support NFS version 4 and minor */
> > + if (mi->version.v_mode == V_GENERAL &&
> > + mi->version.minor == 0)
> > + return result;
> > + if (mi->version.v_mode != V_SPECIFIC) {
> > + if (mi->version.minor > 0) {
> > + mi->version.minor--;
> > + result = nfs_try_mount_v4(mi);
> > + goto check_result;
> > + }
> > + }
> > +
> > goto fall_back;
> > case ENOENT:
> > /* Legacy Linux servers don't export an NFS
> > @@ -818,7 +864,7 @@ check_errno:
> > /* v4 server seems to be registered now. */
> > result = nfs_try_mount_v4(mi);
> > if (result == 0 && errno != ECONNREFUSED)
> > - goto check_errno;
> > + goto check_result;
> > }
> > return result;
> > default:
> > @@ -839,19 +885,19 @@ static int nfs_try_mount(struct nfsmount_info *mi)
> > {
> > int result = 0;
> >
> > - switch (mi->version) {
> > - case 0:
> > - result = nfs_autonegotiate(mi);
> > - break;
> > - case 2:
> > - case 3:
> > - result = nfs_try_mount_v3v2(mi, FALSE);
> > - break;
> > - case 4:
> > - result = nfs_try_mount_v4(mi);
> > - break;
> > - default:
> > - errno = EIO;
> > + switch (mi->version.major) {
> > + case 2:
> > + case 3:
> > + result = nfs_try_mount_v3v2(mi, FALSE);
> > + break;
> > + case 4:
> > + if (mi->version.v_mode != V_SPECIFIC)
> > + result = nfs_autonegotiate(mi);
> > + else
> > + result = nfs_try_mount_v4(mi);
> > + break;
> > + default:
> > + errno = EIO;
> > }
> >
> > return result;
> >
>
