On May 7, 2009, at 4:03 PM, Kevin Constantine wrote:
Chuck Lever wrote:
On May 5, 2009, at 5:46 PM, Kevin Constantine wrote:
This allows a user to specify nfsvers=4, or vers=4 on the mount
commandline and have mount.nfs4 called as though fstype=nfs4 were
specified. This patch handles the nfsmount_string case in
mount.c's try_mount().
We get the value of the "vers=" or "nfsvers=" from the nfsmount_info
structure, and if the value equals 4, we set the fstype to nfs4, and
remove the nfsvers/vers options from the structure since it
shouldn't
be there in the first place, and we don't want to pass it along down
the stack.
po_get_numeric returns the rightmost instance, so we honor the last
value of nfsvers/vers in the event that it is overridden later in
the
options string.
Signed-off-by: Kevin Constantine <kevin.constantine@xxxxxxxxxxxxxxxxxxx
>
---
utils/mount/stropts.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index c369136..72b0d13 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -754,6 +754,15 @@ static const char *nfs_background_opttbl[] = {
static int nfsmount_start(struct nfsmount_info *mi)
{
+ long tmp;
+ po_get_numeric(mi->options, "vers", &tmp);
+ po_get_numeric(mi->options, "nfsvers", &tmp);
If someone specifies both a vers= and a nfsvers= on the command
line, this won't handle it. You need to implement a rightmost
search, as Steve's patch (posted previously on this list) did.
I can't seem to find Steve's patches relating to this issue. I'm
assuming that since there are already patches related to this, it's
not worth pursuing these particular patches.
It was on the NFSv4 mailing list. Sorry about that.
--- Begin Message ---
Hi Steve-
Yep, this is the idea. Comments below.
On Mar 23, 2009, at 12:16 PM, Steve Dickson wrote:
> Chuck Lever wrote:
>> On Mar 20, 2009, at Mar 20, 2009, 1:44 PM, Steve Dickson wrote:
>>> Chuck Lever wrote:
>>>> Hi Steve-
>>>>
>>>> You could limit this to just text-based mounts, then the existing
>>>> option
>>>> parser would handle most of this for you.
>>>>
>>> I took a look at that approach... Basically have parse_opts() and/or
>>> parse_opt() do the work figuring out what nfsvers or vers is set to.
>>> But the problem with that, I would have to pass down the address of
>>> fs_type pointer so I could overwritten... Adding that extra
>>> parameter
>>> I didn't think was the best approach... bordering on the hacky
>>> side...
>>>
>>> So took the approach of lets set file system type once and only
>>> once...
>>> Unfortunately that approach does call for me to strip out the
>>> "nfsvers"
>>> or "vers" string from that options string...
>>
>> The text-based path uses a single data structure, nfs_mountinfo,
>> which
>> carries all the mount data through the whole code path. In
>> nfsmount_start() you can add a little logic that resets the fs_type
>> (in
>> the .type field) before proceeding with the rest of the mount.
>>
>> You would also need to expose nfs_nfs_version() in utils/mount/
>> network.c
>> (and add support for version 4) so you can handle the case where
>> someone
>> specifies "vers=" more than once on the command line (or specifies
>> something obnoxious like "v2,nfsvers=4,vers=3", and you can also
>> detect
>> "v4" here as well.
>>
>> There is code at the top of nfs_construct_new_options() which
>> provides
>> an example of how to easily strip out the existing vers options.
>>
>> So nfsmount_start() could call nfs_nfs_version to see what version to
>> use if the passed-in fs_type is "nfs", and if it's version 4, just
>> remove all the vers options, and say 'mi.type = "nfs4";'.
>>
>> All that assumes you don't care about providing nfsvers=4 support for
>> legacy non-text-based mounts.
> Here is the patch implementing your suggestion...
>
> The patches are about the same size... As you mentioned this patch
> does not provide legacy support, which may or may not matter...
>
> Both patches have to remove the version option from the list
> pumped down to the kernel, since the option will cause the
> kernel routines to error out... Its much cleaner to
> remove moved the option 'mount_options' list than stripping
> the option from the string.
>
> So I guess it comes down to do we really care about legacy support??
I don't have a strong opinion, but my preference would be to leave
enhancements like this to text-based mounts only.
> steved.
>
> Using only the string option parsing routine, allow the file system
> type to be changed to 'nfs4' when the NFS version options are
> specified.
>
>
> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx>
> -----------
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index bcd0c0f..612c9c2 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -94,6 +94,12 @@ static const char *nfs_version_opttbl[] = {
> "nfsvers",
> NULL,
> };
> +static const char *nfs_fstype_opttbl[] = {
> + "v4",
> + "vers",
> + "nfsvers",
> + NULL,
> +};
>
> static const unsigned long nfs_to_mnt[] = {
> 0,
> @@ -1242,6 +1248,47 @@ static rpcvers_t nfs_nfs_version(struct
> mount_options *options)
> }
>
> /*
> + * Returns either "nfs" or "nfs4" as the file system type
> + * depending on which (if any) of the nfs version options
> + * are specified.
> + */
> +char *nfs_fs_type(struct mount_options *options)
> +{
> + char *keyword;
> + long tmp;
> + static char *fs_type = "nfs";
> +
> + switch (po_rightmost(options, nfs_fstype_opttbl)) {
As far as I recall, we recently fixed po_rightmost() to return a C-
style array offset. Shouldn't the cases be 0, 1, and 2?
> + case 2: /* v4 */
> + keyword = "v4";
> + break;
> + case 3: /* vers */
> + if (po_get_numeric(options, "vers", &tmp) == PO_FOUND)
> + if (tmp != 4)
> + return fs_type;
> + keyword = "vers";
> + break;
> + case 4: /* nfsvers */
> + if (po_get_numeric(options, "nfsvers", &tmp) == PO_FOUND)
> + if (tmp != 4)
> + return fs_type;
> + keyword = "nfsvers";
> + break;
> + default:
> + return fs_type;
> + }
> +
> + /*
> + * Need to remove the v4 version option from the
> + * list since the kernel will not how to parse it.
> + */
> + po_remove_all(options, keyword);
You always need to remove all of "nfsvers," "vers", "v2", "v3", and
"v4" if this is a nfs4 mount. The reason for this is to make sure the
user hasn't specified something bogus like "nfsvers=2,vers=3,v4". In
that case the correct behavior is to do a "nfs4" mount and strip out
all verison options. For something like "v4,nfsvers=3,vers=2" the
correct behavior is to do a "nfs" mount and remove any v4-related
version options (which the kernel won't like). The rule is "rightmost
wins".
So you need two exits from this function:
* for "nfs" mounts, return "nfs" and make sure any of "v4"
"nfsvers=4" or "vers=4" are removed, and
* for "nfs4" mounts, return "nfs4" and strip all version options
from the mount string.
> + fs_type = "nfs4";
> +
> + return fs_type;
I'm pretty sure you can get the same effect (returning a statically
allocated character string) with:
return "nfs4";
> +}
> +
> +/*
> * Returns the NFS transport protocol specified by the given mount
> options
> *
> * Returns the IPPROTO_ value specified by the given mount options, or
> diff --git a/utils/mount/network.h b/utils/mount/network.h
> index 0dd90f8..203e728 100644
> --- a/utils/mount/network.h
> +++ b/utils/mount/network.h
> @@ -64,6 +64,7 @@ void nfs_options2pmap(struct mount_options *,
> int start_statd(void);
>
> unsigned long nfsvers_to_mnt(const unsigned long);
> +char *nfs_fs_type(struct mount_options *);
If you're not going to use nfs_nfs_version() at all, then this
function would be better off living in utils/mount/stropts.c.
network.c is just for the network-related functions (like creating a
socket, handling DNS lookups, or doing getport calls), and they are
generally shared between legacy and text-based support. This is
clearly going to be a text-based-only kind of thing.
> int nfs_call_umount(clnt_addr_t *, dirpath *);
> int nfs_advise_umount(const struct sockaddr *, const socklen_t,
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index c369136..7a4f533 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -754,6 +754,9 @@ static const char *nfs_background_opttbl[] = {
>
> static int nfsmount_start(struct nfsmount_info *mi)
> {
> +
> + mi->type = nfs_fs_type(mi->options);
> +
> if (!nfs_validate_options(mi))
> return EX_FAIL;
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
_______________________________________________
NFSv4 mailing list
NFSv4@xxxxxxxxxxxxx
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
--- End Message ---
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
