nfs-utils-1.2.1 released.

[Steve Dickson]

New Features in this release:

* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

* The mount options v4, nfsvers=4 and vers=4 are added.

* With the --enable-mountconfig set during configuration time
a mount configuration file can be used to set mount options
per mount point, per server or globally. See nfsmount.conf(5)


The tarball can be found at:

http://www.kernel.org/pub/linux/utils/nfs/
http://sourceforge.net/projects/nfs

The git tree is at:
git://linux-nfs.org/nfs-utils

The change log:

commit 67e6d2d8a96e98d7f21693a9d034da81d3f90084
Author: Steve Dickson <steved at redhat.com>
Date: Wed Nov 4 06:13:56 2009 -0500

Release 1.2.1

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 5df68929ceef7742f7669ba5925ea31b7b5a699c
Author: Steve Dickson <steved at redhat.com>
Date: Tue Nov 3 15:11:09 2009 -0500

Fixed configuration error when --disable-mount was used.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 97731f394c6b83ed7e5c3923278bbe98ee130bee
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Nov 3 11:19:08 2009 -0500

mount: Fix po_join() call site in nfs_try_mount_v4()

Make sure the copied options string is freed in case po_join() fails.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit e4328bb8d13ae6dda33308557e6bbb352d5674bb
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Nov 3 11:16:30 2009 -0500

mount.nfs: Assume v2/v3 if mount-related options are present

Don't try NFSv4 if any MNT protocol related options were presented by
the user.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 57af0cce3ed6f555bb40132d667e8b10d41855ca
Author: Steve Dickson <steved at redhat.com>
Date: Tue Nov 3 10:37:36 2009 -0500

Made some aesthetic changes to the code that sets
the defaults that were a result of the code review.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit e1eccc5fc317f73801522b3b498c6dab67b048d2
Author: Steve Dickson <steved at redhat.com>
Date: Tue Nov 3 09:49:03 2009 -0500

Retry v4 mounts with a v3 mount when the version
is not explicitly specified and the mount fails
with ENOENT. The will help deal with Linux servers
that do not automatically export a pseudo root

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1af166179c3f28fa9943f7844e03032f3bdae7ea
Author: Steve Dickson <steved at redhat.com>
Date: Tue Oct 27 15:47:27 2009 -0400

Added wrappers around the setting of default values
from the config file which will be compiled out
when the config file is not enabled.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 37122505cab9a3370e06a147efde36262371e664
Author: Steve Dickson <steved at redhat.com>
Date: Thu Oct 22 15:35:54 2009 -0400

Added the defaultproto and defaultvers variable to the mount
configuration file.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit a8b90e8f64a7d53b20c0980f2a6d9a8d5945bcc4
Author: Steve Dickson <steved at redhat.com>
Date: Sat Oct 17 09:26:18 2009 -0400

Use the default protocol and version values, when they
are set in the configuration file, to start the negation
with the server

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 2f61f62ac777cc03e30513f6fd3699f9e2f04e27
Author: Steve Dickson <steved at redhat.com>
Date: Sat Oct 17 09:16:18 2009 -0400

Introducing the parsing of both 'defaultvers' and 'defaultproto'
config variables which will be used to set the the default
version and network protocol.

A global variable will be set for each option with the
corresponding value. The value will be used as the
initial value in the server negation.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit f87ae8235ae6042c0e514ba03e4eee7782d5bc6e
Author: Steve Dickson <steved at redhat.com>
Date: Fri Oct 9 13:22:27 2009 -0400

Make sure all protocol version options are checked in check_vers()

Signed-off-by: Steve Dickson <steved at redhat.com>

commit f312676ca61f826efa5d0eb39433c5aa075711ab
Author: Steve Dickson <steved at redhat.com>
Date: Fri Oct 9 13:14:52 2009 -0400

Make the network transports value in the mount
config file case sensitive, since they are in the
mount command's parsing code.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 2091bc67253d6dc1f68dba2218593701bfc729c0
Author: Steve Dickson <steved at redhat.com>
Date: Fri Oct 9 09:19:39 2009 -0400

There are a number of different mount options that can be
used to set the protocol version on the command line. The
config file code needs to know about each option so the
command line value will override the config file value.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 0e0526cce8127f1c18063ff700f5e4d5c77dc108
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Sep 29 10:38:52 2009 -0400

mount: Support negotiation between v4, v3, and v2

When negotiating between v3 and v2, mount.nfs first tries v3, then v2.
Take the same approach for v4: try v4 first, then v3, then v2, in
order to get the highest NFS version both the client and server
support.

No MNT request is needed for v4. Since we want to avoid an rpcbind
query for the v4 attempt, just go straight for mount(2) without a MNT
request or rpcbind negotiation first. If the server reports that v4
is not supported, try lower versions.

The decisions made by the fg/bg retry loop have nothing to do with
version negotation. To avoid a layering violation, mount.nfs's
multi-version negotiation strategy is wholly encapsulated within
nfs_try_mount(). Thus, code duplication between nfsmount_fg(),
nfsmount_parent(), and nfsmount_child() is avoided.

For now, negotiating version 4 is supported only on kernels that can
handle the vers=4 option on type "nfs" file systems. At some point
we could also allow mount.nfs to switch to an "nfs4" file system in
this case.

Since mi->version == 0 can now mean v2, v3, or v4, limit the versions
tried for RDMA mounts. Today, only version 3 supports RDMA.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 0de3189d32a183f5e91a6d27a9e8f159216d9473
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Sep 29 10:38:05 2009 -0400

The user's mount options and the set of versions to try should not
change over the course of mount retries.

With this patch, each version-specific mount attempt is compartment-
alized, and starts from the user's original mount options each time.
Thus these attempts can now be safely performed in any order,
depending on what the user has requested, what the server advertises,
and what is up and running at any given point.

Don't regress the fix in commit 23c1a452. For v2/v3 negotation, only
the user's mount options are written to /etc/mtab, and not any options
that were negotiated by mount.nfs. There's no way to guarantee that
the server configuration will be the same at umount time as it was at
mount time.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 3faa54e8a17411de28c7b707207d49b4466ae873
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Sep 29 10:37:12 2009 -0400

mount.nfs: Keep server's address in nfsmount_info

We want to pass the server's address around. Put it in the mount
context structure.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 88c4f664f869d7450b84f0262fd87fbda4f1f61b
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Sep 29 10:36:19 2009 -0400

mount.nfs: Add API to duplicate a mount option list

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 5f06c2b5e18990a2d62987ea06bdf5afb8306a2d
Author: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Date: Tue Sep 15 14:42:47 2009 -0400

nfs-utils: nfs-iostat.py autofs cleanup and option to sort by ops/s

Adds --sort option to display mount point stats sorted by ops/s
Adds --list=<n> option to only display stats for first <n> mount points
E.g. the use of "--sort --list=1" should be useful in seeing stats for
only the mountpoint with the highest ops/s.

Signed-off-by: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit c3203ff9a940e1e2270e06673eca77066aabd77c
Author: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Date: Tue Sep 15 14:41:46 2009 -0400

nfs-utils: nfs-iostat.py autofs cleanup and option to sort by ops/s

Introduce optparse for managing command usage/help and the statistics
options. This change helps more cleanly add new options such as --sort
while preserving the iostat-like interval, count, and mount point
positional arguments.

Signed-off-by: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 88deba4a8db06d371659aa85b668460b85900d48
Author: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Date: Tue Sep 15 14:31:35 2009 -0400

nfs-utils: nfs-iostat.py autofs cleanup and option to sort by ops/s

Update list of mount points at each interval and check for differences
when producing comparative stats. This ensures proper stats collection
for autofs mountpoints.

Signed-off-by: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 8e0ca8f2b6152efe58fbf78648bb889ab76a3d43
Author: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Date: Tue Sep 15 14:30:50 2009 -0400

nfs-utils: nfs-iostat.py autofs cleanup and option to sort by ops/s

Conforms Python path to the LSB 3.2+ standard of /usr/bin/python
http://refspecs.freestandards.org/LSB_3.2.0/LSB-Languages/LSB-Languages/pylocation.html
Per SteveD this is also required for proper rpm dep resolution during
builds

Signed-off-by: Lans Carstensen <Lans.Carstensen at dreamworks.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit b25c022e58c53cedfd6bd9592f082364d002821b
Author: Jeff Layton <jlayton at redhat.com>
Date: Mon Sep 14 14:06:53 2009 -0400

idmapd: rearm event handler after error in nfsdcb()

A couple of years ago, Bruce committed a patch to make knfsd send
unsigned uid's and gid's to idmapd, rather than signed values. Part
of that earlier discussion is here:

http://linux-nfs.org/pipermail/nfsv4/2007-December/007321.html

While this fixed the immediate problem, it doesn't appear that anything
was ever done to make idmapd continue working when it gets a bogus
upcall.

idmapd uses libevent for its main event handling loop. When idmapd gets
an upcall from knfsd it will service the request and then rearm the
event by calling event_add on the event structure again.

When it hits an error though, it returns in most cases w/o rearming the
event. That prevents idmapd from servicing any further requests from
knfsd.

I've made another change too. If an error is encountered while reading
the channel file, this patch has it close and reopen the file prior to
rearming the event.

I've not been able to test this patch directly, but I have tested a
backport of it to earlier idmapd code and verified that it did prevent
idmapd from hanging when it got a badly formatted upcall from knfsd.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 74badf6f30f7aea95e9d784244488084dbadcb55
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Mon Sep 14 13:55:36 2009 -0400

mount.nfs: Support "-t nfs,vers=4" mounts in the kernel
Support "vers=4" in nfs_nfs_version()

Skip UMNT call for "-t nfs -o vers=4" mounts

For "-t nfs -o vers=4" mounts, we want to skip v2/v3
version/transport negotiation, but be sure to append
the "clientaddr" option.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>
Tested-by: Steve Dickson <steved at redhat.com>

commit b2a3cd590442309d40e9dd6d43213445df250694
Author: Jeff Layton <jlayton at redhat.com>
Date: Mon Sep 14 13:24:00 2009 -0400

IPv6 support for nfsd was finished before some of the other daemons
(mountd and statd in particular). That could be a problem in the future
if someone were to boot a kernel that supports IPv6 serving with an
older nfs-utils. For now, hardcode the IPv6 switch into the off position
until the other daemons are functional.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit bd947185cfc7978c562fddf2f14f602c44a5cac9
Author: Lukas Hejtmanek <xhejtman at ics.muni.cz>
Date: Thu Aug 27 11:42:24 2009 -0400

Gssd blindly caches machine credentials

We have a problem with rpc.gssd which blindly caches machine credentials.
E.g., if someone deletes /tmp/krb5cc_machine_REALM, rpc.gss does not create
new one until the old one expires. Also, it has problems with clock skew, if
time goes back and gssd thinks that machine credentials are not expired yet.

The following patch tries to use cache but in case of failure, it tries it
again without cache. Any comments?

Signed-off-by: Lukas Hejtmanek <xhejtman at ics.muni.cz>
Acked-by: Kevin Coffman <kwc at citi.umich.edu>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1d2951b518dd5df4fc0a637880d204f0f4e949c5
Author: Steve Dickson <steved at redhat.com>
Date: Thu Aug 27 11:31:08 2009 -0400

Cleaned up some warnings in the mount config file code.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 9999c036952eced407162c394ea69145752ea5c4
Author: J. Bruce Fields <bfields at citi.umich.edu>
Date: Mon Aug 24 08:20:10 2009 -0400

Don't give client an empty flavor list

In the absence of an explicit sec= option on an export, rpc.mountd
is returning a zero-length flavor list to clients in the MOUNT results.

The linux client doesn't seem to mind, but the Solaris client
(reasonably enough) is giving up; the symptom is a "security mode
does not match" error on mount.

We could modify the export-parsing code to ensure the secinfo array
is nonzero. But I think it's slightly simpler to handle this default
case in the implementation of the MOUNT call. This is more-or-less the
same thing the kernel does when mountd passes it an export without any
security flavors specified.

Thanks to Tom Haynes for bug report and diagnosis.

Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit fd51c0c39017f44ceec4229f86eaa7c8e193ebdc
Author: Steve Dickson <steved at redhat.com>
Date: Mon Aug 17 08:50:04 2009 -0400

Cleaned up parsing errors to hopeful be more precise

Also had mount_config_init() call xlog_open() so
the program name is set on xlog() calls.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 4d0175ad400ec56456765a15829557f1d541866a
Author: Benny Halevy <bhalevy at panasas.com>
Date: Mon Aug 17 07:12:03 2009 -0400

Added support for line comments parsing which should
help with readability with in the configuration file.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 8414d150cee62ba0554cfd645956a88dba02a7eb
Author: Steve Dickson <steved at redhat.com>
Date: Fri Aug 7 14:34:42 2009 -0400

Now that only the Section names are case-insensitive
the mount code has to make sure the the mount options
given to the kernel are in the correct case.

Fixed a couple of warnings on #ifndefs

Signed-off-by: Steve Dickson <steved at redhat.com>

commit a0caba9887474cdc9b9ba8b113a869ff7954ab84
Author: Steve Dickson <steved at redhat.com>
Date: Wed Aug 5 17:07:21 2009 -0400

The example nfsmount.conf file

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 4d18845fee7c6f5fc3d987fd9d3b5c5215c9e68b
Author: Steve Dickson <steved at redhat.com>
Date: Sun Aug 16 17:05:56 2009 -0400

The new nfsmount.conf(5) man page and the update to
the nfs(5) man page

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 9082582d6675e45067838805a65b6fcc07164557
Author: Steve Dickson <steved at redhat.com>
Date: Fri Aug 7 14:29:07 2009 -0400

Added hooks to the mount command that allow
mount options to be set in a configuration file

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 0cdb36e69a51eabc119de314e43d40daf6ee49ab
Author: Steve Dickson <steved at redhat.com>
Date: Wed Aug 5 16:17:38 2009 -0400

Support routines used to read sections from the configuration file
and parse them into comma separated mount options.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1d1f701656f86c54b07e7fd9481d1f45018cfddb
Author: Steve Dickson <steved at redhat.com>
Date: Wed Aug 5 16:10:01 2009 -0400

Adds '--enable-mountconfig' configuration flag that will
enabled mount to read from a configuration file.
The default value is disabled (or no)

Adds '--with-mountfile' configuration flag that is used when
mountconf is enabled to define the configuration file name.
The default is /etc/nfsmount.conf.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 9350a97a266ada8d8b3282cf4248e3b9ffdc0058
Author: Steve Dickson <steved at redhat.com>
Date: Wed Aug 5 16:02:33 2009 -0400

Added an conditional argument to the Section names
with the format being:
[ Section <"argument"> ]
This will help group similar functioning Section
together. The argument is conditional but must be
surrounded by the '"' characters.

The new conf_get_section() interface can used
to locate a Section by its Section name and/or
argument.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit c6a270ea8ab6ad299e6a43445420f22e0c617e3e
Author: Steve Dickson <steved at redhat.com>
Date: Wed Aug 5 15:53:36 2009 -0400

Make Section names case-insensitive which should
help in locating them resulting in make the config
files a bit less error prone

Signed-off-by: Steve Dickson <steved at redhat.com>

