On Fri, 05 Feb 2010 12:13:08 -0500
Chuck Lever <chuck.lever@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
