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

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

[Jeff Layton]

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
 		} 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
+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)
-- 
1.6.6

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

[Chuck Lever]

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.

>   		} 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
> +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

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

[Jeff Layton]

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:

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.

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
> > +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)
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

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

[Chuck Lever]

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.

>>> +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

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

[Jeff Layton]

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.


> >>> +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)
> >>
> >>
> >
> >
> 


-- 
Jeff Layton <jlayton@redhat.com>

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

[Chuck Lever]

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

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

[Jeff Layton]

On Fri, 05 Feb 2010 13:11:45 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> 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.
> 

Correct. Ok, I'll respin and repost with the ifdef's in default_value()
removed.

> >>>>> +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)
> >>>>
> >>>>
> >>>
> >>>
> >>
> >
> >
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

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

[Jeff Layton]

On Fri, 5 Feb 2010 13:17:57 -0500
Jeff Layton <jlayton@redhat.com> wrote:

> > > 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.
> > 
> 
> Correct. Ok, I'll respin and repost with the ifdef's in default_value()
> removed.

New patch sent. Just for giggles, I tested this by building a
tirpc-enabled/ipv6-disabled nfs-utils and setting Defaultproto=tcp6. It
does indeed set the address family to IPv6:

# mount -v opensolaris:/export/public /mnt/test
mount: no type was given - I'll assume nfs because of the colon
mount.nfs: timeout set for Fri Feb  5 14:51:01 2010
mount.nfs: trying text-based options 'vers=4,addr=feed::23,clientaddr=feed::22'

...and the mount succeeds.

So in that situation and also when proto=tcp6 is specified, someone can
mount an IPv6-capable server even with a nfs-utils that isn't built for
IPv6 as long as it has tirpc support.

Is that a good thing? I'm not sure -- statd won't work over IPv6 in
that case, for instance. Would it be better to have mount.nfs fail to
use IPv6 addrs at all when not built with IPv6 support?

-- 
Jeff Layton <jlayton@redhat.com>

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

[Chuck Lever]

On 02/05/2010 02:55 PM, Jeff Layton wrote:
> On Fri, 5 Feb 2010 13:17:57 -0500
> Jeff Layton<jlayton@redhat.com>  wrote:
>
>>>> 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.
>>>
>>
>> Correct. Ok, I'll respin and repost with the ifdef's in default_value()
>> removed.
>
> New patch sent. Just for giggles, I tested this by building a
> tirpc-enabled/ipv6-disabled nfs-utils and setting Defaultproto=tcp6. It
> does indeed set the address family to IPv6:
>
> # mount -v opensolaris:/export/public /mnt/test
> mount: no type was given - I'll assume nfs because of the colon
> mount.nfs: timeout set for Fri Feb  5 14:51:01 2010
> mount.nfs: trying text-based options 'vers=4,addr=feed::23,clientaddr=feed::22'
>
> ...and the mount succeeds.
>
> So in that situation and also when proto=tcp6 is specified, someone can
> mount an IPv6-capable server even with a nfs-utils that isn't built for
> IPv6 as long as it has tirpc support.
>
> Is that a good thing? I'm not sure -- statd won't work over IPv6 in
> that case, for instance. Would it be better to have mount.nfs fail to
> use IPv6 addrs at all when not built with IPv6 support?

If IPv6 support is disabled at build time, mount.nfs shouldn't allow a 
mount over IPv6, even if the kernel supports it.

Is that with, or without, my additional protocol family negotiation patches?

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

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

[Jeff Layton]

On Fri, 05 Feb 2010 14:59:30 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> On 02/05/2010 02:55 PM, Jeff Layton wrote:
> > On Fri, 5 Feb 2010 13:17:57 -0500
> > Jeff Layton<jlayton@redhat.com>  wrote:
> >
> >>>> 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.
> >>>
> >>
> >> Correct. Ok, I'll respin and repost with the ifdef's in default_value()
> >> removed.
> >
> > New patch sent. Just for giggles, I tested this by building a
> > tirpc-enabled/ipv6-disabled nfs-utils and setting Defaultproto=tcp6. It
> > does indeed set the address family to IPv6:
> >
> > # mount -v opensolaris:/export/public /mnt/test
> > mount: no type was given - I'll assume nfs because of the colon
> > mount.nfs: timeout set for Fri Feb  5 14:51:01 2010
> > mount.nfs: trying text-based options 'vers=4,addr=feed::23,clientaddr=feed::22'
> >
> > ...and the mount succeeds.
> >
> > So in that situation and also when proto=tcp6 is specified, someone can
> > mount an IPv6-capable server even with a nfs-utils that isn't built for
> > IPv6 as long as it has tirpc support.
> >
> > Is that a good thing? I'm not sure -- statd won't work over IPv6 in
> > that case, for instance. Would it be better to have mount.nfs fail to
> > use IPv6 addrs at all when not built with IPv6 support?
> 
> If IPv6 support is disabled at build time, mount.nfs shouldn't allow a 
> mount over IPv6, even if the kernel supports it.
> 
> Is that with, or without, my additional protocol family negotiation patches?
> 