commit a61e7ab6dbf7e3d6ad4e6377c96748dfaf353542
Author: Steve Dickson <steved at redhat.com>
Date: Mon Mar 9 13:55:25 2009 -0400

Taught conf_parse_line() to ignore spaces in the
'[section]' parsing and before the assignment statements

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1a9010602442f466d700fbc4d64fe82ac69b1dd3
Author: Steve Dickson <steved at redhat.com>
Date: Wed Aug 5 15:47:05 2009 -0400

Move idmapd's configuration file parsing routines into
the shared libnfs.a library, making them available to\
other daemons and programs.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit b7a68e47bb869f3c4895a176018c4ee054214fc2
Author: Benny Halevy <bhalevy at panasas.com>
Date: Sun Aug 16 16:39:07 2009 -0400

Augment nfs4 stats to cover new nfs41 client and
server operations' stats.

Signed-off-by: Benny Halevy <bhalevy at panasas.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit cd6d0c9258ed96f4e7721987061969394426810d
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Sun Aug 16 16:31:31 2009 -0400

nfs(5): Remove trailing blanks

Clean up: eliminate trailing blanks in utils/mount/nfs.man.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1b5aeac23eea499db3bdc02857070f12e258609c
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Sun Aug 16 16:23:36 2009 -0400

nfs(5): Add description of lookupcache mount option

See kernel commit 7973c1f1.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b
Author: J. Bruce Fields <bfields at citi.umich.edu>
Date: Sun Aug 16 16:18:17 2009 -0400

Since

2d77e3a27b7b211f303f.. "Fix bug when both crossmnt and fsid are set"

Subexports automatically created by "crossmnt" get the NFSEXP_FSID flag
cleared. That flag should also be cleared in the
security-flavor-specific flag fields. Otherwise the kernel detects the
inconsistent flags and rejects the export.

The symptoms are clients hanging the first time they export a filesystem
mounted under a filesystem that was exported with something like:

/exports *(crossmnt,fsid=0,sec=krb5)

Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit ceb090c16e3a9f0509b3eb5e703c91a89d5a25f0
Author: Jeff Layton <jlayton at redhat.com>
Date: Fri Aug 14 13:42:22 2009 -0400

Add some clarification about the purpose of the program, info about the
--debug and --syslog options, and a note about how it behaves when
TI-RPC support is built in.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 542b5ba9e68a6bbabe9b29b5d29b5b793b3e8d4f
Author: Jeff Layton <jlayton at redhat.com>
Date: Fri Aug 14 13:37:12 2009 -0400

nfs-utils: add IPv6 support to nfsd

Add support for handing off IPv6 sockets to the kernel for nfsd. One of
the main goals here is to not change the behavior of options and not to
add any new ones, so this patch attempts to do that.

We also don't want to break anything in the event that someone has an
rpc.nfsd program built with IPv6 capability, but the knfsd doesn't
support IPv6. Ditto for the cases where IPv6 is either not compiled in
or is compiled in and blacklisted.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 513acad321c325207a9d8f7f0129253b0e2b6d11
Author: Jeff Layton <jlayton at redhat.com>
Date: Fri Aug 14 13:33:02 2009 -0400

Allow nfssvc_setfds to properly deal with AF_INET6.

IPv6 sockets for knfsd can't be allowed to accept IPv4 packets. Set the
correct option to prevent that from occurring on IPv6 sockets.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 094b26031a376050d8610d055eb01c7949ad6547
Author: Jeff Layton <jlayton at redhat.com>
Date: Fri Aug 14 13:30:04 2009 -0400

nfs-utils: convert nfssvc_setfds to use getaddrinfo

Convert nfssvc_setfds to use getaddrinfo. Change the args that it takes
and fix up nfssvc function to pass in the proper args. The things that
nfssvc has to do to call the new nfssvc_setfds is a little cumbersome
for now, but that will eventually be cleaned up in a later patch.

nfs-utils: break up the nfssvc interface

Currently, the only public interface to the routines in nfssvc.c is
nfssvc(). This means that we do an awful lot of work after closing
stderr that could be done while it's still available.

Add prototypes to the header so that more functions in nfssvc.c can be
called individually, and change the nfsd program to call those routines
individually.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 4c477855cd025a18ac9decaf1bc9002aaae75689
Author: Jeff Layton <jlayton at redhat.com>
Date: Sat Aug 1 07:31:36 2009 -0400

nfs-utils: move check for active knfsd to helper function

nfssvc_setfds checks to see if knfsd is already running. Move this
check to a helper function. Eventually the nfsd code will call this
directly.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 775dea70ccc7556ac613def7896b3d3c1ff85ab5
Author: Jeff Layton <jlayton at redhat.com>
Date: Sat Aug 1 07:21:26 2009 -0400

nfs-utils: declare a static common buffer for nfssvc.c routines

Several of the routines in nfssvc.c declare a buffer for strings. Use a
shared static buffer instead to keep it off of the stack. Also, the
buffer allocated in some places is *really* large. BUFSIZ is generally
8k. These routines don't need nearly that much.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 0d53a1d0ff5635d3af2f0d10e1f00f5de1353490
Author: Jeff Layton <jlayton at redhat.com>
Date: Sat Aug 1 07:20:38 2009 -0400

nfs-utils: clean up NFSCTL_* macros for handling protocol bits

They are a little hard to follow currently. Clean them up and add new
macros that can set these bits in addition to the ones that unset them.

Also add a new macro that reports when any valid protocol bit is set.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 6f25394cb5651e7e44cc3fc0b2b4b2ccba8c3625
Author: Jeff Layton <jlayton at redhat.com>
Date: Sat Aug 1 06:27:40 2009 -0400

nfs-utils: convert rpc.nfsd to use xlog()

...and add --debug and --syslog options.

With the switch to xlog(), it becomes trivial to add debug messages, so
add an option to turn them on when requested.

Also, rpc.nfsd isn't a proper daemon per-se, so it makes more sense to
log errors to stderr where possible. Usually init scripts take care of
redirecting stderr output to syslog anyway.

For those that don't, add a --syslog option that forces all output to go
to syslog instead. Note that even with this option, errors encountered
during option processing will still go to stderr.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 5d37055061e92df07c4bf483ce06551d82ae9338
Author: Jeff Layton <jlayton at redhat.com>
Date: Sat Aug 1 06:26:40 2009 -0400

nfs-utils: clean up option parsing in nfsd.c

Minor formatting nits.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit f8dd0b70ecf5a02eda29af4acead86f3359c3081
Author: Jeff Layton <jlayton at redhat.com>
Date: Sat Aug 1 06:26:15 2009 -0400

nfs-utils: move nfssvc.c to nfsd dir and clean up linking of nfsd

rpc.nfsd is the only user of nfssvc.c, so we might as well move it
out of libnfs.a.

Also, don't link in libexport.a and libmisc.a, they aren't needed.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 3339c1f73c05fc9b5cf51e14a2033ec38f671334
Author: Steve Dickson <steved at redhat.com>
Date: Wed Jul 15 07:58:22 2009 -0400

errno not be set on RPC errors

Changed both nfs_advise_umount() and nfs_gp_ping() to
set the errno by calling CLNT_GETERR() after a CLNT_CALL()
error. Also added code to rpc_strerror() that will log
the errno value, when set, via strerror().

These changes added essential information to the error message
making it much easier to detect errorsuch as "Connection refused"

Signed-off-by: Steve Dickson <steved at redhat.com>

commit b46dc42505da799a05a3a5e8f004b308f6b9eef7
Author: Steve Dickson <steved at redhat.com>
Date: Wed Jul 15 06:21:54 2009 -0400

Don't use initialized garbage for address lengths

Make sure address lengths are initialized before
call calling nfs_extract_server_addresses() from
nfs_rewrite_pmap_mount_options(). Otherwise the
length check in nfs_string_to_sockaddr() can fail
since its will be using garbage from the stack.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit 906f0b27730b0506a24ed43029983b4d6819dd12
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 17:00:47 2009 -0400

mount.nfs: Squelch compiler warnings in nfs_strerror()

Address compiler warnings:

error.c: In function nfs_strerror:
error.c:341: warning: comparison between signed and unsigned
error.c:342: warning: comparison between signed and unsigned

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit d976ec69f0df3f19a3be9351f07086de54af42b9
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:59:31 2009 -0400

mount.nfs: Squelch unused parameter warnings on empty functions

Address compiler warnings:

fstab.c:288: warning: unused parameter sig

parse_dev.c:186: warning: unused parameter dev
parse_dev.c:187: warning: unused parameter hostname
parse_dev.c:187: warning: unused parameter pathname

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit ae548c2d55e442c96ad51e90c1e00ce3eb0b718b
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:58:38 2009 -0400

mount.nfs: Fix compiler warning in stropts.c

Address compiler warning:

stropts.c: In function ?nfs_append_generic_address_option?:
stropts.c:138: warning: comparison between signed and unsigned

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 6e3fa0e103a494a37cf3a4f9199516923928ab31
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:57:07 2009 -0400

umount.nfs: Use correct data type in nfsumount()

Address compiler warning:

nfsumount.c: In function nfsumount:
nfsumount.c:347: warning: comparison between signed and unsigned

The result type of pointer arithmetic and the return type of strlen(3)
are both size_t.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit c27bf85f0bd41015352468f35dfbf0d431d1e4c5
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:56:01 2009 -0400

mount.nfs: remove unused @addrlen argument from nfs_string_to_sockaddr()

Address compiler warning:

network.c: In function nfs_string_to_sockaddr:
network.c:272: warning: unused parameter addrlen

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit c51c20dfa8a81a5d512defcbbf1b7adec3adc591
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:53:29 2009 -0400

mount.nfs: Remove unused @salen parameter from nfs_ca_gai()

Address compiler warning:

network.c:1124: warning: unused parameter salen

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1767d5f5c9c26aa33b602a33ece83c2bfe55259f
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:51:27 2009 -0400

mount.nfs: Fix some nfs_error() nits in network.c

Fix a couple of nfs_error() call sites in utils/mount/network.c.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit d47f9baba71e92730e94d3361fa1098dc31b7627
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:50:00 2009 -0400

mount.nfs: Remove unused parameter in try_mount()

Address compiler warning:

mount.c: At top level:
mount.c:420: warning: unused parameter nomtab

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 4fc8cacd748d59dd3f463148994700bdd7610908
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:48:50 2009 -0400

mount.nfs: Use correct data type in discover_nfs_mount_data_version()

Address compiler warning:

mount.c: In function discover_nfs_mount_data_version?:
mount.c:162: warning: comparison between signed and unsigned
mount.c:164: warning: comparison between signed and unsigned
mount.c:166: warning: comparison between signed and unsigned
mount.c:168: warning: comparison between signed and unsigned
mount.c:170: warning: comparison between signed and unsigned
mount.c:178: warning: comparison between signed and unsigned

linux_version_code() and MAKE_VERSION() both return an unsigned int.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 5600327322a78a3a803368c0fe4f923cf14a4cf7
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:47:09 2009 -0400

support: Introduce sockaddr helpers to get and set IP port numbers

Introduce address family-agnostic functions that get and set IP port
numbers in socket addresses. We can already replace a few similar
functions in the mount command, and a few more will come up with
statd and sm-notify.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 23c1a452afce5726cfe661a4d7ac14a1883ecb55
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:45:07 2009 -0400

mount.nfs: Don't update extra_opts after text-based negotiation

The umount.nfs command will negotiate the mount options again, so all
that is needed in /etc/mnttab is the original set of options used for
the mount, plus the additional mandatory options like addr=''.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 134ea8cb64885fb587d038f80a1924f4c26470b9
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:39:17 2009 -0400

mount.nfs: Clean up after restructuring version/protocol negotiation

Fix up comments and function names to reflect the new version/protocol
negotiation scheme. We can now remove a bunch of mount processing
that is specific to v2/v3, removing about 100 lines of logic from
stropts.c.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 987ee0e340a413c3bf3187084df9834eefb37bb9
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:37:02 2009 -0400

mount.nfs: Clean up nfs_is_permanent_error()

Clean up: Move nfs_is_permanent_error() closer to the functions that
call it, and update a documenting comment to reflect recent
restructuring in this area.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit ca4e817b083553b6244e81ac91d9db86c09948b1
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:35:26 2009 -0400

mount.nfs: rearchitect mount version/protocol negotiation logic

Text-based mounts try a mount operation first with default settings,
then negotiate via rpcbind queries and retry the mount, if the default
settings don't work. This method introduces long delays in certain
common scenarios, and makes it difficult to tell when it is
appropriate to fail immediately or negotiate and retry.

To address these behavioral regressions, make text-based mounts
operate the same way that legacy mounts work. Perform rpcbind queries
with short timeouts first, then use the results to determine
transport, version, and port number settings for the mount.

This allows the mount.nfs command to detect server settings, or
whether negotiation is even possible, quickly. It also makes it
simple to determine when to fail vs. when to retry.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 19ba81f64447dca205362a119f1e72701438aecc
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:34:20 2009 -0400

mount.nfs: make nfs_options2pmap return errors

Up until now, nfs_options2pmap() has been passed mount options that
have already gone through the kernel's parser successfully. So, it
never had to check for invalid mount option values.

However, we are about to pass it options that come right from the
user. So nfs_options2pmap() will now need to report an error and
fail if it encounters a bogus value for any of the options it cares
about.

=====

Note that nfs_options2pmap() will allow a bogus value for an option
if the same option is specified farther to the right with a useable
value.

For example, if a user specifies "proto=foo,...,tcp" then
nfs_options2pmap() uses "tcp" and ignores "proto=foo".

However, if the options are specified in the other order:
"tcp,...,proto=foo" then nfs_options2pmap() will fail. This is a simple
and unambiguous extension of the "rightmost wins" rule.

Since mount.nfs strips out these options out and replaces them with
the rpcbind-negotiated options before invoking mount(2), the kernel
should never receive bogus values for these options from mount.nfs in
such cases.

This is probably slightly more flexible behavior than the legacy
mount implementation, but should be harmless. All mount options
unrelated to pmap are ignored by nfs_options2pmap().

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 3fc80f43ffb272036188d070a01090bbf243b7b5
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:31:15 2009 -0400

mount.nfs: force rpcbind queries if options aren't specified

nfs_options2pmap() fills in default values if the passed-in mount
options don't specify values. This short-circuits the version, port,
and transport negotiation logic in nfs_probe_bothports().

Instead, nfs_options2pmap() should plant zeros in these pmap fields
to force nfs_probe_bothports() and nfs_advise_mount() to discover, via
rpcbind queries, what the server supports.

This fixes some scenarios where umount.nfs fails to connect to servers
that don't have all rpcbind ports open, in addition to fixing other
corner cases during mount.nfs version/protocol negotiation.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 5f7313a10b63da796d5cd79c01db1e097d4a6bf1
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:29:11 2009 -0400

mount.nfs: If port= specifies an unregistered port, retry, then fail

Suppose a port= option is specified on the mount command line, but not
enough other mount options are specified to avoid an rpcbind query to
discover the NFS service.

If the NFS service isn't registered on [100003, 3, "tcp", port] (even
if the server is listening on the specified port), the legacy mount.nfs
command fails immediately with:

