Re: [PATCH] mount.nfs: set the default family for lookups based on Defaultproto= setting

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

On 02/05/2010 12:43 PM, Jeff Layton wrote:
> On Fri, 05 Feb 2010 12:13:08 -0500
> Chuck Lever<chuck.lever@oracle.com>  wrote:
>
>> Hi Jeff-
>>
>> On 02/05/2010 11:30 AM, Jeff Layton wrote:
>>> On Fri, 05 Feb 2010 10:31:28 -0500
>>> Chuck Lever<chuck.lever@oracle.com>   wrote:
>>>
>>>> On 02/05/2010 09:27 AM, Jeff Layton wrote:
>>>>> When IPv6 is enabled, the Proto= config file option is treated as a
>>>>> netid, and the address family for lookups is selected based on that
>>>>> setting. The Defaultproto= option however still only affects the
>>>>> protocol setting for the sockets (IPPROTO_*) and not the address family.
>>>>>
>>>>> This patch makes it so that if someone sets the "Defaultproto=" option
>>>>> in the nfsmount.conf, it's used to determine the default address family
>>>>> for lookups as well as the protocol type.
>>>>>
>>>>> This gives users a way to force a particular address family to be used
>>>>> universally for mounts and brings the behavior of the Defaultproto=
>>>>> option in line with the Proto= option.
>>>>>
>>>>> Signed-off-by: Jeff Layton<jlayton@redhat.com>
>>>>> ---
>>>>>     utils/mount/configfile.c |    8 ++++++++
>>>>>     utils/mount/network.c    |   18 ++++++++----------
>>>>>     2 files changed, 16 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
>>>>> index 6843098..71d3627 100644
>>>>> --- a/utils/mount/configfile.c
>>>>> +++ b/utils/mount/configfile.c
>>>>> @@ -221,6 +221,8 @@ int inline check_vers(char *mopt, char *field)
>>>>>
>>>>>     unsigned long config_default_vers;
>>>>>     unsigned long config_default_proto;
>>>>> +extern sa_family_t config_default_family;
>>>>> +
>>>>>     /*
>>>>>      * Check to see if a default value is being set.
>>>>>      * If so, set the appropriate global value which will
>>>>> @@ -242,6 +244,12 @@ int inline default_value(char *mopt)
>>>>>     				xlog_warn("Unable to set default protocol : %s",
>>>>>     					strerror(errno));
>>>>>     			}
>>>>> +#ifdef IPV6_SUPPORTED
>>>>> +			if (!nfs_nfs_proto_family(options,&config_default_family)) {
>>>>> +				xlog_warn("Unable to set default family : %s",
>>>>> +					strerror(errno));
>>>>> +			}
>>>>> +#endif
>>>>
>>>> Maybe you don't need the #ifdef here?
>>>>
>>>> Aside from making the code more readable, removing the #ifdef would also
>>>> make it so the same code path is followed whether IPv6 is supported or
>>>> not, which makes testing easier.
>>>>
>>>> Just a thought.
>>>>
>>>
>>> They probably aren't essential. It all comes down to what we want the
>>> behavior to be in the following situation:
>>
>> OK... I had thought the behavior would be the same with and without the
>> #ifdef's.  Since it clearly doesn't work the same way in both cases,
>> that's a sure sign that we will have testing and documentation problems
>> here if the #ifdef is left in.
>>
>>> If someone had a TIRPC enabled nfs-utils but IPv6 support is disabled,
>>> and then set Defaultproto=tcp6.
>>>
>>> With the ifdef's in place, then the address family part of it would
>>> just be ignored. Without those in place, IPv6 will be forced for the
>>> lookup but we won't actually be able to make use of the addresses
>>> returned.
>>
>> As an admin (who understands how netids work), I would be more surprised
>> if setting the default protocol (netid, really) to tcp6, and then
>> disabling IPv6 on the client, or at build time in nfs-utils, or if the
>> IPv6-related netids were missing from /etc/netconfig, would work at all.
>>    It would be more consistent behaviour IMO to make this case fail.
>>
>> I agree with having things "just work" in reasonable cases, but it seems
>> like we are bending the semantics of netids and proto= to make that happen.
>>
>> "tcp6" does not mean "use IPv6 if it's available."  It means "always use
>> IPv6."  And, "tcp" does not mean "use TCP on either IPv4 or IPv6," it
>> now means "always use TCP on IPv4."  This is what we buy with netids and
>> TI-RPC.  That's why we use HAVE_LIBTIRPC rather than IPV6_SUPPORTED in
>> nfs_nfs_proto_family and nfs_mount_proto_family.
>>
>>> Granted, it's a bit of a pathological setup, but I don't think the
>>> #ifdef's here really take away anything and may help prevent confusion
>>> with the union of all these different options.
>>>
>>>>>     		} else {
>>>>>     			xlog_warn("Unable to alloc memory for default protocol");
>>>>>     		}
>>>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>>>> index 92bba2d..0ab3bb1 100644
>>>>> --- a/utils/mount/network.c
>>>>> +++ b/utils/mount/network.c
>>>>> @@ -1331,6 +1331,12 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
>>>>>     	return 1;
>>>>>     }
>>>>>
>>>>> +#ifdef IPV6_SUPPORTED
>>
>> By the above argument, this should be HAVE_LIBTIRPC.
>>
>
> I can sort of buy the argument for leaving out the earlier #ifdef
> around the default_value() function.
>
> If you change this one though, then lookups will still return IPv6
> addresses by default even when IPV6_SUPPORTED isn't set. IOW, in the
> absence of a proto= option, you'll pass AF_UNSPEC to getaddrinfo and
> get IPv6 addresses. I don't think that's what we want here for a
> non-ipv6 enabled nfs-utils.

OK, agreed.  This setting is actually not determining the meaning of any 
of the netids, but rather it is determining the default address family 
if _no_ netid is specified.

>>>>> +sa_family_t	config_default_family = AF_UNSPEC;
>>>>> +#else
>>>>> +sa_family_t	config_default_family = AF_INET;
>>>>> +#endif
>>>>> +
>>>>>     /*
>>>>>      * Returns TRUE and fills in @family if a valid NFS protocol option
>>>>>      * is found, or FALSE if the option was specified with an invalid value.
>>>>> @@ -1341,11 +1347,7 @@ int nfs_nfs_proto_family(struct mount_options *options,
>>>>>     	unsigned long protocol;
>>>>>     	char *option;
>>>>>
>>>>> -#ifdef IPV6_SUPPORTED
>>>>> -	*family = AF_UNSPEC;
>>>>> -#else
>>>>> -	*family = AF_INET;
>>>>> -#endif
>>>>> +	*family = config_default_family;
>>>>>
>>>>>     	switch (po_rightmost(options, nfs_transport_opttbl)) {
>>>>>     	case 0:	/* udp */
>>>>> @@ -1488,11 +1490,7 @@ int nfs_mount_proto_family(struct mount_options *options,
>>>>>     	unsigned long protocol;
>>>>>     	char *option;
>>>>>
>>>>> -#ifdef HAVE_LIBTIRPC
>>>>> -	*family = AF_UNSPEC;
>>>>> -#else
>>>>> -	*family = AF_INET;
>>>>> -#endif
>>>>> +	*family = config_default_family;
>>>>>
>>>>>     	option = po_get(options, "mountproto");
>>>>>     	if (option != NULL)
>>>>
>>>>
>>>
>>>
>>
>
>


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