With those patches. I haven't tested this without them.

-- 
Jeff Layton <jlayton@redhat.com>

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

[Chuck Lever]

On 02/05/2010 03:05 PM, Jeff Layton wrote:
> On Fri, 05 Feb 2010 14:59:30 -0500
> Chuck Lever<chuck.lever@oracle.com>  wrote:
>
>> On 02/05/2010 02:55 PM, Jeff Layton wrote:
>>> On Fri, 5 Feb 2010 13:17:57 -0500
>>> Jeff Layton<jlayton@redhat.com>   wrote:
>>>
>>>>>> 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.
>>>>>
>>>>
>>>> Correct. Ok, I'll respin and repost with the ifdef's in default_value()
>>>> removed.
>>>
>>> New patch sent. Just for giggles, I tested this by building a
>>> tirpc-enabled/ipv6-disabled nfs-utils and setting Defaultproto=tcp6. It
>>> does indeed set the address family to IPv6:
>>>
>>> # mount -v opensolaris:/export/public /mnt/test
>>> mount: no type was given - I'll assume nfs because of the colon
>>> mount.nfs: timeout set for Fri Feb  5 14:51:01 2010
>>> mount.nfs: trying text-based options 'vers=4,addr=feed::23,clientaddr=feed::22'
>>>
>>> ...and the mount succeeds.
>>>
>>> So in that situation and also when proto=tcp6 is specified, someone can
>>> mount an IPv6-capable server even with a nfs-utils that isn't built for
>>> IPv6 as long as it has tirpc support.
>>>
>>> Is that a good thing? I'm not sure -- statd won't work over IPv6 in
>>> that case, for instance. Would it be better to have mount.nfs fail to
>>> use IPv6 addrs at all when not built with IPv6 support?
>>
>> If IPv6 support is disabled at build time, mount.nfs shouldn't allow a
>> mount over IPv6, even if the kernel supports it.
>>
>> Is that with, or without, my additional protocol family negotiation patches?
>>
>
> With those patches. I haven't tested this without them.

Ah, I see... you are not talking about by default.

In that case, you probably want nfs_{nfs,mount}_proto_family to return 
FALSE if nfs_get_proto returns AF_INET6 and IPV6_SUPPORTED is not defined.

/me winces

That would be a separate fix from your config file patch.

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

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

[Jeff Layton]

On Fri, 05 Feb 2010 15:25:24 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> On 02/05/2010 03:05 PM, Jeff Layton wrote:
> > On Fri, 05 Feb 2010 14:59:30 -0500
> > Chuck Lever<chuck.lever@oracle.com>  wrote:
> >
> >> On 02/05/2010 02:55 PM, Jeff Layton wrote:
> >>> On Fri, 5 Feb 2010 13:17:57 -0500
> >>> Jeff Layton<jlayton@redhat.com>   wrote:
> >>>
> >>>>>> 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.
> >>>>>
> >>>>
> >>>> Correct. Ok, I'll respin and repost with the ifdef's in default_value()
> >>>> removed.
> >>>
> >>> New patch sent. Just for giggles, I tested this by building a
> >>> tirpc-enabled/ipv6-disabled nfs-utils and setting Defaultproto=tcp6. It
> >>> does indeed set the address family to IPv6:
> >>>
> >>> # mount -v opensolaris:/export/public /mnt/test
> >>> mount: no type was given - I'll assume nfs because of the colon
> >>> mount.nfs: timeout set for Fri Feb  5 14:51:01 2010
> >>> mount.nfs: trying text-based options 'vers=4,addr=feed::23,clientaddr=feed::22'
> >>>
> >>> ...and the mount succeeds.
> >>>
> >>> So in that situation and also when proto=tcp6 is specified, someone can
> >>> mount an IPv6-capable server even with a nfs-utils that isn't built for
> >>> IPv6 as long as it has tirpc support.
> >>>
> >>> Is that a good thing? I'm not sure -- statd won't work over IPv6 in
> >>> that case, for instance. Would it be better to have mount.nfs fail to
> >>> use IPv6 addrs at all when not built with IPv6 support?
> >>
> >> If IPv6 support is disabled at build time, mount.nfs shouldn't allow a
> >> mount over IPv6, even if the kernel supports it.
> >>
> >> Is that with, or without, my additional protocol family negotiation patches?
> >>
> >
> > With those patches. I haven't tested this without them.
> 
> Ah, I see... you are not talking about by default.
> 
> In that case, you probably want nfs_{nfs,mount}_proto_family to return 
> FALSE if nfs_get_proto returns AF_INET6 and IPV6_SUPPORTED is not defined.
> 
> /me winces
> 
> That would be a separate fix from your config file patch.
> 