mount.nfs: mount to NFS server 'server' failed: RPC Error: Success

What's more, this mount request should succeeded if an NFS service is
registered on the specified port for another version and/or protocol.

So instead, let's retry the rpcbind query with the other versions and
transport protocols to be absolutely sure that port won't work with
either version or transport. Then, if all fails, report:

mount.nfs: mount to NFS server 'server' failed:
RPC Error: Program not registered

This change also affects text-based mounts that require negotiation
by the mount.nfs command.

Note that if the mount options specify all four pmap parameters for
NFS, the rpcbind query for the NFS service is skipped entirely. The
mount command then hangs and times out later if NFS service is not
listening on the requested tuple. This is unchanged from previous
behavior.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 682a9855b93a7d3545a26eea39a0895b9757cdcb
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:27:54 2009 -0400

getport: Convert TCP connection refused to RPC_CANTRECV

In a similar vein to the timeout logic we just restored, a refused
TCP connection should be mapped to an equivalent UDP error code:
RPC_CANTRECV.

This is new behavior for TCP connections; the legacy mount command
appears to have simply failed immediately if a TCP connection was
refused during an rpcbind query.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit aa633adcbe63b7539d23d7e0fb1342659cf22953
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:26:42 2009 -0400

getport: Restore historical TCP connect timeout error code

The latest versions of mount.nfs appear not to fall back to
UDP if TCP isn't available on the server.

Our new nfs_getport() implementation is missing a bit of logic
from the original mount getport() implementation. Without it,
nfs_probe_port() sees a TCP connect timeout as a permanent error,
so it fails immediately instead of attempting to try again with
UDP.

Similar changes for our new ping API (see the old clnt_ping()
function, which is still in utils/mount/network.c).

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit dd0761b4852d9946efa86c7403e45b937462503f
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:25:43 2009 -0400

mount.nfs: Add more debugging output around nfs_getport()

So we can see how rpcbind queries are failing during mount processing,
add some debugging messages (enabled with "mount.nfs -v") around the
nfs_getport() calls.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit c7fa61e76f072d97a9bdb4a551aa2ba28e5818cc
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:24:11 2009 -0400

getport: Clear shared error fields before trying rpcbind queries

Some RPC errors set fields in rpc_createerr.cf_error in addition
to cf_stat. Be sure to clear _all_ error fields in rpc_createerr
each time through the rpcbind API.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit e188c214f487c9783ab3ae3e987d9a98b9298dfb
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:21:01 2009 -0400

getport: RPC_PROGNOTREGISTERED is a permanent error

rpcbind returns RPC_PROGNOTREGISTERED if it knows for certain that an
RPC program is not supported for a given transport. This is a
permanent and authoritative error, so the library's rpcbind query API
should never retry the query -- it will only get the same answer.

A similar change was submitted for libtirpc. Unlike rpcb_getaddr(3t),
mount.nfs's rpcbind client only retries once (with RPCB3PROC_GETADDR),
but an extra TCP socket in this case would leave another port in
TIME_WAIT. It's infrequent enough, but might as well get rid of it.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit d0280c201a47cce4aadcfa610b8e03865cce5c5e
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:18:37 2009 -0400

support: Set proper retransmit timeout for datagram transports

Instead of setting the total timeout and the retransmit timeout to the
same value for datagram transports, use a 1 second retransmit timeout,
so we actually get a retransmit or two before failing.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 56a1b590d2f60e62feb3589a7b5b6fab2fed75f7
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:17:28 2009 -0400

support: Don't return RPC_UNKNOWNHOST from rpc_socket.c

RPC_UNKNOWNHOST means a hostname isn't known -- basically it's
EAI_NONAME from getaddrinfo(3). Since the functions in rpc_socket.c
don't take a hostname argument, RPC_UNKNOWNHOST is not an appropriate
return code from these functions.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 965b4cf6398bed2a2c722b9d632b9674c228e36c
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:16:46 2009 -0400

support: Use HAVE_LIBTIRPC to switch in bindresvport_sa(3t)

commit 383a026d99624c88c0e802103ef4c4865db8eb71, which fixed an
earlier commit, is still not quite correct.

bindresvport_sa(3t) is available whenever libtirpc is linked.
There's no need to use IPV6_SUPPORTED here.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1f9c144db7be4c7e5eb07325a752c3306e13bfeb
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:14:19 2009 -0400

New versions of libtool add extra aclocal scripts

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit fb5b52806c58b1b295d1a9aa1fc178a69765321d
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:13:39 2009 -0400

getport: Remove unneeded @salen arguments

Clean up: Now that getnameinfo(3) is no longer used, the @salen
argument to nfs_sockaddr2universal() is no longer needed.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit df5e6316f040f49065d2821c05d4673994d48d4c
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:12:23 2009 -0400

getport: replace getnameinfo(NI_NUMERICHOST) with inet_ntop(3)

getnameinfo(3) with the NI_NUMERICHOST flag is used in
support/nfs/getport.c to convert socket addresses to universal address
strings.

Older versions of glibc do not have getnameinfo(3), however. In order
for nfs-utils to build on older systems we switch in legacy code via
HAVE_GETNAMEINFO and use inet_ntoa(3).

A problem with this is that we have to double our test matrix to be
sure that both versions of these routines build and operate correctly.
Another minor problem is that inet_ntoa(3) is officially deprecated.

So let's always use a single implementation based on inet_ntop(3).
Universal address strings do not support link-local / scope IDs, so we
don't lose any functionality by using inet_ntop(3) here.

This means we open code a bit of logic that is available in most
modern versions of glibc, but in return we can use exactly the same
code for all builds (on systems with getnameinfo(3) and without).

An additional benefit is we can avoid using NI_MAXHOST for character
buffers that live on the stack: it's 1025 bytes. Instead,
INET6_ADDRSTRLEN is used, which is just 46 bytes, plus an additional
eight bytes for the port information. We add beefier buffer overflow
detection logic as well.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 2fcfd397bc555a6c31082c09e6fee7750e41fdb0
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:11:08 2009 -0400

getport: Remove AI_ADDRCONFIG from nfs_gp_loopback_address()

AI_ADDRCONFIG was used ostensibly to figure out if the local system
had IPv6 available when generating a loopback address.

A legacy version of nfs_gp_loopback_address() was created to handle
ANYADDR address generation for old versions of glibc where
AI_ADDRCONFIG doesn't exist. This means we have to be careful to
test both the normal and legacy versions when committing changes in
this area.

But it turns out that even contemporary versions of glibc ignore
AI_ADDRCONFIG when the hostname string is NULL. getaddrinfo(3)
always returns an AF_INET and an AF_INET6 loopback address in this
case, no matter how the system is configured.

Change nfs_gp_loopback_address() to have one version that simply looks
up "localhost" instead of doing anything fancy. If "localhost" is an
IPv6 address, we'll use that. Otherwise, it should nearly always be
an AF_INET loopback address.

This eliminates the need for AI_ADDRCONFIG, and removes the duplicate
version of nfs_gp_loopback_address(). Note that callers never used
the port number in the returned socket address, so get rid of the
"sunrpc" service string too.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 93b7d1f16db674827fc85e70bff1745f57fb6a60
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:09:03 2009 -0400

getport: RPCB_GETADDR's r_addr should contain rpcbind port, not zero

Similar to a change made to the kernel's rpcbind client. See
kernel commit 143b6c4008a7928de7e139c3a77a90e4cad8db2c.

The r_addr argument of RPCB_GETADDR procedures contains the
rpcbind server's address and port number.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 8f859912e786859d21c341bb8a9b4effbdadb941
Author: Chuck Lever <chuck.lever at oracle.com>
Date: Tue Jul 14 16:07:35 2009 -0400

getport: RPCB_GETADDR r_owner should be an empty string

Some servers reject RPCB_GETADDR requests with a non-empty r_owner
field. "RPC: Server can't decode arguments"

An empty string is already used by libtirpc and the kernel
for RPCB_GETADDR requests.

Signed-off-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 1aa4121ba599de836702d7b2d38cad63e6a09044
Author: Steve Dickson <steved at redhat.com>
Date: Mon Jun 29 10:44:20 2009 -0400

mydaemon: remove closeall() calls from mydaemon()

idmapd and svcgssd have a mydaemon() routine that uses closeall() to
close file descriptors. Unfortunately, they aren't using it correctly
and it ends up closing the pipe that the child process uses to talk to
its parent.

Fix this by not using closeall() in this routine and instead, just close
the file descriptors that we know need to be closed. If /dev/null can't
be opened for some reason, then just have the child exit with a non-zero
error.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 097128d72d1ab4be299bf5fdc0b8e83667fc159b
Author: Steve Dickson <steved at redhat.com>
Date: Mon Jun 22 10:05:44 2009 -0400

The closeall function is broken in such a way that it almost never
closes any file descriptors. It's calling strtol on the text
representation of the file descriptor, and then checking to see if the
value of *endptr is not '\0' before trying to close the file. This check
is wrong.

When strtol returns an endptr that points to a NULL byte, that indicates
that the conversion was completely successful. I believe this check
should instead be requiring that endptr is pointing to '\0' before
closing the fd.

Also, fix up the function to check for conversion errors from strtol. If
one occurs, just skip the close on that entry.

Finally, as Trond pointed out, it's unlikely that readdir will return a
blank string in d_name but that situation wouldn't be detected by the
current code. This patch adds such a check and skips the close if it
occurs.

Reported-by: Chuck Lever <chuck.lever at oracle.com>
Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit 148503a575534a4050d709045a6c0dcddd565445
Author: Steve Dickson <steved at redhat.com>
Date: Mon Jun 22 09:49:17 2009 -0400

Make --enable-tirpc the default. If --enable-tirpc wasn't explicitly
specified, but TIRPC libs or headers aren't present then just throw a
warning and disable it. If it was explicitly specified, then throw an
error and exit if they aren't present.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
Signed-off-by: Steve Dickson <steved at redhat.com>

commit f91afa8eaee1d7ba390918d00d4f7d3f338d18d3
Author: Steve Dickson <steved at redhat.com>
Date: Mon Jun 22 09:17:27 2009 -0400

Set the verbosity level in both the librpcsecgss and
libnfsidmapd libraries when verbosity level is set
by the '-v' flag it on either daemon.

Signed-off-by: Steve Dickson <steved at redhat.com>

commit abf92485ae52bd637d544e2ac73147271b310a14
Author: NeilBrown <neilb at suse.de>
Date: Wed Jun 3 15:48:08 2009 -0400

Retry export if getfh fails.

mountd tries to avoid telling the kernel to export something
when the kernel already knows to do that.
However sometimes (exportfs -r) the kernel can be told
to forget something without mountd realising.
So if mountd finds that it cannot get a valid filehandle,
make sure it really has been exported to the kernel.

This only applies if the nfsd filesystem is not mounted.

Signed-off-by: NeilBrown <neilb at suse.de>
Signed-off-by: Steve Dickson <steved at redhat.com>

[Guillaume Rousse]

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated), and the man
page is clearly wrong:

"The fstype field contains either "nfs" (for version 2 or version 3
NFS mounts) or "nfs4" (for NFS version 4 mounts)."

"If this option is not specified, the client attempts to use version 3,
but negotiates the NFS version with the server if version 3 support is
not available."

This is causing a lot of grief among users:
https://qa.mandriva.com/show_bug.cgi?id=57255

[Steve Dickson]

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Post by Guillaume Rousse
"The fstype field contains either "nfs" (for version 2 or version 3
NFS mounts) or "nfs4" (for NFS version 4 mounts)."
"If this option is not specified, the client attempts to use version 3,
but negotiates the NFS version with the server if version 3 support is
not available."

point.

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

steved.

[Guillaume Rousse]

Post by Steve Dickson

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Well, the git log doesn't really constitue a user-targeted
documentation. Your own resume of changes in your mail announcement is
way simpler to read :)

[..]

Post by Steve Dickson

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

Appartently, an up-to-date linux server, from the various reports I had
sofar, such as this one:
http://www.gossamer-threads.com/lists/linux/kernel/1184319

I have yet to reproduce it myself.

[J. Bruce Fields]

Post by Guillaume Rousse

Post by Steve Dickson

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Well, the git log doesn't really constitue a user-targeted
documentation. Your own resume of changes in your mail announcement is
way simpler to read :)
[..]

Post by Steve Dickson

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

Appartently, an up-to-date linux server, from the various reports I had
http://www.gossamer-threads.com/lists/linux/kernel/1184319
I have yet to reproduce it myself.

Almost certainly:

f39bde24b275ddc45df1ed835725b609e178c7a0

I believe that commit is correct; see

http://thread.gmane.org/gmane.linux.nfs/29555/focus=29626

and followup; SERVERFAULT probably needs to be added to mount's list of
errors.

--b.

[J. Bruce Fields]

Post by J. Bruce Fields

Post by Guillaume Rousse

Post by Steve Dickson

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Well, the git log doesn't really constitue a user-targeted
documentation. Your own resume of changes in your mail announcement is
way simpler to read :)
[..]

Post by Steve Dickson

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

Appartently, an up-to-date linux server, from the various reports I had
http://www.gossamer-threads.com/lists/linux/kernel/1184319
I have yet to reproduce it myself.

f39bde24b275ddc45df1ed835725b609e178c7a0
I believe that commit is correct; see
http://thread.gmane.org/gmane.linux.nfs/29555/focus=29626
and followup; SERVERFAULT probably needs to be added to mount's list of
errors.

That said, perhaps we should do both: I can also submit a revert of that
patch for now.

And we also really have to figure out how to fix the server's behavior,
and make sure its default are reasonable. The pseudofs work isn't
enough on its own.

--b.

[J. Bruce Fields]

From: J. Bruce Fields <bfields at citi.umich.edu>

ESERVERFAULT should also cause us to fall back from v4.

It's probably the right thing to do regardless of the linux server
behavior.

Also expand the documentation a little.

And the combination of nested error handling and the switch fall-through
is potentially a little confusing; with a single
(!should_fall_back_to_v3v2()) hopefully it's more obvious.

Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
---
utils/mount/stropts.c | 33 +++++++++++++++++++++++----------
1 files changed, 23 insertions(+), 10 deletions(-)

I thought it was clear from previous discussion that the server would
start returning ESERVERFAULT, but I should have looked at the mount
patch and noticed it didn't handle this.

And we do need to keep working on the server exports; the latest patches
didn't completely eliminate this sort of problem.

Compile-tested only!

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..d3fe9cf 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -658,6 +658,27 @@ out_fail:
return result;
}

+#ifndef ESERVERFAULT
+#define ESERVERFAULT 526
+#endif
+static int should_fall_back_to_v3v2(int err)
+{
+ /*
+ * This is the return we should get if a server doesn't support
+ * v4:
+ */
+ if (err == EPROTONOSUPPORT)
+ return 1;
+ /*
+ * However, Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's when there is no pseudoroot, instead
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Servers prior to 2.6.25 return EPERM, newer servers
+ * may return ENOENT or ESERVERFAULT.
+ */
+ return err == ENOENT || err == EPERM || err == ESERVERFAULT;
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -673,16 +694,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
errno = 0;
result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ if (!should_fall_back_to_v3v2(errno))
+ break;
}
case 2:
case 3:

