Re: [PATCH] mount.nfs: return error if proto= option specified IPv6 when IPv6 isn't supported

Chuck Lever
2010-02-08T17:59:42Z

On 02/08/2010 12:43 PM, Jeff Layton wrote:
> On Mon, 08 Feb 2010 12:27:24 -0500
> Chuck Lever<chuck.lever@oracle.com> wrote:
>
>> On 02/08/2010 12:14 PM, Jeff Layton wrote:
>>> Right now, there's nothing that expressly forbids someone from
>>> specifying proto=tcp6 for instance, even when nfs-utils it built without
>>> IPv6 support. This may not work well if (for instance) they are using
>>> NFSv3, since statd won't support IPv6. Explicitly return an error if
>>> someone specifies an IPv6 proto= or mountproto= option and IPv6 isn't
>>> supported.
>>>
>>> Signed-off-by: Jeff Layton<jlayton@redhat.com>
>>> ---
>>> utils/mount/network.c | 34 +++++++++++++++++++++++++++-------
>>> 1 files changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>> index c400dd8..ad165f4 100644
>>> --- a/utils/mount/network.c
>>> +++ b/utils/mount/network.c
>>> @@ -1334,9 +1334,26 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
>>>
>>> #ifdef IPV6_SUPPORTED
>>> sa_family_t config_default_family = AF_UNSPEC;
>>> -#else
>>> +
>>> +static int
>>> +nfs_verify_family(sa_family_t family)
>>> +{
>>> + return 1;
>>> +}
>>> +#else /* IPV6_SUPPORTED */
>>> sa_family_t config_default_family = AF_INET;
>>> -#endif
>>> +
>>> +static int
>>> +nfs_verify_family(sa_family_t family)
>>> +{
>>> + if (family != AF_INET) {
>>> + errno = EAFNOSUPPORT;
>>> + return 0;
>>
>> I assume you do this so that mount.nfs emits a proper error message in
>> this case. What about below where nfs_get_proto() returns false?
>>
>
> Yep, otherwise if you specify a "Defaultproto=tcp6" in the config file,
> you'll get:
>
> mount.nfs: Unable to set default family : Success
>
> ...which is a little confusing.
>
> I'm not sure what to do about nfs_get_proto returning false. default_value()
> calls this before the address family stuff:
>
> if (!nfs_nfs_protocol(options,&config_default_proto)) {
> xlog_warn("Unable to set default protocol : %s",
> strerror(errno));
> }

By the way, "%m" is equivalent to "%s", strerror(errno)

> ...and I assume that it'll also always display "Success" there too,
> since errno doesn't seem to be set by nfs_get_proto. Maybe
> nfs_get_proto() should set errno?

Maybe.

I would think that nfs_nfs_protocol, being a part of the mount command
proper, should worry about what error message mount.nfs should display.
The functions in getport.c are supposed to be more generic, and often
set the rpccreate_err fields instead of or in addition to errno. (Never
mind that nobody but mount.nfs uses them).

So, I'd say you probably should have nfs_nfs_protocol set errno as
needed, as you did with nfs_verify_family.

> Again though, that sort of seems like a separate patch from this.
>
>>> + }
>>> +
>>> + return 1;
>>> +}
>>> +#endif /* IPV6_SUPPORTED */
>>>
>>> /*
>>> * Returns TRUE and fills in @family if a valid NFS protocol option
>>> @@ -1357,15 +1374,15 @@ int nfs_nfs_proto_family(struct mount_options *options,
>>> return 1;
>>> case 2: /* proto */
>>> option = po_get(options, "proto");
>>> - if (option != NULL)
>>> - return nfs_get_proto(option, family,&protocol);
>>> + if (option != NULL&& !nfs_get_proto(option, family,&protocol))
>>> + return 0;
>>> }
>>>
>>> /*
>>> * NFS transport protocol wasn't specified. Return the
>>> * default address family.
>>> */
>>
>> It would help if this comment mentioned why you need to verify *family
>> in the default case.
>>
>>> - return 1;
>>> + return nfs_verify_family(*family);
>
> I suppose we don't really need to check it for the default case. I just
> did that since it made fewer conditionals in the code. I can reorganize
> it so that it's not needed if that'll be clearer.
>
>>> }
>>>
>>> /*
>>> @@ -1494,8 +1511,11 @@ int nfs_mount_proto_family(struct mount_options *options,
>>> *family = config_default_family;
>>>
>>> option = po_get(options, "mountproto");
>>> - if (option != NULL)
>>> - return nfs_get_proto(option, family,&protocol);
>>> + if (option != NULL) {
>>> + if (!nfs_get_proto(option, family,&protocol))
>>> + return 0;
>>> + return nfs_verify_family(*family);
>>> + }
>>>
>>> /*
>>> * MNT transport protocol wasn't specified. If the NFS
>>
>
>

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