If we do that, then in the hypothetical scenario above (someone
specifies Defaultproto=tcp6 in the config file and has a
tirpc-enabled/ipv6-disabled nfs-utils), then their mounts will always
fail unless they override that setting with a proto= option.

Is that the behavior we want here?

(/me struggles not to get lost in the twisty maze of options)
-- 
Jeff Layton <jlayton@redhat.com>

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

[Chuck Lever]

On 02/05/2010 04:20 PM, Jeff Layton wrote:
> On Fri, 05 Feb 2010 15:25:24 -0500
> Chuck Lever<chuck.lever@oracle.com>  wrote:
>
>> On 02/05/2010 03:05 PM, Jeff Layton wrote:
>>> On Fri, 05 Feb 2010 14:59:30 -0500
>>> Chuck Lever<chuck.lever@oracle.com>   wrote:
>>>
>>>> On 02/05/2010 02:55 PM, Jeff Layton wrote:
>>>>> On Fri, 5 Feb 2010 13:17:57 -0500
>>>>> Jeff Layton<jlayton@redhat.com>    wrote:
>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>
>>>>>> Correct. Ok, I'll respin and repost with the ifdef's in default_value()
>>>>>> removed.
>>>>>
>>>>> New patch sent. Just for giggles, I tested this by building a
>>>>> tirpc-enabled/ipv6-disabled nfs-utils and setting Defaultproto=tcp6. It
>>>>> does indeed set the address family to IPv6:
>>>>>
>>>>> # mount -v opensolaris:/export/public /mnt/test
>>>>> mount: no type was given - I'll assume nfs because of the colon
>>>>> mount.nfs: timeout set for Fri Feb  5 14:51:01 2010
>>>>> mount.nfs: trying text-based options 'vers=4,addr=feed::23,clientaddr=feed::22'
>>>>>
>>>>> ...and the mount succeeds.
>>>>>
>>>>> So in that situation and also when proto=tcp6 is specified, someone can
>>>>> mount an IPv6-capable server even with a nfs-utils that isn't built for
>>>>> IPv6 as long as it has tirpc support.
>>>>>
>>>>> Is that a good thing? I'm not sure -- statd won't work over IPv6 in
>>>>> that case, for instance. Would it be better to have mount.nfs fail to
>>>>> use IPv6 addrs at all when not built with IPv6 support?
>>>>
>>>> If IPv6 support is disabled at build time, mount.nfs shouldn't allow a
>>>> mount over IPv6, even if the kernel supports it.
>>>>
>>>> Is that with, or without, my additional protocol family negotiation patches?
>>>>
>>>
>>> With those patches. I haven't tested this without them.
>>
>> Ah, I see... you are not talking about by default.
>>
>> In that case, you probably want nfs_{nfs,mount}_proto_family to return
>> FALSE if nfs_get_proto returns AF_INET6 and IPV6_SUPPORTED is not defined.
>>
>> /me winces
>>
>> That would be a separate fix from your config file patch.
>>
>
> If we do that, then in the hypothetical scenario above (someone
> specifies Defaultproto=tcp6 in the config file and has a
> tirpc-enabled/ipv6-disabled nfs-utils), then their mounts will always
> fail unless they override that setting with a proto= option.
>
> Is that the behavior we want here?

Yes.  If IPv6 support is disabled, then any usage of "tcp6" or "udp6" 
with mount.nfs should cause an error.

If not, how do we resolve the case where mount.nfs uses IPv6 but the 
rest of nfs-utils doesn't have IPv6 support enabled?

> (/me struggles not to get lost in the twisty maze of options)

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