[Chuck Lever]

Some initial comments below...

Post by J. Bruce Fields
From: J. Bruce Fields <bfields at citi.umich.edu>
ESERVERFAULT should also cause us to fall back from v4.
It's probably the right thing to do regardless of the linux server
behavior.
Also expand the documentation a little.
And the combination of nested error handling and the switch fall-
through
is potentially a little confusing; with a single
(!should_fall_back_to_v3v2()) hopefully it's more obvious.

I don't have a problem with the refactoring you did, but perhaps we
could use the convention of /*FALLTHROUGH*/ to document that the break
is missing on purpose.

Post by J. Bruce Fields
Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
---
utils/mount/stropts.c | 33 +++++++++++++++++++++++----------
1 files changed, 23 insertions(+), 10 deletions(-)
I thought it was clear from previous discussion that the server would
start returning ESERVERFAULT, but I should have looked at the mount
patch and noticed it didn't handle this.
And we do need to keep working on the server exports; the latest patches
didn't completely eliminate this sort of problem.
Compile-tested only!
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..d3fe9cf 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
return result;
}
+#ifndef ESERVERFAULT
+#define ESERVERFAULT 526
+#endif

It might be a little cleaner if this were moved to the top of stropts.c.

Post by J. Bruce Fields
+static int should_fall_back_to_v3v2(int err)

I think we would be better off returning a _bool here instead of an
int. Lots of places in mount.nfs use an int as the protocol version
number, and you are passing an int error value.

It's difficult to look at the name of the function and the type of the
return value and immediately see what's going on. "should_" doesn't
say "this function is a predicate" to me, but that's just my opinion.

Also, nit: the name of static helper functions in this file are
usually prefixed with nfs_, conventionally.

Post by J. Bruce Fields
+{
+ /*
+ * This is the return we should get if a server doesn't support
+ */
+ if (err == EPROTONOSUPPORT)
+ return 1;
+ /*
+ * However, Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's when there is no pseudoroot, instead
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Servers prior to 2.6.25 return EPERM, newer servers
+ * may return ENOENT or ESERVERFAULT.
+ */
+ return err == ENOENT || err == EPERM || err == ESERVERFAULT;

The expression has a boolean result, but it's implicitly cast to (int)
by the "return;". Perhaps another reason for this function to return
_bool.

Maybe this should be a switch statement, like nfs_is_permanent_error().

switch(err) {
case EPROTONOSUPPORT:
case ENOENT:
case EPERM:
case ESERVERFAULT:
return true;
}
return false;

This makes it less error-prone to add and remove error cases.

Post by J. Bruce Fields
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -673,16 +694,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
errno = 0;
result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ if (!should_fall_back_to_v3v2(errno))
+ break;

I think the refactoring helps since it moves that big block comment
out of the retry loop, making the code here shorter and easier to read
(the comment is good to keep somewhere, though).

And, I'm going to bet that the decision whether to fall back to v2/v3
is only going to get more complicated going forward. I can see adding
more linux_version_code() checks, for example, in the future.

Post by J. Bruce Fields
}
--
1.6.3.3
_______________________________________________
NFSv4 mailing list
NFSv4 at linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

[J. Bruce Fields]

Post by Chuck Lever
Some initial comments below...

Thanks.

Post by Chuck Lever

Post by J. Bruce Fields
From: J. Bruce Fields <bfields at citi.umich.edu>
+#ifndef ESERVERFAULT
+#define ESERVERFAULT 526
+#endif

It might be a little cleaner if this were moved to the top of stropts.c.

OK.

Post by Chuck Lever

Post by J. Bruce Fields
+static int should_fall_back_to_v3v2(int err)

I think we would be better off returning a _bool here instead of an int.
Lots of places in mount.nfs use an int as the protocol version number,
and you are passing an int error value.

This patch just uses the existing convention.

Post by Chuck Lever
It's difficult to look at the name of the function and the type of the
return value and immediately see what's going on. "should_" doesn't say
"this function is a predicate" to me, but that's just my opinion.
Also, nit: the name of static helper functions in this file are usually
prefixed with nfs_, conventionally.

How about nfs_v4_unsupported(errno)?

Post by Chuck Lever
Maybe this should be a switch statement, like nfs_is_permanent_error().
switch(err) {
return true;
}
return false;
This makes it less error-prone to add and remove error cases.

OK.

Post by Chuck Lever

Post by J. Bruce Fields
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -673,16 +694,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
errno = 0;
result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ if (!should_fall_back_to_v3v2(errno))
+ break;

I think the refactoring helps since it moves that big block comment out
of the retry loop, making the code here shorter and easier to read (the
comment is good to keep somewhere, though).

(Note it's still there, slightly rewritten.)

Post by Chuck Lever
And, I'm going to bet that the decision whether to fall back to v2/v3 is
only going to get more complicated going forward. I can see adding more
linux_version_code() checks, for example, in the future.

OK.

--b.

[J. Bruce Fields]

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+static int should_fall_back_to_v3v2(int err)

I think we would be better off returning a _bool here instead of an int.
Lots of places in mount.nfs use an int as the protocol version number,
and you are passing an int error value.

This patch just uses the existing convention.

If you wanted to switch to _Bool, you could do something like this.
(Didn't look at anything outside of this file.)

--b.

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index a1f3db3..bcce425 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -152,7 +152,7 @@ static time_t nfs_parse_retry_option(struct mount_options *options,
*
* Returns 1 if the option was appended successfully; otherwise zero.
*/
-static int nfs_append_generic_address_option(const struct sockaddr *sap,
+static _Bool nfs_append_generic_address_option(const struct sockaddr *sap,
const socklen_t salen,
const char *keyword,
struct mount_options *options)
@@ -191,7 +191,7 @@ out_err:
* Returns 1 if 'addr=' option appended successfully;
* otherwise zero.
*/
-static int nfs_append_addr_option(const struct sockaddr *sap,
+static _Bool nfs_append_addr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -206,7 +206,7 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
* Returns 1 if 'clientaddr=' option created successfully or if
* 'clientaddr=' option is already present; otherwise zero.
*/
-static int nfs_append_clientaddr_option(const struct sockaddr *sap,
+static _Bool nfs_append_clientaddr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -229,7 +229,7 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
* 1. "mounthost=" was specified, or
* 2. The address families for proto= and mountproto= are different.
*/
-static int nfs_fix_mounthost_option(struct mount_options *options,
+static _Bool nfs_fix_mounthost_option(struct mount_options *options,
const char *nfs_hostname)
{
union nfs_sockaddr address;
@@ -270,7 +270,7 @@ static const char *nfs_lock_opttbl[] = {
NULL,
};

-static int nfs_verify_lock_option(struct mount_options *options)
+static _Bool nfs_verify_lock_option(struct mount_options *options)
{
if (po_rightmost(options, nfs_lock_opttbl) == 0)
return 1;
@@ -286,7 +286,7 @@ static int nfs_verify_lock_option(struct mount_options *options)
return 1;
}

-static int nfs_append_sloppy_option(struct mount_options *options)
+static _Bool nfs_append_sloppy_option(struct mount_options *options)
{
if (!sloppy || linux_version_code() < MAKE_VERSION(2, 6, 27))
return 1;
@@ -296,7 +296,7 @@ static int nfs_append_sloppy_option(struct mount_options *options)
return 1;
}

-static int nfs_set_version(struct nfsmount_info *mi)
+static _Bool nfs_set_version(struct nfsmount_info *mi)
{
if (!nfs_nfs_version(mi->options, &mi->version))
return 0;
@@ -335,7 +335,7 @@ static int nfs_set_version(struct nfsmount_info *mi)
*
* Returns 1 if successful; otherwise zero.
*/
-static int nfs_validate_options(struct nfsmount_info *mi)
+static _Bool nfs_validate_options(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
sa_family_t family;
@@ -367,7 +367,7 @@ static int nfs_validate_options(struct nfsmount_info *mi)
* Returns 1 and fills in @nfs_saddr, @nfs_salen, @mnt_saddr, and @mnt_salen
* if all goes well; otherwise zero.
*/
-static int nfs_extract_server_addresses(struct mount_options *options,
+static _Bool nfs_extract_server_addresses(struct mount_options *options,
struct sockaddr *nfs_saddr,
socklen_t *nfs_salen,
struct sockaddr *mnt_saddr,
@@ -391,7 +391,7 @@ static int nfs_extract_server_addresses(struct mount_options *options,
return 1;
}

-static int nfs_construct_new_options(struct mount_options *options,
+static _Bool nfs_construct_new_options(struct mount_options *options,
struct sockaddr *nfs_saddr,
struct pmap *nfs_pmap,
struct sockaddr *mnt_saddr,
@@ -470,7 +470,7 @@ static int nfs_construct_new_options(struct mount_options *options,
* Returns TRUE if rewriting was successful; otherwise
* FALSE is returned if some failure occurred.
*/
-static int
+static _Bool
nfs_rewrite_pmap_mount_options(struct mount_options *options)
{
union nfs_sockaddr nfs_address;
@@ -539,10 +539,10 @@ out:
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
+static _Bool nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
{
char *options = NULL;
- int result;
+ _Bool result;

if (po_join(opts, &options) == PO_FAILED) {
errno = EIO;
@@ -569,10 +569,10 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
/*
* For "-t nfs vers=2" or "-t nfs vers=3" mounts.
*/
-static int nfs_try_mount_v3v2(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v3v2(struct nfsmount_info *mi)
{
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;

if (!options) {
errno = ENOMEM;
@@ -612,11 +612,11 @@ out_fail:
/*
* For "-t nfs -o vers=4" or "-t nfs4" mounts.
*/
-static int nfs_try_mount_v4(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v4(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;

if (!options) {
errno = ENOMEM;
@@ -662,7 +662,7 @@ out_fail:
return result;
}

-static int nfs_v4_unsupported(int err)
+static _Bool nfs_v4_unsupported(int err)
{
switch (err) {
/* What we *should* get from a non-v4-supporting server: */
@@ -688,9 +688,9 @@ static int nfs_v4_unsupported(int err)
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_try_mount(struct nfsmount_info *mi)
+static _Bool nfs_try_mount(struct nfsmount_info *mi)
{
- int result = 0;
+ _Bool result = 0;

switch (mi->version) {
case 0:
@@ -729,7 +729,7 @@ static int nfs_try_mount(struct nfsmount_info *mi)
* Returns 1 if we should fail immediately, or 0 if we
* should retry.
*/
-static int nfs_is_permanent_error(int error)
+static _Bool nfs_is_permanent_error(int error)
{
switch (error) {
case ESTALE:

[Chuck Lever]

Post by J. Bruce Fields

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+static int should_fall_back_to_v3v2(int err)

I think we would be better off returning a _bool here instead of an int.
Lots of places in mount.nfs use an int as the protocol version number,
and you are passing an int error value.

This patch just uses the existing convention.

If you wanted to switch to _Bool, you could do something like this.
(Didn't look at anything outside of this file.)

You'd also have to change the comments that document the return
values, and make sure to return "true" instead of "1" and "false"
instead of "0". It's a lot of work, potentially.

Post by J. Bruce Fields
--b.
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index a1f3db3..bcce425 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -152,7 +152,7 @@ static time_t nfs_parse_retry_option(struct mount_options *options,
*
* Returns 1 if the option was appended successfully; otherwise zero.
*/
-static int nfs_append_generic_address_option(const struct sockaddr *sap,
+static _Bool nfs_append_generic_address_option(const struct
sockaddr *sap,
const socklen_t salen,
const char *keyword,
struct mount_options *options)
* Returns 1 if 'addr=' option appended successfully;
* otherwise zero.
*/
-static int nfs_append_addr_option(const struct sockaddr *sap,
+static _Bool nfs_append_addr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -206,7 +206,7 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
* Returns 1 if 'clientaddr=' option created successfully or if
* 'clientaddr=' option is already present; otherwise zero.
*/
-static int nfs_append_clientaddr_option(const struct sockaddr *sap,
+static _Bool nfs_append_clientaddr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -229,7 +229,7 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
* 1. "mounthost=" was specified, or
* 2. The address families for proto= and mountproto= are different.
*/
-static int nfs_fix_mounthost_option(struct mount_options *options,
+static _Bool nfs_fix_mounthost_option(struct mount_options *options,
const char *nfs_hostname)
{
union nfs_sockaddr address;
@@ -270,7 +270,7 @@ static const char *nfs_lock_opttbl[] = {
NULL,
};
-static int nfs_verify_lock_option(struct mount_options *options)
+static _Bool nfs_verify_lock_option(struct mount_options *options)
{
if (po_rightmost(options, nfs_lock_opttbl) == 0)
return 1;
@@ -286,7 +286,7 @@ static int nfs_verify_lock_option(struct
mount_options *options)
return 1;
}
-static int nfs_append_sloppy_option(struct mount_options *options)
+static _Bool nfs_append_sloppy_option(struct mount_options *options)
{
if (!sloppy || linux_version_code() < MAKE_VERSION(2, 6, 27))
return 1;
@@ -296,7 +296,7 @@ static int nfs_append_sloppy_option(struct
mount_options *options)
return 1;
}
-static int nfs_set_version(struct nfsmount_info *mi)
+static _Bool nfs_set_version(struct nfsmount_info *mi)
{
if (!nfs_nfs_version(mi->options, &mi->version))
return 0;
@@ -335,7 +335,7 @@ static int nfs_set_version(struct nfsmount_info *mi)
*
* Returns 1 if successful; otherwise zero.
*/
-static int nfs_validate_options(struct nfsmount_info *mi)
+static _Bool nfs_validate_options(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
sa_family_t family;
@@ -367,7 +367,7 @@ static int nfs_validate_options(struct
nfsmount_info *mi)
* if all goes well; otherwise zero.
*/
-static int nfs_extract_server_addresses(struct mount_options
*options,
+static _Bool nfs_extract_server_addresses(struct mount_options *options,
struct sockaddr *nfs_saddr,
socklen_t *nfs_salen,
struct sockaddr *mnt_saddr,
@@ -391,7 +391,7 @@ static int nfs_extract_server_addresses(struct mount_options *options,
return 1;
}
-static int nfs_construct_new_options(struct mount_options *options,
+static _Bool nfs_construct_new_options(struct mount_options *options,
struct sockaddr *nfs_saddr,
struct pmap *nfs_pmap,
struct sockaddr *mnt_saddr,
@@ -470,7 +470,7 @@ static int nfs_construct_new_options(struct mount_options *options,
* Returns TRUE if rewriting was successful; otherwise
* FALSE is returned if some failure occurred.
*/
-static int
+static _Bool
nfs_rewrite_pmap_mount_options(struct mount_options *options)
{
union nfs_sockaddr nfs_address;
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_sys_mount(struct nfsmount_info *mi, struct
mount_options *opts)
+static _Bool nfs_sys_mount(struct nfsmount_info *mi, struct
mount_options *opts)
{
char *options = NULL;
- int result;
+ _Bool result;
if (po_join(opts, &options) == PO_FAILED) {
errno = EIO;
@@ -569,10 +569,10 @@ static int nfs_sys_mount(struct nfsmount_info
*mi, struct mount_options *opts)
/*
* For "-t nfs vers=2" or "-t nfs vers=3" mounts.
*/
-static int nfs_try_mount_v3v2(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v3v2(struct nfsmount_info *mi)
{
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;
if (!options) {
errno = ENOMEM;
/*
* For "-t nfs -o vers=4" or "-t nfs4" mounts.
*/
-static int nfs_try_mount_v4(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v4(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;
if (!options) {
errno = ENOMEM;
return result;
}
-static int nfs_v4_unsupported(int err)
+static _Bool nfs_v4_unsupported(int err)
{
switch (err) {
/* What we *should* get from a non-v4-supporting server: */
@@ -688,9 +688,9 @@ static int nfs_v4_unsupported(int err)
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_try_mount(struct nfsmount_info *mi)
+static _Bool nfs_try_mount(struct nfsmount_info *mi)
{
- int result = 0;
+ _Bool result = 0;
switch (mi->version) {
@@ -729,7 +729,7 @@ static int nfs_try_mount(struct nfsmount_info *mi)
* Returns 1 if we should fail immediately, or 0 if we
* should retry.
*/
-static int nfs_is_permanent_error(int error)
+static _Bool nfs_is_permanent_error(int error)
{
switch (error) {

[J. Bruce Fields]

Post by J. Bruce Fields

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+static int should_fall_back_to_v3v2(int err)

I think we would be better off returning a _bool here instead of an int.
Lots of places in mount.nfs use an int as the protocol version number,
and you are passing an int error value.

This patch just uses the existing convention.

If you wanted to switch to _Bool, you could do something like this.
(Didn't look at anything outside of this file.)

You'd also have to change the comments that document the return values,
and make sure to return "true" instead of "1" and "false" instead of "0".

I don't think that's necessary.

It's a lot of work, potentially.

I'm not submitting the patch, just responding to your comment....
Between _Bool and int I've got no strong preference.

--b.

Post by J. Bruce Fields
--b.
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index a1f3db3..bcce425 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -152,7 +152,7 @@ static time_t nfs_parse_retry_option(struct mount_options *options,
*
* Returns 1 if the option was appended successfully; otherwise zero.
*/
-static int nfs_append_generic_address_option(const struct sockaddr *sap,
+static _Bool nfs_append_generic_address_option(const struct sockaddr *sap,
const socklen_t salen,
const char *keyword,
struct mount_options *options)
* Returns 1 if 'addr=' option appended successfully;
* otherwise zero.
*/
-static int nfs_append_addr_option(const struct sockaddr *sap,
+static _Bool nfs_append_addr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -206,7 +206,7 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
* Returns 1 if 'clientaddr=' option created successfully or if
* 'clientaddr=' option is already present; otherwise zero.
*/
-static int nfs_append_clientaddr_option(const struct sockaddr *sap,
+static _Bool nfs_append_clientaddr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -229,7 +229,7 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
* 1. "mounthost=" was specified, or
* 2. The address families for proto= and mountproto= are different.
*/
-static int nfs_fix_mounthost_option(struct mount_options *options,
+static _Bool nfs_fix_mounthost_option(struct mount_options *options,
const char *nfs_hostname)
{
union nfs_sockaddr address;
@@ -270,7 +270,7 @@ static const char *nfs_lock_opttbl[] = {
NULL,
};
-static int nfs_verify_lock_option(struct mount_options *options)
+static _Bool nfs_verify_lock_option(struct mount_options *options)
{
if (po_rightmost(options, nfs_lock_opttbl) == 0)
return 1;
@@ -286,7 +286,7 @@ static int nfs_verify_lock_option(struct
mount_options *options)
return 1;
}
-static int nfs_append_sloppy_option(struct mount_options *options)
+static _Bool nfs_append_sloppy_option(struct mount_options *options)
{
if (!sloppy || linux_version_code() < MAKE_VERSION(2, 6, 27))
return 1;
@@ -296,7 +296,7 @@ static int nfs_append_sloppy_option(struct mount_options *options)
return 1;
}
-static int nfs_set_version(struct nfsmount_info *mi)
+static _Bool nfs_set_version(struct nfsmount_info *mi)
{
if (!nfs_nfs_version(mi->options, &mi->version))
return 0;
@@ -335,7 +335,7 @@ static int nfs_set_version(struct nfsmount_info *mi)
*
* Returns 1 if successful; otherwise zero.
*/
-static int nfs_validate_options(struct nfsmount_info *mi)
+static _Bool nfs_validate_options(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
sa_family_t family;
@@ -367,7 +367,7 @@ static int nfs_validate_options(struct
nfsmount_info *mi)
* if all goes well; otherwise zero.
*/
-static int nfs_extract_server_addresses(struct mount_options
*options,
+static _Bool nfs_extract_server_addresses(struct mount_options *options,
struct sockaddr *nfs_saddr,
socklen_t *nfs_salen,
struct sockaddr *mnt_saddr,
@@ -391,7 +391,7 @@ static int nfs_extract_server_addresses(struct
mount_options *options,
return 1;
}
-static int nfs_construct_new_options(struct mount_options *options,
+static _Bool nfs_construct_new_options(struct mount_options *options,
struct sockaddr *nfs_saddr,
struct pmap *nfs_pmap,
struct sockaddr *mnt_saddr,
@@ -470,7 +470,7 @@ static int nfs_construct_new_options(struct mount_options *options,
* Returns TRUE if rewriting was successful; otherwise
* FALSE is returned if some failure occurred.
*/
-static int
+static _Bool
nfs_rewrite_pmap_mount_options(struct mount_options *options)
{
union nfs_sockaddr nfs_address;
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_sys_mount(struct nfsmount_info *mi, struct
mount_options *opts)
+static _Bool nfs_sys_mount(struct nfsmount_info *mi, struct
mount_options *opts)
{
char *options = NULL;
- int result;
+ _Bool result;
if (po_join(opts, &options) == PO_FAILED) {
errno = EIO;
@@ -569,10 +569,10 @@ static int nfs_sys_mount(struct nfsmount_info
*mi, struct mount_options *opts)
/*
* For "-t nfs vers=2" or "-t nfs vers=3" mounts.
*/
-static int nfs_try_mount_v3v2(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v3v2(struct nfsmount_info *mi)
{
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;
if (!options) {
errno = ENOMEM;
/*
* For "-t nfs -o vers=4" or "-t nfs4" mounts.
*/
-static int nfs_try_mount_v4(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v4(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;
if (!options) {
errno = ENOMEM;
return result;
}
-static int nfs_v4_unsupported(int err)
+static _Bool nfs_v4_unsupported(int err)
{
switch (err) {
/* What we *should* get from a non-v4-supporting server: */
@@ -688,9 +688,9 @@ static int nfs_v4_unsupported(int err)
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_try_mount(struct nfsmount_info *mi)
+static _Bool nfs_try_mount(struct nfsmount_info *mi)
{
- int result = 0;
+ _Bool result = 0;
switch (mi->version) {
@@ -729,7 +729,7 @@ static int nfs_try_mount(struct nfsmount_info *mi)
* Returns 1 if we should fail immediately, or 0 if we
* should retry.
*/
-static int nfs_is_permanent_error(int error)
+static _Bool nfs_is_permanent_error(int error)
{
switch (error) {

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

[Peter Staubach]

Post by J. Bruce Fields

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+static int should_fall_back_to_v3v2(int err)

I think we would be better off returning a _bool here instead of an int.
Lots of places in mount.nfs use an int as the protocol version number,
and you are passing an int error value.

This patch just uses the existing convention.

If you wanted to switch to _Bool, you could do something like this.
(Didn't look at anything outside of this file.)

Ewww... I'd prefer int over _Bool.

ps

Post by J. Bruce Fields
--b.
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index a1f3db3..bcce425 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -152,7 +152,7 @@ static time_t nfs_parse_retry_option(struct mount_options *options,
*
* Returns 1 if the option was appended successfully; otherwise zero.
*/
-static int nfs_append_generic_address_option(const struct sockaddr *sap,
+static _Bool nfs_append_generic_address_option(const struct sockaddr *sap,
const socklen_t salen,
const char *keyword,
struct mount_options *options)
* Returns 1 if 'addr=' option appended successfully;
* otherwise zero.
*/
-static int nfs_append_addr_option(const struct sockaddr *sap,
+static _Bool nfs_append_addr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -206,7 +206,7 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
* Returns 1 if 'clientaddr=' option created successfully or if
* 'clientaddr=' option is already present; otherwise zero.
*/
-static int nfs_append_clientaddr_option(const struct sockaddr *sap,
+static _Bool nfs_append_clientaddr_option(const struct sockaddr *sap,
socklen_t salen,
struct mount_options *options)
{
@@ -229,7 +229,7 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
* 1. "mounthost=" was specified, or
* 2. The address families for proto= and mountproto= are different.
*/
-static int nfs_fix_mounthost_option(struct mount_options *options,
+static _Bool nfs_fix_mounthost_option(struct mount_options *options,
const char *nfs_hostname)
{
union nfs_sockaddr address;
@@ -270,7 +270,7 @@ static const char *nfs_lock_opttbl[] = {
NULL,
};
-static int nfs_verify_lock_option(struct mount_options *options)
+static _Bool nfs_verify_lock_option(struct mount_options *options)
{
if (po_rightmost(options, nfs_lock_opttbl) == 0)
return 1;
@@ -286,7 +286,7 @@ static int nfs_verify_lock_option(struct mount_options *options)
return 1;
}
-static int nfs_append_sloppy_option(struct mount_options *options)
+static _Bool nfs_append_sloppy_option(struct mount_options *options)
{
if (!sloppy || linux_version_code() < MAKE_VERSION(2, 6, 27))
return 1;
@@ -296,7 +296,7 @@ static int nfs_append_sloppy_option(struct mount_options *options)
return 1;
}
-static int nfs_set_version(struct nfsmount_info *mi)
+static _Bool nfs_set_version(struct nfsmount_info *mi)
{
if (!nfs_nfs_version(mi->options, &mi->version))
return 0;
@@ -335,7 +335,7 @@ static int nfs_set_version(struct nfsmount_info *mi)
*
* Returns 1 if successful; otherwise zero.
*/
-static int nfs_validate_options(struct nfsmount_info *mi)
+static _Bool nfs_validate_options(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
sa_family_t family;
@@ -367,7 +367,7 @@ static int nfs_validate_options(struct nfsmount_info *mi)
* if all goes well; otherwise zero.
*/
-static int nfs_extract_server_addresses(struct mount_options *options,
+static _Bool nfs_extract_server_addresses(struct mount_options *options,
struct sockaddr *nfs_saddr,
socklen_t *nfs_salen,
struct sockaddr *mnt_saddr,
@@ -391,7 +391,7 @@ static int nfs_extract_server_addresses(struct mount_options *options,
return 1;
}
-static int nfs_construct_new_options(struct mount_options *options,
+static _Bool nfs_construct_new_options(struct mount_options *options,
struct sockaddr *nfs_saddr,
struct pmap *nfs_pmap,
struct sockaddr *mnt_saddr,
@@ -470,7 +470,7 @@ static int nfs_construct_new_options(struct mount_options *options,
* Returns TRUE if rewriting was successful; otherwise
* FALSE is returned if some failure occurred.
*/
-static int
+static _Bool
nfs_rewrite_pmap_mount_options(struct mount_options *options)
{
union nfs_sockaddr nfs_address;
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
+static _Bool nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
{
char *options = NULL;
- int result;
+ _Bool result;
if (po_join(opts, &options) == PO_FAILED) {
errno = EIO;
@@ -569,10 +569,10 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
/*
* For "-t nfs vers=2" or "-t nfs vers=3" mounts.
*/
-static int nfs_try_mount_v3v2(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v3v2(struct nfsmount_info *mi)
{
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;
if (!options) {
errno = ENOMEM;
/*
* For "-t nfs -o vers=4" or "-t nfs4" mounts.
*/
-static int nfs_try_mount_v4(struct nfsmount_info *mi)
+static _Bool nfs_try_mount_v4(struct nfsmount_info *mi)
{
struct sockaddr *sap = &mi->address.sa;
struct mount_options *options = po_dup(mi->options);
- int result = 0;
+ _Bool result = 0;
if (!options) {
errno = ENOMEM;
return result;
}
-static int nfs_v4_unsupported(int err)
+static _Bool nfs_v4_unsupported(int err)
{
switch (err) {
/* What we *should* get from a non-v4-supporting server: */
@@ -688,9 +688,9 @@ static int nfs_v4_unsupported(int err)
* Returns TRUE if successful, otherwise FALSE.
* "errno" is set to reflect the individual error.
*/
-static int nfs_try_mount(struct nfsmount_info *mi)
+static _Bool nfs_try_mount(struct nfsmount_info *mi)
{
- int result = 0;
+ _Bool result = 0;
switch (mi->version) {
@@ -729,7 +729,7 @@ static int nfs_try_mount(struct nfsmount_info *mi)
* Returns 1 if we should fail immediately, or 0 if we
* should retry.
*/
-static int nfs_is_permanent_error(int error)
+static _Bool nfs_is_permanent_error(int error)
{
switch (error) {
_______________________________________________
NFSv4 mailing list
NFSv4 at linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

[Chuck Lever]

Post by J. Bruce Fields

Post by Chuck Lever
It's difficult to look at the name of the function and the type of the
return value and immediately see what's going on. "should_"
doesn't say
"this function is a predicate" to me, but that's just my opinion.
Also, nit: the name of static helper functions in this file are usually
prefixed with nfs_, conventionally.

How about nfs_v4_unsupported(errno)?

I like it.

[J. Bruce Fields]

From: J. Bruce Fields <bfields at citi.umich.edu>

ESERVERFAULT should also cause us to fall back from v4.

It's probably the right thing to do regardless of the linux server
behavior.

Also expand the documentation a little, and attempt to clarify the
control flow.

Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
---
utils/mount/stropts.c | 40 +++++++++++++++++++++++++++++-----------
1 files changed, 29 insertions(+), 11 deletions(-)

Updated with Chuck's suggestions.

Still only compile-tested.--b.

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..a1f3db3 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -73,6 +73,10 @@
#define NFS_DEF_BG_TIMEOUT_MINUTES (10000u)
#endif

+#ifndef ESERVERFAULT
+#define ESERVERFAULT 526
+#endif
+
extern int nfs_mount_data_version;
extern char *progname;
extern int verbose;
@@ -658,6 +662,26 @@ out_fail:
return result;
}

+static int nfs_v4_unsupported(int err)
+{
+ switch (err) {
+ /* What we *should* get from a non-v4-supporting server: */
+ case EPROTONOSUPPORT:
+ /*
+ * However, Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's when there is no pseudoroot, instead
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Servers prior to 2.6.25 return EPERM, newer servers
+ * may return ENOENT or ESERVERFAULT:
+ */
+ case ENOENT:
+ case EPERM:
+ case ESERVERFAULT:
+ return 1;
+ }
+ return 0;
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -671,19 +695,13 @@ static int nfs_try_mount(struct nfsmount_info *mi)
switch (mi->version) {
case 0:
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
- errno = 0;
result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ if (result)
+ break;
+ if (!nfs_v4_unsupported(errno))
+ break;
}
+ /* otherwise fall through: */
case 2:
case 3:
result = nfs_try_mount_v3v2(mi);

[Chuck Lever]

Post by J. Bruce Fields
From: J. Bruce Fields <bfields at citi.umich.edu>
ESERVERFAULT should also cause us to fall back from v4.
It's probably the right thing to do regardless of the linux server
behavior.
Also expand the documentation a little, and attempt to clarify the
control flow.
Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
---
utils/mount/stropts.c | 40 +++++++++++++++++++++++++++++-----------
1 files changed, 29 insertions(+), 11 deletions(-)
Updated with Chuck's suggestions.
Still only compile-tested.--b.
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..a1f3db3 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -73,6 +73,10 @@
#define NFS_DEF_BG_TIMEOUT_MINUTES (10000u)
#endif
+#ifndef ESERVERFAULT
+#define ESERVERFAULT 526
+#endif
+
extern int nfs_mount_data_version;
extern char *progname;
extern int verbose;
return result;
}
+static int nfs_v4_unsupported(int err)
+{
+ switch (err) {
+ /* What we *should* get from a non-v4-supporting server: */
+ /*
+ * However, Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's when there is no pseudoroot, instead

of

Post by J. Bruce Fields
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Servers prior to 2.6.25 return EPERM, newer servers
+ */
+ return 1;
+ }
+ return 0;
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -671,19 +695,13 @@ static int nfs_try_mount(struct nfsmount_info *mi)
switch (mi->version) {
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
- errno = 0;
result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ if (result)
+ break;

Doesn't this cause you to skip the "unsupported" check?

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;
}
+ /* otherwise fall through: */
result = nfs_try_mount_v3v2(mi);
--
1.6.3.3
_______________________________________________
NFSv4 mailing list
NFSv4 at linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

[J. Bruce Fields]

Post by Chuck Lever

Post by J. Bruce Fields
switch (mi->version) {
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
- errno = 0;
result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ if (result)
+ break;

Doesn't this cause you to skip the "unsupported" check?

Yep, but that's correct. If the mount succeeds, there's no need to
check the error number.

I gain renewed appreciation for the kernel's -ERRNO return conventions.

--b.

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;
}
+ /* otherwise fall through: */
result = nfs_try_mount_v3v2(mi);
--
1.6.3.3
_______________________________________________
NFSv4 mailing list
NFSv4 at linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

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

[Chuck Lever]

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
switch (mi->version) {
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
- errno = 0;
result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ if (result)
+ break;

Doesn't this cause you to skip the "unsupported" check?

Yep, but that's correct. If the mount succeeds, there's no need to
check the error number.
I gain renewed appreciation for the kernel's -ERRNO return
conventions.
--b.

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;

The original code fell through only if v4 was unsupported; either
success or some other error would cause it to break and return, but we
needed only the one check. And "not v4 unsupported" seems a little
unnatural.

This might be simpler (and match the other two cases here) if you
reversed the sense of nfs_v4_unsupported(). But maybe it doesn't make
any difference.

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
}
+ /* otherwise fall through: */
result = nfs_try_mount_v3v2(mi);
--
1.6.3.3
_______________________________________________
NFSv4 mailing list
NFSv4 at linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

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

[J. Bruce Fields]

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;

The original code fell through only if v4 was unsupported; either
success or some other error would cause it to break and return,
but we needed only the one check. And "not v4 unsupported" seems a
little unnatural.
This might be simpler (and match the other two cases here) if you
reversed the sense of nfs_v4_unsupported(). But maybe it doesn't make
any difference.

nfs_v4_supported(errno) would be a little hard to interpret.

OK, but I agree, I don't like double-negatives either. I guess you
could do:

switch (mi->version) {
case 0:
if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
goto try_v2v3;
result = nfs_try_mount_v4(mi);
if (result)
break;
if (nfs_v4_unsupported(errno))
goto try_v2v3;
break;
case 2:
case 3:
try_v2v3:
result = nfs_try_mount_v3v2(mi);
break;
case 4:

The goto's a little odd, but it makes the fallback case totally obvious.

--b.

[Chuck Lever]

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;

The original code fell through only if v4 was unsupported; either
success or some other error would cause it to break and return,
but we needed only the one check. And "not v4 unsupported" seems a
little unnatural.
This might be simpler (and match the other two cases here) if you
reversed the sense of nfs_v4_unsupported(). But maybe it doesn't make
any difference.

nfs_v4_supported(errno) would be a little hard to interpret.
OK, but I agree, I don't like double-negatives either. I guess you
switch (mi->version) {
if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
goto try_v2v3;
result = nfs_try_mount_v4(mi);
if (result)
break;
if (nfs_v4_unsupported(errno))
goto try_v2v3;
break;
result = nfs_try_mount_v3v2(mi);
break;
The goto's a little odd, but it makes the fallback case totally obvious.

Oh dear.

Well, fall-through is a long practiced trick for switch statements, so
I thought it was already obvious what was going on here. That's why I
used a switch. You could argue that it's a trick that should be
avoided. But so are goto's, and in my opinion they are the worse of
the two evils.

I don't have a better idea at the moment.

[J. Bruce Fields]

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;

The original code fell through only if v4 was unsupported; either
success or some other error would cause it to break and return,
but we needed only the one check. And "not v4 unsupported" seems a
little unnatural.
This might be simpler (and match the other two cases here) if you
reversed the sense of nfs_v4_unsupported(). But maybe it doesn't make
any difference.

nfs_v4_supported(errno) would be a little hard to interpret.
OK, but I agree, I don't like double-negatives either. I guess you
switch (mi->version) {
if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
goto try_v2v3;
result = nfs_try_mount_v4(mi);
if (result)
break;
if (nfs_v4_unsupported(errno))
goto try_v2v3;
break;
result = nfs_try_mount_v3v2(mi);
break;
The goto's a little odd, but it makes the fallback case totally obvious.

Oh dear.
Well, fall-through is a long practiced trick for switch statements, so I
thought it was already obvious what was going on here. That's why I
used a switch. You could argue that it's a trick that should be
avoided. But so are goto's, and in my opinion they are the worse of the
two evils.
I don't have a better idea at the moment.

OK. In that case, Steve, any objections to applying the last version I
posted?

--b.

[Steve Dickson]

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;

The original code fell through only if v4 was unsupported; either
success or some other error would cause it to break and return,
but we needed only the one check. And "not v4 unsupported" seems a
little unnatural.
This might be simpler (and match the other two cases here) if you
reversed the sense of nfs_v4_unsupported(). But maybe it doesn't make
any difference.

nfs_v4_supported(errno) would be a little hard to interpret.
OK, but I agree, I don't like double-negatives either. I guess you
switch (mi->version) {
if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
goto try_v2v3;
result = nfs_try_mount_v4(mi);
if (result)
break;
if (nfs_v4_unsupported(errno))
goto try_v2v3;
break;
result = nfs_try_mount_v3v2(mi);
break;
The goto's a little odd, but it makes the fallback case totally obvious.

Oh dear.
Well, fall-through is a long practiced trick for switch statements, so I
thought it was already obvious what was going on here. That's why I
used a switch. You could argue that it's a trick that should be
avoided. But so are goto's, and in my opinion they are the worse of the
two evils.
I don't have a better idea at the moment.

OK. In that case, Steve, any objections to applying the last version I
posted?

The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??

Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...

I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Finally, Is there any plans on documenting the new API? I hope so...

steved.

[Chuck Lever]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;

The original code fell through only if v4 was unsupported; either
success or some other error would cause it to break and return,
but we needed only the one check. And "not v4 unsupported" seems a
little unnatural.
This might be simpler (and match the other two cases here) if you
reversed the sense of nfs_v4_unsupported(). But maybe it doesn't make
any difference.

nfs_v4_supported(errno) would be a little hard to interpret.
OK, but I agree, I don't like double-negatives either. I guess you
switch (mi->version) {
if (linux_version_code()<= MAKE_VERSION(2, 6, 31))
goto try_v2v3;
result = nfs_try_mount_v4(mi);
if (result)
break;
if (nfs_v4_unsupported(errno))
goto try_v2v3;
break;
result = nfs_try_mount_v3v2(mi);
break;
The goto's a little odd, but it makes the fallback case totally obvious.

Oh dear.
Well, fall-through is a long practiced trick for switch statements, so I
thought it was already obvious what was going on here. That's why I
used a switch. You could argue that it's a trick that should be
avoided. But so are goto's, and in my opinion they are the worse of the
two evils.
I don't have a better idea at the moment.

OK. In that case, Steve, any objections to applying the last version I
posted?

The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.

Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

[J. Bruce Fields]

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

--b.

[Chuck Lever]

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

Post by J. Bruce Fields
The error values given below result from filesystem type independent
errors. Each filesystem type may have its own special errors and its
own special behavior. See the kernel source code for details.

We've been adding undocumented error returns for mount(2) for quite a
while, I suspect.

[J. Bruce Fields]

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

I suppose it could be translated to a generic EIO or something, but I
think it'd be useful to admins as a way to indicate that the server
might have a hint to the problem.

Post by Chuck Lever

Post by J. Bruce Fields
The error values given below result from filesystem type independent
errors. Each filesystem type may have its own special errors and its
own special behavior. See the kernel source code for details.

We've been adding undocumented error returns for mount(2) for quite a
while, I suspect.

If there's a collection of them then it might make sense to start an
ERRORS section to nfs(5) some day.

--b.

[Steve Dickson]

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

Post by J. Bruce Fields
I suppose it could be translated to a generic EIO or something, but I
think it'd be useful to admins as a way to indicate that the server
might have a hint to the problem.

Post by Chuck Lever

Post by J. Bruce Fields
The error values given below result from filesystem type independent
errors. Each filesystem type may have its own special errors and its
own special behavior. See the kernel source code for details.

We've been adding undocumented error returns for mount(2) for quite a
while, I suspect.

If there's a collection of them then it might make sense to start an
ERRORS section to nfs(5) some day.

How can we document an non-existent (at least in user space) errno?

steved.

[J. Bruce Fields]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

The v3 and v4 client maps NFS4ERR_SERVERFAULT, MNT3ERR_SERVERFAULT, and
NFSERR_SERVERFAULT to ESERVERFAULT to return to userspace. As far as I
can tell it's been doing that for years. I believe that behavior is
intentional, and correct.

--b.

[Trond Myklebust]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

What say we remap it to EREMOTEIO on the client in that case? Clearly we
do want to pass any and all occurrences of NFS4ERR_SERVERFAULT back to
userland. As long as it is defined in the protocol, we must have a valid
mapping for it. It doesn't matter whether or not the Linux server uses
that particular error code...

Cheers
Trond

[Steve Dickson]

Post by Trond Myklebust

Post by Steve Dickson

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

What say we remap it to EREMOTEIO on the client in that case?

I guess this would be a bit more tolerable, but boy its just really
feels wrong to establish this type API between mount and the kernel.
Granted with ENOENT and EPERM we had to out of necessity, but to
actually be expanding it just really is leaving a bad taste...

steved.

[J. Bruce Fields]

Post by Trond Myklebust

Post by Steve Dickson

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

What say we remap it to EREMOTEIO on the client in that case? Clearly we
do want to pass any and all occurrences of NFS4ERR_SERVERFAULT back to
userland. As long as it is defined in the protocol, we must have a valid
mapping for it. It doesn't matter whether or not the Linux server uses
that particular error code...

I could live with mapping {NFS,NFS4,MNT3}ERR_SERVERFAULT to EROMOTEIO
instead of ESERVERFAULT. It still has the suggestion that the problem
is with the server, which is likely to be a useful hint in such cases.

For mount, the suggestion that you and Neil have both made to just
always fail back on error sounds fine too--with the one caveat that if
the server only supports v4, it would be most helpful to return the
original error from the first mount; so e.g., if mount:

tries v4, gets ENOENT, then
tries v3, gets RPC_PROG_MISMATCH,

then ENOENT would be the more helpful error to return.

--b.

[Steve Dickson]

Post by J. Bruce Fields

Post by Trond Myklebust

Post by Steve Dickson

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

What say we remap it to EREMOTEIO on the client in that case? Clearly we
do want to pass any and all occurrences of NFS4ERR_SERVERFAULT back to
userland. As long as it is defined in the protocol, we must have a valid
mapping for it. It doesn't matter whether or not the Linux server uses
that particular error code...

I could live with mapping {NFS,NFS4,MNT3}ERR_SERVERFAULT to EROMOTEIO
instead of ESERVERFAULT. It still has the suggestion that the problem
is with the server, which is likely to be a useful hint in such cases.
For mount, the suggestion that you and Neil have both made to just
always fail back on error sounds fine too--with the one caveat that if
the server only supports v4, it would be most helpful to return the
tries v4, gets ENOENT, then
tries v3, gets RPC_PROG_MISMATCH,
then ENOENT would be the more helpful error to return.

This sounds reasonable...

steved.

[J. Bruce Fields]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Trond Myklebust

Post by Steve Dickson

Post by J. Bruce Fields
I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

What say we remap it to EREMOTEIO on the client in that case? Clearly we
do want to pass any and all occurrences of NFS4ERR_SERVERFAULT back to
userland. As long as it is defined in the protocol, we must have a valid
mapping for it. It doesn't matter whether or not the Linux server uses
that particular error code...

I could live with mapping {NFS,NFS4,MNT3}ERR_SERVERFAULT to EROMOTEIO
instead of ESERVERFAULT. It still has the suggestion that the problem
is with the server, which is likely to be a useful hint in such cases.
For mount, the suggestion that you and Neil have both made to just
always fail back on error sounds fine too--with the one caveat that if
the server only supports v4, it would be most helpful to return the
tries v4, gets ENOENT, then
tries v3, gets RPC_PROG_MISMATCH,
then ENOENT would be the more helpful error to return.

This sounds reasonable...

So, how about this? (Untested.)

--b.

From: J. Bruce Fields <bfields at citi.umich.edu>
Date: Mon, 8 Feb 2010 12:30:37 -0500
Subject: [PATCH] mount: fall back to v2/v3 on any v4 mount error

Linux servers have for a long time been commonly misconfigured in such a
way that they claim support for version 4 of the NFS protocol even when
no pseudoroot is configured, so that they couldn't possibly respond to a
putrootfh.

Rather than try to enumerate the errors which result, just fall back to
v2/v3 on any v4 failure. But in case of a server that only supports v4,
make sure we return the error from the v4 mount and not the subsequent
EPROTONOSUPPORT.

This situation is confusing, so seems worth some additional
documentation. Also attempt to clarify the control flow.

Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
---
utils/mount/stropts.c | 53 +++++++++++++++++++++++++++++++++++++-----------
1 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..de09713 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -658,6 +658,41 @@ out_fail:
return result;
}

+static int nfs_try_mount_v4v3v2(struct nfsmount_info *mi)
+{
+ int result;
+ int savederrno;
+
+ result = nfs_try_mount_v4(mi);
+ if (result)
+ return result;
+ /*
+ * OK, v4 failed, so let's try v2/v3.
+ *
+ * We might choose to do this fallback to v2/v3 only if we got
+ * the specific error EPROTONOSUPPORT, which is what we'd
+ * normally expect when a server doesn't accept NFSv4 rpc's.
+ *
+ * However, many Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's even when there is no pseufilesystem,
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Depending on client and server version, the resulting
+ * return from the mount system call may be ENOENT, EPERM,
+ * ESERVERFAULT, or EREMOTEIO. Instead of enumerating every
+ * possibility, just fall back on any error:
+ */
+ savederrno = errno;
+ result = nfs_try_mount_v3v2(mi);
+ if (!result && errno == EPROTONOSUPPORT)
+ /*
+ * In the case of a v4-only server it's more
+ * helpful to return the error from the original
+ * v4 mount:
+ */
+ errno = savederrno;
+ return result;
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -671,19 +706,13 @@ static int nfs_try_mount(struct nfsmount_info *mi)
switch (mi->version) {
case 0:
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
- errno = 0;
- result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ result = nfs_try_mount_v4v3v2(mi);
+ break;
}
+ /*
+ * We don't consider the client v4 implementation mature
+ * enough to try by default; start with v3/v2 instead:
+ */
case 2:
case 3:
result = nfs_try_mount_v3v2(mi);

[J. Bruce Fields]

Post by J. Bruce Fields
So, how about this? (Untested.)

...

Post by J. Bruce Fields
+ result = nfs_try_mount_v3v2(mi);
+ if (!result && errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
works:

if (!result && rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)

Updated below.

By the way, the error you get from an unhandled RPC_PROGVERSMISMATCH
case is a little ungainly:

mount.nfs: mount to NFS server 'localhost:/exports/HUH/' failed:
RPC Error: Program/version mismatch; errno = Interrupted system
call

--b.

From: J. Bruce Fields <bfields at citi.umich.edu>
Date: Mon, 8 Feb 2010 12:30:37 -0500
Subject: [PATCH] mount: fall back to v2/v3 on any v4 mount error

Linux servers have for a long time been commonly misconfigured in such a
way that they claim support for version 4 of the NFS protocol even when
no pseudoroot is configured, so that they couldn't possibly respond to a
putrootfh.

Rather than try to enumerate the errors which result, just fall back to
v2/v3 on any v4 failure. But in case of a server that only supports v4,
make sure we return the error from the v4 mount and not the subsequent
EPROTONOSUPPORT.

This situation is confusing, so seems worth some additional
documentation. Also attempt to clarify the control flow.

Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
---
utils/mount/stropts.c | 53 +++++++++++++++++++++++++++++++++++++-----------
1 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..6db29a1 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -658,6 +658,41 @@ out_fail:
return result;
}

+static int nfs_try_mount_v4v3v2(struct nfsmount_info *mi)
+{
+ int result;
+ int savederrno;
+
+ result = nfs_try_mount_v4(mi);
+ if (result)
+ return result;
+ /*
+ * OK, v4 failed, so let's try v2/v3.
+ *
+ * We might choose to do this fallback to v2/v3 only if we got
+ * the specific error EPROTONOSUPPORT, which is what we'd
+ * normally expect when a server doesn't accept NFSv4 rpc's.
+ *
+ * However, many Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's even when there is no pseufilesystem,
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Depending on client and server version, the resulting
+ * return from the mount system call may be ENOENT, EPERM,
+ * ESERVERFAULT, or EREMOTEIO. Instead of enumerating every
+ * possibility, just fall back on any error:
+ */
+ savederrno = errno;
+ result = nfs_try_mount_v3v2(mi);
+ if (!result && rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
+ /*
+ * In the case of a v4-only server it's more
+ * helpful to return the error from the original
+ * v4 mount:
+ */
+ errno = savederrno;
+ return result;
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -671,19 +706,13 @@ static int nfs_try_mount(struct nfsmount_info *mi)
switch (mi->version) {
case 0:
if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
- errno = 0;
- result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT && errno != EPERM)
- break;
- }
+ result = nfs_try_mount_v4v3v2(mi);
+ break;
}
+ /*
+ * We don't consider the client v4 implementation mature
+ * enough to try by default; start with v3/v2 instead:
+ */
case 2:
case 3:
result = nfs_try_mount_v3v2(mi);

[Chuck Lever]

Post by J. Bruce Fields

Post by J. Bruce Fields
So, how about this? (Untested.)

...

Post by J. Bruce Fields
+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
Updated below.

Hrm. According to nfs_rewrite_pmap_mount_options you could also see
RPC_PROGNOTREGISTERED. It returns EOPNOTSUPP in that case. Maybe you
should check for PROGVERSMISMATCH in nfs_rewrite_pmap_mount_options as
well, and just check for EOPNOTSUPP in the retry loop.

Post by J. Bruce Fields
By the way, the error you get from an unhandled RPC_PROGVERSMISMATCH
RPC Error: Program/version mismatch; errno = Interrupted system
call
--b.
From: J. Bruce Fields<bfields at citi.umich.edu>
Date: Mon, 8 Feb 2010 12:30:37 -0500
Subject: [PATCH] mount: fall back to v2/v3 on any v4 mount error
Linux servers have for a long time been commonly misconfigured in such a
way that they claim support for version 4 of the NFS protocol even when
no pseudoroot is configured, so that they couldn't possibly respond to a
putrootfh.
Rather than try to enumerate the errors which result, just fall back to
v2/v3 on any v4 failure. But in case of a server that only supports v4,
make sure we return the error from the v4 mount and not the subsequent
EPROTONOSUPPORT.
This situation is confusing, so seems worth some additional
documentation. Also attempt to clarify the control flow.
Signed-off-by: J. Bruce Fields<bfields at citi.umich.edu>
---
utils/mount/stropts.c | 53 +++++++++++++++++++++++++++++++++++++-----------
1 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..6db29a1 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
return result;
}
+static int nfs_try_mount_v4v3v2(struct nfsmount_info *mi)
+{
+ int result;
+ int savederrno;
+
+ result = nfs_try_mount_v4(mi);
+ if (result)
+ return result;
+ /*
+ * OK, v4 failed, so let's try v2/v3.
+ *
+ * We might choose to do this fallback to v2/v3 only if we got
+ * the specific error EPROTONOSUPPORT, which is what we'd
+ * normally expect when a server doesn't accept NFSv4 rpc's.
+ *
+ * However, many Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's even when there is no pseufilesystem,
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Depending on client and server version, the resulting
+ * return from the mount system call may be ENOENT, EPERM,
+ * ESERVERFAULT, or EREMOTEIO. Instead of enumerating every
+ */
+ savederrno = errno;
+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
+ /*
+ * In the case of a v4-only server it's more
+ * helpful to return the error from the original
+ */
+ errno = savederrno;
+ return result;
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -671,19 +706,13 @@ static int nfs_try_mount(struct nfsmount_info *mi)
switch (mi->version) {
if (linux_version_code()> MAKE_VERSION(2, 6, 31)) {
- errno = 0;
- result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT&& errno != EPERM)
- break;
- }
+ result = nfs_try_mount_v4v3v2(mi);
+ break;
}
+ /*
+ * We don't consider the client v4 implementation mature
+ */
result = nfs_try_mount_v3v2(mi);

[Chuck Lever]

So, how about this? (Untested.)

...

+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
Updated below.

Hrm. According to nfs_rewrite_pmap_mount_options you could also see
RPC_PROGNOTREGISTERED. It returns EOPNOTSUPP in that case. Maybe you
should check for PROGVERSMISMATCH in nfs_rewrite_pmap_mount_options as
well, and just check for EOPNOTSUPP in the retry loop.

Sigh. That patch isn't upstream yet. It was posted Jan 22nd.

By the way, the error you get from an unhandled RPC_PROGVERSMISMATCH
RPC Error: Program/version mismatch; errno = Interrupted system
call
--b.
From: J. Bruce Fields<bfields at citi.umich.edu>
Date: Mon, 8 Feb 2010 12:30:37 -0500
Subject: [PATCH] mount: fall back to v2/v3 on any v4 mount error
Linux servers have for a long time been commonly misconfigured in such a
way that they claim support for version 4 of the NFS protocol even when
no pseudoroot is configured, so that they couldn't possibly respond to a
putrootfh.
Rather than try to enumerate the errors which result, just fall back to
v2/v3 on any v4 failure. But in case of a server that only supports v4,
make sure we return the error from the v4 mount and not the subsequent
EPROTONOSUPPORT.
This situation is confusing, so seems worth some additional
documentation. Also attempt to clarify the control flow.
Signed-off-by: J. Bruce Fields<bfields at citi.umich.edu>
---
utils/mount/stropts.c | 53
+++++++++++++++++++++++++++++++++++++-----------
1 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 57a4b32..6db29a1 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
return result;
}
+static int nfs_try_mount_v4v3v2(struct nfsmount_info *mi)
+{
+ int result;
+ int savederrno;
+
+ result = nfs_try_mount_v4(mi);
+ if (result)
+ return result;
+ /*
+ * OK, v4 failed, so let's try v2/v3.
+ *
+ * We might choose to do this fallback to v2/v3 only if we got
+ * the specific error EPROTONOSUPPORT, which is what we'd
+ * normally expect when a server doesn't accept NFSv4 rpc's.
+ *
+ * However, many Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's even when there is no pseufilesystem,
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Depending on client and server version, the resulting
+ * return from the mount system call may be ENOENT, EPERM,
+ * ESERVERFAULT, or EREMOTEIO. Instead of enumerating every
+ */
+ savederrno = errno;
+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
+ /*
+ * In the case of a v4-only server it's more
+ * helpful to return the error from the original
+ */
+ errno = savederrno;
+ return result;
+}
+
/*
* This is a single pass through the fg/bg loop.
*
@@ -671,19 +706,13 @@ static int nfs_try_mount(struct nfsmount_info *mi)
switch (mi->version) {
if (linux_version_code()> MAKE_VERSION(2, 6, 31)) {
- errno = 0;
- result = nfs_try_mount_v4(mi);
- if (errno != EPROTONOSUPPORT) {
- /*
- * To deal with legacy Linux servers that don't
- * automatically export a pseudo root, retry
- * ENOENT errors using version 3. And for
- * Linux servers prior to 2.6.25, retry EPERM
- */
- if (errno != ENOENT&& errno != EPERM)
- break;
- }
+ result = nfs_try_mount_v4v3v2(mi);
+ break;
}
+ /*
+ * We don't consider the client v4 implementation mature
+ */
result = nfs_try_mount_v3v2(mi);

[J. Bruce Fields]

Post by Chuck Lever

So, how about this? (Untested.)

...

+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
Updated below.

Hrm. According to nfs_rewrite_pmap_mount_options you could also see
RPC_PROGNOTREGISTERED. It returns EOPNOTSUPP in that case. Maybe you
should check for PROGVERSMISMATCH in nfs_rewrite_pmap_mount_options as
well, and just check for EOPNOTSUPP in the retry loop.

Sigh. That patch isn't upstream yet. It was posted Jan 22nd.

OK. I'm a little lost, in that case. Can I let you and Steved figure
out what order to do this in? Or let me know what I can do.

--b.

[Chuck Lever]

Post by J. Bruce Fields

Post by Chuck Lever

So, how about this? (Untested.)

...

+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
Updated below.

Hrm. According to nfs_rewrite_pmap_mount_options you could also see
RPC_PROGNOTREGISTERED. It returns EOPNOTSUPP in that case. Maybe you
should check for PROGVERSMISMATCH in nfs_rewrite_pmap_mount_options as
well, and just check for EOPNOTSUPP in the retry loop.

Sigh. That patch isn't upstream yet. It was posted Jan 22nd.

OK. I'm a little lost, in that case. Can I let you and Steved figure
out what order to do this in? Or let me know what I can do.

IMO, you should apply this patch

http://marc.info/?l=linux-nfs&m=126419444905668&w=2

and then do your fix.

[J. Bruce Fields]

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

So, how about this? (Untested.)

...

+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
Updated below.

Hrm. According to nfs_rewrite_pmap_mount_options you could also see
RPC_PROGNOTREGISTERED. It returns EOPNOTSUPP in that case. Maybe you
should check for PROGVERSMISMATCH in nfs_rewrite_pmap_mount_options as
well, and just check for EOPNOTSUPP in the retry loop.

Sigh. That patch isn't upstream yet. It was posted Jan 22nd.

OK. I'm a little lost, in that case. Can I let you and Steved figure
out what order to do this in? Or let me know what I can do.

IMO, you should apply this patch
http://marc.info/?l=linux-nfs&m=126419444905668&w=2
and then do your fix.

Uh, dropped that off the cc.

Looking at your patch, I don't think that's really necessary; they
should be able to go in either order.

--b.

[Steve Dickson]

Post by J. Bruce Fields

Post by Chuck Lever

So, how about this? (Untested.)

...

+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
Updated below.

Hrm. According to nfs_rewrite_pmap_mount_options you could also see
RPC_PROGNOTREGISTERED. It returns EOPNOTSUPP in that case. Maybe you
should check for PROGVERSMISMATCH in nfs_rewrite_pmap_mount_options as
well, and just check for EOPNOTSUPP in the retry loop.

Sigh. That patch isn't upstream yet. It was posted Jan 22nd.

OK. I'm a little lost, in that case. Can I let you and Steved figure
out what order to do this in? Or let me know what I can do.

Me too... Let me catch up with this thread and I'll propose something
(as soon as I can shake these darn deadlines... :-( )

steved.

[Steve Dickson]

Post by Chuck Lever

So, how about this? (Untested.)

...

+ result = nfs_try_mount_v3v2(mi);
+ if (!result&& errno == EPROTONOSUPPORT)

After testing: no, that's not the right condition to check for. This
if (!result&& rpc_createerr.cf_stat == RPC_PROGVERSMISMATCH)
Updated below.

Hrm. According to nfs_rewrite_pmap_mount_options you could also see
RPC_PROGNOTREGISTERED. It returns EOPNOTSUPP in that case. Maybe you
should check for PROGVERSMISMATCH in nfs_rewrite_pmap_mount_options as
well, and just check for EOPNOTSUPP in the retry loop.

Sigh. That patch isn't upstream yet. It was posted Jan 22nd.

Right... Those patches were on the top of my todo list, but they were
posted as RFC plus I thought they only had to do with IPv6 stuff so I
didn't think much of a hurry... I'll take a look at them..

steved.

[Steve Dickson]

Post by J. Bruce Fields

Post by Steve Dickson

Post by J. Bruce Fields

Post by Trond Myklebust

Post by Steve Dickson

Post by J. Bruce Fields
I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

What say we remap it to EREMOTEIO on the client in that case? Clearly we
do want to pass any and all occurrences of NFS4ERR_SERVERFAULT back to
userland. As long as it is defined in the protocol, we must have a valid
mapping for it. It doesn't matter whether or not the Linux server uses
that particular error code...

I could live with mapping {NFS,NFS4,MNT3}ERR_SERVERFAULT to EROMOTEIO
instead of ESERVERFAULT. It still has the suggestion that the problem
is with the server, which is likely to be a useful hint in such cases.
For mount, the suggestion that you and Neil have both made to just
always fail back on error sounds fine too--with the one caveat that if
the server only supports v4, it would be most helpful to return the
tries v4, gets ENOENT, then
tries v3, gets RPC_PROG_MISMATCH,
then ENOENT would be the more helpful error to return.

This sounds reasonable...

So, how about this? (Untested.)

Due to the fact you reverted your last change with this:

commit 260c64d23532caf19abb77e696971da05c388489
Author: J. Bruce Fields <bfields at citi.umich.edu>
Date: Mon Feb 8 13:42:26 2010 -0500

Revert "nfsd4: fix error return when pseudoroot missing"

I'm going to assume this patch is no longer needed... atm...

steved.

[J. Bruce Fields]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Steve Dickson

Post by J. Bruce Fields

Post by Trond Myklebust

Post by Steve Dickson

Post by J. Bruce Fields
I don't know if anyone defines it in userspace; I'm talking about
include/linux/errno.h in the kernel tree. Maybe that's meant to be a
kernel-internal-only error, but it doesn't look that way to me.

The answer here is clearly no... ESERVERFAULT is not define in user space.

What say we remap it to EREMOTEIO on the client in that case? Clearly we
do want to pass any and all occurrences of NFS4ERR_SERVERFAULT back to
userland. As long as it is defined in the protocol, we must have a valid
mapping for it. It doesn't matter whether or not the Linux server uses
that particular error code...

I could live with mapping {NFS,NFS4,MNT3}ERR_SERVERFAULT to EROMOTEIO
instead of ESERVERFAULT. It still has the suggestion that the problem
is with the server, which is likely to be a useful hint in such cases.
For mount, the suggestion that you and Neil have both made to just
always fail back on error sounds fine too--with the one caveat that if
the server only supports v4, it would be most helpful to return the
tries v4, gets ENOENT, then
tries v3, gets RPC_PROG_MISMATCH,
then ENOENT would be the more helpful error to return.

This sounds reasonable...

So, how about this? (Untested.)

commit 260c64d23532caf19abb77e696971da05c388489
Author: J. Bruce Fields <bfields at citi.umich.edu>
Date: Mon Feb 8 13:42:26 2010 -0500
Revert "nfsd4: fix error return when pseudoroot missing"
I'm going to assume this patch is no longer needed... atm...

No, please: we must fix this on both sides!

--b.

[Steve Dickson]

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by Steve Dickson
The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...

Yeah, I was a little surprised that the patch added a definition for
ESERVERFAULT in stropts.c, but didn't really understand the
ramifications of that.
Bruce, would it be better if the NFS client mapped NFS4ERR_SERVERFAULT
to a well-known errno?

Looks to me like ESERVERFAULT has been in include/linux/errno.h since
the beginning of git history at least. I believe the client is correct
to translate NFS4ERR_SERVERFAULT to ESERVERFAULT.

Which user spaces do not have ESERVERFAULT? I don't remember why you
added that conditional macro definition.

Post by J. Bruce Fields
The error values given below result from filesystem type independent
errors. Each filesystem type may have its own special errors
and its
own special behavior. See the kernel source code for details.

We've been adding undocumented error returns for mount(2) for quite a
while, I suspect.

Well I don't think we ever crossed this path... 'we' being the NFS community...

steved.

[J. Bruce Fields]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields

Post by Chuck Lever

Post by J. Bruce Fields
+ if (!nfs_v4_unsupported(errno))
+ break;

The original code fell through only if v4 was unsupported; either
success or some other error would cause it to break and return,
but we needed only the one check. And "not v4 unsupported" seems a
little unnatural.
This might be simpler (and match the other two cases here) if you
reversed the sense of nfs_v4_unsupported(). But maybe it doesn't make
any difference.

nfs_v4_supported(errno) would be a little hard to interpret.
OK, but I agree, I don't like double-negatives either. I guess you
switch (mi->version) {
if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
goto try_v2v3;
result = nfs_try_mount_v4(mi);
if (result)
break;
if (nfs_v4_unsupported(errno))
goto try_v2v3;
break;
result = nfs_try_mount_v3v2(mi);
break;
The goto's a little odd, but it makes the fallback case totally obvious.

Oh dear.
Well, fall-through is a long practiced trick for switch statements, so I
thought it was already obvious what was going on here. That's why I
used a switch. You could argue that it's a trick that should be
avoided. But so are goto's, and in my opinion they are the worse of the
two evils.
I don't have a better idea at the moment.

OK. In that case, Steve, any objections to applying the last version I
posted?

The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??
Where does things like this stop? Will this new kernel to mount API continue to
grow? I hope not...
I just think its very bad form for the kernel to return non standard values that
only a few choose apps know about...
Finally, Is there any plans on documenting the new API? I hope so...

There's no change to the actual client-side mount api; it could always
return SERVERFAULT if something crazy happened on the server.

I agree that it might make sense to add SERVERFAULT and some error
string to libc.

--b.

[J. Bruce Fields]

Post by Steve Dickson

Post by J. Bruce Fields
OK. In that case, Steve, any objections to applying the last version I
posted?

The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??

So, first the legalistic reasons:

- The errors previously used by the kernel here were not legal
errors for the given operation--see the rfc's (both 4.0 and
4.1). SERVERFAULT is a legal error, and also makes the most
sense: an NFSv4 server that knows it can't answer any
PUTROOTFH request should reject

- On the client side, this was resulting in the mount system
call returning errors (EPERM and ENOENT) which have
already-established meanings (see mount(2), which this case
didn't fit into.

More practically:

We should continue working to ensure that the server doesn't accept
NFSv4 rpc's in the absence of a pseudofs. (The recent patches take care
of most of those cases, but not all.)

But the division of labor betwen the kernel and mountd here means that
we can't eliminate those cases completely--the kernel will always have
to have code to handle this case.

So, ignoring the rfc's for now, what error do we want the kernel to
return in this case?

The problem with the existing workaround in mount is it leaves us unable
to distinguish between:

- no v4 support (RPC_PROG_MISMATCH)

- mistyped pathname (ENOENT)

- no root (EPERM)

- The buggy/misconfigured server case, where mountd and nfsd
disagree about v4 support.

Eventually (I hope!) the advantages of being able to return distinct
errors in those cases will outweigh the advantages of supporting older
servers.

It will be more useful to have the fourth case identified by some error
other than ENOENT and EPERM. ESERVERFAULT, as an error that gives a
good hint of where the problem is, and is a natural error to return when
the server returns NFS4ERR_SERVERFAULT, seems like the best choice.

So that's why the server should return SERVERFAULT here.

If, say, reverting this for one kernel version would help give time to
patch mount, maybe I'd consider that.

But I think you'll want this mount patch for now regardless.

--b.

[Steve Dickson]

Post by J. Bruce Fields

Post by Steve Dickson

Post by J. Bruce Fields
OK. In that case, Steve, any objections to applying the last version I
posted?

The problem I have with this is the fact we have to do this at all...
Was it really necessary to change the kernel so it now returns non-standard
error that will break all the recently installed mount commands??

- The errors previously used by the kernel here were not legal
errors for the given operation--see the rfc's (both 4.0 and
4.1). SERVERFAULT is a legal error, and also makes the most
sense: an NFSv4 server that knows it can't answer any
PUTROOTFH request should reject

Legal with respect to the protocol, yes... but SERVERFAULT is unknown
there for not legal for the application namespace.

Post by J. Bruce Fields
- On the client side, this was resulting in the mount system
call returning errors (EPERM and ENOENT) which have
already-established meanings (see mount(2), which this case
didn't fit into.

Maybe the didn't fit but they were known errors... Errors applications
know how to deal with...

Post by J. Bruce Fields
We should continue working to ensure that the server doesn't accept
NFSv4 rpc's in the absence of a pseudofs. (The recent patches take care
of most of those cases, but not all.)

Granted... But how do other server handle this error?

Post by J. Bruce Fields
But the division of labor betwen the kernel and mountd here means that
we can't eliminate those cases completely--the kernel will always have
to have code to handle this case.
So, ignoring the rfc's for now, what error do we want the kernel to
return in this case?
The problem with the existing workaround in mount is it leaves us unable
- no v4 support (RPC_PROG_MISMATCH)
- mistyped pathname (ENOENT)
- no root (EPERM)

But who cares? All three of these error will not even be seen unless they
are seen again with the v3 mount.

Post by J. Bruce Fields
- The buggy/misconfigured server case, where mountd and nfsd
disagree about v4 support.

Again, I have to refer back to my first question: What do other servers
return in this case...

Post by J. Bruce Fields
Eventually (I hope!) the advantages of being able to return distinct
errors in those cases will outweigh the advantages of supporting older
servers.

Well then we should document this new API... If the man page said something
about ESERVERFAULT being return on v4 mounts the again.. this would
be a bit more tolerable

steved.

[J. Bruce Fields]

Post by J. Bruce Fields
+static int nfs_v4_unsupported(int err)
+{
+ switch (err) {
+ /* What we *should* get from a non-v4-supporting server: */
+ /*
+ * However, Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's when there is no pseudoroot, instead

of

Post by J. Bruce Fields
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Servers prior to 2.6.25 return EPERM, newer servers
+ */

It's correct as written. Could simplify the syntax as follows.

--b.

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index a1f3db3..6be2c8b 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -669,8 +669,8 @@ static int nfs_v4_unsupported(int err)
case EPROTONOSUPPORT:
/*
* However, Linux servers are known to violate the rfc's by
- * allowing NFSv4 rpc's when there is no pseudoroot, instead
- * waiting till the client sends PUTROOTFH before returning an
+ * allowing NFSv4 rpc's when there is no pseudoroot. Instead
+ * they wait till the client sends PUTROOTFH before returning an
* error. Servers prior to 2.6.25 return EPERM, newer servers
* may return ENOENT or ESERVERFAULT:
*/

[Chuck Lever]

Post by J. Bruce Fields

Post by J. Bruce Fields
+static int nfs_v4_unsupported(int err)
+{
+ switch (err) {
+ /* What we *should* get from a non-v4-supporting server: */
+ /*
+ * However, Linux servers are known to violate the rfc's by
+ * allowing NFSv4 rpc's when there is no pseudoroot, instead

of

Post by J. Bruce Fields
+ * waiting till the client sends PUTROOTFH before returning an
+ * error. Servers prior to 2.6.25 return EPERM, newer servers
+ */

It's correct as written. Could simplify the syntax as follows.

OK, that makes sense.

Post by J. Bruce Fields
--b.
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index a1f3db3..6be2c8b 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -669,8 +669,8 @@ static int nfs_v4_unsupported(int err)
/*
* However, Linux servers are known to violate the rfc's by
- * allowing NFSv4 rpc's when there is no pseudoroot, instead
- * waiting till the client sends PUTROOTFH before returning an
+ * allowing NFSv4 rpc's when there is no pseudoroot. Instead
+ * they wait till the client sends PUTROOTFH before returning an
* error. Servers prior to 2.6.25 return EPERM, newer servers
*/

[Steve Dickson]

Post by J. Bruce Fields

Post by Guillaume Rousse

Post by Steve Dickson

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Well, the git log doesn't really constitue a user-targeted
documentation. Your own resume of changes in your mail announcement is
way simpler to read :)
[..]

Post by Steve Dickson

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

Appartently, an up-to-date linux server, from the various reports I had
http://www.gossamer-threads.com/lists/linux/kernel/1184319
I have yet to reproduce it myself.

f39bde24b275ddc45df1ed835725b609e178c7a0
I believe that commit is correct; see
http://thread.gmane.org/gmane.linux.nfs/29555/focus=29626
and followup; SERVERFAULT probably needs to be added to mount's list of
errors.

Two questions, why can't you use one of the two errno that will cause
mount.nfs to retry the mount?

Secondly why is this change even needed?

steved.

[Steve Dickson]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Guillaume Rousse

Post by Steve Dickson

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Well, the git log doesn't really constitue a user-targeted
documentation. Your own resume of changes in your mail announcement is
way simpler to read :)
[..]

Post by Steve Dickson

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

Appartently, an up-to-date linux server, from the various reports I had
http://www.gossamer-threads.com/lists/linux/kernel/1184319
I have yet to reproduce it myself.

f39bde24b275ddc45df1ed835725b609e178c7a0
I believe that commit is correct; see
http://thread.gmane.org/gmane.linux.nfs/29555/focus=29626
and followup; SERVERFAULT probably needs to be added to mount's list of
errors.

Two questions, why can't you use one of the two errno that will cause
mount.nfs to retry the mount?
Secondly why is this change even needed?

Also please note, it seems ESERVERFAULT errno is a kernel only errno since
I can seem to find it in any of the headers under /usr/include...

steved.

[J. Bruce Fields]

Post by Steve Dickson

Post by Steve Dickson

Post by J. Bruce Fields

Post by Guillaume Rousse

Post by Steve Dickson

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Well, the git log doesn't really constitue a user-targeted
documentation. Your own resume of changes in your mail announcement is
way simpler to read :)
[..]

Post by Steve Dickson

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

Appartently, an up-to-date linux server, from the various reports I had
http://www.gossamer-threads.com/lists/linux/kernel/1184319
I have yet to reproduce it myself.

f39bde24b275ddc45df1ed835725b609e178c7a0
I believe that commit is correct; see
http://thread.gmane.org/gmane.linux.nfs/29555/focus=29626
and followup; SERVERFAULT probably needs to be added to mount's list of
errors.

Two questions, why can't you use one of the two errno that will cause
mount.nfs to retry the mount?
Secondly why is this change even needed?

Also please note, it seems ESERVERFAULT errno is a kernel only errno since
I can seem to find it in any of the headers under /usr/include...

It is actually returned to userspace (see the bug reports), and is
defined in the kernel's errno.h, so I believe that's intentional. This
is why my patch provides the definition in the nfs-utils search until
userspace catches up.

--b.

[J. Bruce Fields]

Post by Steve Dickson

Post by J. Bruce Fields

Post by Guillaume Rousse

Post by Steve Dickson

Post by Guillaume Rousse

Post by Steve Dickson
* NFS version 4 will be the first version to be tried on client mounts.
If the server does not support version 4, the mounts will roll
back to version 3 and then version 2.

Unfortunatly, this change seems to be mentionned nowhere in the package
documentation (both NEWS and ChangeLog files are outdated),

Yeah.. I have not been Updating either NEWS or the Changelog... I
guess I didn't think that was a problem, since there is a
git log...

Well, the git log doesn't really constitue a user-targeted
documentation. Your own resume of changes in your mail announcement is
way simpler to read :)
[..]

Post by Steve Dickson

Post by Guillaume Rousse
https://qa.mandriva.com/show_bug.cgi?id=57255

With what type of server are you seeing this error?

Appartently, an up-to-date linux server, from the various reports I had
http://www.gossamer-threads.com/lists/linux/kernel/1184319
I have yet to reproduce it myself.

f39bde24b275ddc45df1ed835725b609e178c7a0
I believe that commit is correct; see
http://thread.gmane.org/gmane.linux.nfs/29555/focus=29626
and followup; SERVERFAULT probably needs to be added to mount's list of
errors.

Two questions, why can't you use one of the two errno that will cause
mount.nfs to retry the mount?

Neither one's actually a legal return for PUTROOTFH (according to either
rfc 3530 or rfc 5661).

--b.

Post by Steve Dickson
Secondly why is this change even needed?
steved.