Skip to contentSkip to conversation

showmount: try v3 before falling back to v1

PATCH In review Dan McGee 3 participants 11 messages Jan 5, 2010 – Jan 7, 2010 4 messages not in the archive

Subsystems (2)
lore
Share & tools
lore

Dan McGee

author
#0 lore raw

PATCH 1/1 showmount: try v3 before falling back to v1

A lot of people don't have anything below v3 enabled, so showmount is completely unusable. Try v3 {tcp, udp} first; if they don't work, fall back to v1 {tcp, udp}; if those don't work then just fail as before.

---

First (and quick) attempt at a patch here for showmount. Let me know if you see serious problems with it or the approach. It seemed relatively sane to me and fixed my problems after brief testing.

See a report like this for some proof this is an issue in the wild:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=557954

Thanks,

-Dan

 utils/showmount/showmount.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
utils/showmount/showmount.c +8 -0
index 418e8b9..716c06d 100644
@@ -96,6 +96,14 @@ static CLIENT *nfs_get_mount_client(const char *hostname)
 	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
 	CLIENT *client;
 
+	client = clnt_create(hostname, program, MOUNTVERS_NFSV3, "tcp");
+	if (client)
+		return client;
+
+	client = clnt_create(hostname, program, MOUNTVERS_NFSV3, "udp");
+	if (client)
+		return client;
+
 	client = clnt_create(hostname, program, MOUNTVERS, "tcp");
 	if (client)
 		return client;
Signature · 1 line
1.6.6
#1 lore raw
Quoted 3 lines · Dan McGee

A lot of people don't have anything below v3 enabled, so showmount is completely unusable. Try v3 {tcp, udp} first; if they don't work, fall back to v1 {tcp, udp}; if those don't work then just fail as before.

I don't see any immediate problems with this.

It might be nice to add a command line option to select which transport to use, but that's entirely optional.

Quoted 44 lines

---

First (and quick) attempt at a patch here for showmount. Let me know if you see serious problems with it or the approach. It seemed relatively sane to me and fixed my problems after brief testing.

See a report like this for some proof this is an issue in the wild:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=557954

Thanks,

-Dan

utils/showmount/showmount.c | 8 ++++++++

1 files changed, 8 insertions(+), 0 deletions(-)
utils/showmount/showmount.c +8 -0
index 418e8b9..716c06d 100644
@@ -96,6 +96,14 @@ static CLIENT *nfs_get_mount_client(const char  
 *hostname)
 	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
 	CLIENT *client;
 
+	client = clnt_create(hostname, program, MOUNTVERS_NFSV3, "tcp");
+	if (client)
+		return client;
+
+	client = clnt_create(hostname, program, MOUNTVERS_NFSV3, "udp");
+	if (client)
+		return client;
+
 	client = clnt_create(hostname, program, MOUNTVERS, "tcp");
 	if (client)
		return client;
Signature · 6 lines
1.6.6

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#2 lore raw
Quoted 10 lines · Chuck Lever
Quoted 4 lines · Dan McGee

A lot of people don't have anything below v3 enabled, so showmount is completely unusable. Try v3 {tcp, udp} first; if they don't work, fall back
to v1 {tcp, udp}; if those don't work then just fail as before.

I don't see any immediate problems with this.

Well I do have a bz request that we stop using the lower mount version so we can move away from NFSv2 support... so here is my version of this patch...

Comments??

steved.

Author: Steve Dickson <[email protected]>
Date:   Tue Jan 5 13:29:07 2010 -0500

showmount: Try the highest mount version then fall back to lower ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a RPC_PROGVERSMISMATCH error. The idea being not using the lower mount versions will begin the process of moving away from NFSv2 support.

utils/showmount/showmount.c +21 -5
index 418e8b9..17224a6 100644
@@ -85,22 +85,29 @@ static const char *nfs_sm_pgmtbl[] = {
 	NULL,
 };
 
+static const int mount_vers[] = {
+	MOUNTVERS_NFSV3,
+	MOUNTVERS_POSIX,
+	MOUNTVERS,
+};
+static const int max_vers = (sizeof(mount_vers)/sizeof(mount_vers[0]));
+
 /*
  * Generate an RPC client handle connected to the mountd service
  * at @hostname, or die trying.
  *
  * Supports both AF_INET and AF_INET6 server addresses.
  */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, int vers)
 {
 	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
 	CLIENT *client;
 
-	client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+	client = clnt_create(hostname, program, vers, "tcp");
 	if (client)
 		return client;
 
-	client = clnt_create(hostname, program, MOUNTVERS, "udp");
+	client = clnt_create(hostname, program, vers, "udp");
 	if (client)
 		return client;
 
@@ -122,7 +129,7 @@ int main(int argc, char **argv)
 	mountlist list;
 	int i;
 	int n;
-	int maxlen;
+	int maxlen, vers=0;
 	char **dumpv;
 
 	program_name = argv[0];
@@ -185,7 +192,8 @@ int main(int argc, char **argv)
 		break;
 	}
 
-	mclient = nfs_get_mount_client(hostname);
+again:
+	mclient = nfs_get_mount_client(hostname, mount_vers[vers]);
 	mclient->cl_auth = authunix_create_default();
 	total_timeout.tv_sec = TOTAL_TIMEOUT;
 	total_timeout.tv_usec = 0;
@@ -197,6 +205,10 @@ int main(int argc, char **argv)
 			(xdrproc_t) xdr_void, NULL,
 			(xdrproc_t) xdr_exports, (caddr_t) &exportlist,
 			total_timeout);
+		if (clnt_stat == RPC_PROGVERSMISMATCH) {
+			if (++vers <  max_vers)
+				goto again;
+		}
 		if (clnt_stat != RPC_SUCCESS) {
 			clnt_perror(mclient, "rpc mount export");
 			clnt_destroy(mclient);
@@ -232,6 +244,10 @@ int main(int argc, char **argv)
 		(xdrproc_t) xdr_void, NULL,
 		(xdrproc_t) xdr_mountlist, (caddr_t) &dumplist,
 		total_timeout);
+	if (clnt_stat == RPC_PROGVERSMISMATCH) {
+		if (++vers <  max_vers)
+			goto again;
+	}
 	if (clnt_stat != RPC_SUCCESS) {
 		clnt_perror(mclient, "rpc mount dump");
 		clnt_destroy(mclient);

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw
Quoted 39 lines · Steve Dickson
Quoted 10 lines · Chuck Lever
Quoted 4 lines · Dan McGee

A lot of people don't have anything below v3 enabled, so showmount is completely unusable. Try v3 {tcp, udp} first; if they don't work, fall back
to v1 {tcp, udp}; if those don't work then just fail as before.

I don't see any immediate problems with this.

Well I do have a bz request that we stop using the lower mount version so we can move away from NFSv2 support... so here is my version of this patch...

Comments??

steved.

Author: Steve Dickson <[email protected]>
Date:   Tue Jan 5 13:29:07 2010 -0500

showmount: Try the highest mount version then fall back to lower
ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a
RPC_PROGVERSMISMATCH
error. The idea being not using the lower mount versions will begin the process of moving away from NFSv2 support.

utils/showmount/showmount.c +0 -0
index 418e8b9..17224a6 100644

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] = { · line 88

Quoted 4 lines
utils/showmount/showmount.c +1 -0
@@ -85,22 +85,29 @@ static const char *nfs_sm_pgmtbl[] = {
 	NULL,
 };
 
+static const int mount_vers[] = {

RPC version numbers are rpcvers_t, not int.

The array above this one is called "nfs_sm_pgmtbl"... to be consistent with existing code you should name this one "nfs_sm_verstbl".

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] = {

Quoted 6 lines
utils/showmount/showmount.c +5 -0
+	MOUNTVERS_NFSV3,
+	MOUNTVERS_POSIX,
+	MOUNTVERS,
+};
+static const int max_vers = (sizeof(mount_vers)/ 
 sizeof(mount_vers[0]));

Array indices are unsigned.

Calling this "max_vers" suggests it's actually a version number, and not an index into the version array; that's confusing.

The array above this one has a NULL entry as it's final element. To be consistent with existing code, you should copy that logic for this array instead of using a separate "max_vers" variable.

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] = { · line 101

Quoted 9 lines
utils/showmount/showmount.c +2 -1
+
 /*
 * Generate an RPC client handle connected to the mountd service
 * at @hostname, or die trying.
 *
 * Supports both AF_INET and AF_INET6 server addresses.
 */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, int vers)

RPC version numbers are rpcvers_t, not int. At the very least, clnt_create(3t) takes an unsigned version number argument.

Quoted 15 lines
utils/showmount/showmount.c +2 -2
 {
 	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
 	CLIENT *client;
 
-	client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+	client = clnt_create(hostname, program, vers, "tcp");
 	if (client)
 		return client;
 
-	client = clnt_create(hostname, program, MOUNTVERS, "udp");
+	client = clnt_create(hostname, program, vers, "udp");
 	if (client)
 		return client;

utils/showmount/showmount.c int main(int argc, char **argv) · line 132

Quoted 5 lines
utils/showmount/showmount.c +1 -1
@@ -122,7 +129,7 @@ int main(int argc, char **argv)
 	mountlist list;
 	int i;
 	int n;
-	int maxlen;
+	int maxlen, vers=0;

Array indices are unsigned integers.

Calling this "vers" suggests it's actually a version number, and not an index into the version array; that's confusing.

Quoted 14 lines
utils/showmount/showmount.c +2 -1
 	char **dumpv;
 
 	program_name = argv[0];
@@ -185,7 +192,8 @@ int main(int argc, char **argv)
 		break;
 	}
 
-	mclient = nfs_get_mount_client(hostname);
+again:
+	mclient = nfs_get_mount_client(hostname, mount_vers[vers]);
 	mclient->cl_auth = authunix_create_default();
 	total_timeout.tv_sec = TOTAL_TIMEOUT;
 	total_timeout.tv_usec = 0;

utils/showmount/showmount.c int main(int argc, char **argv) · line 211

Quoted 7 lines
utils/showmount/showmount.c +4 -0
@@ -197,6 +205,10 @@ int main(int argc, char **argv)
 			(xdrproc_t) xdr_void, NULL,
 			(xdrproc_t) xdr_exports, (caddr_t) &exportlist,
 			total_timeout);
+		if (clnt_stat == RPC_PROGVERSMISMATCH) {
+			if (++vers <  max_vers)
+				goto again;
+		}

clnt_create(3t) should tell you if the requested version number is working; libtirpc's version does a NULLPROC call, for example. It would be simpler to move this check into nfs_get_mount_client(), then you wouldn't need the retry loop here.

But nfs_get_mount_client() already doesn't care why the clnt_create(3t) call fails, it just retries the creation with different parameters if it didn't succeed. Do we really care enough to retry _only_ if the specific RPC version isn't available? We use the same procedure number with the same arguments and the same results for all three protocol versions, yes?

Quoted 14 lines
utils/showmount/showmount.c +4 -0
 		if (clnt_stat != RPC_SUCCESS) {
 			clnt_perror(mclient, "rpc mount export");
 			clnt_destroy(mclient);
@@ -232,6 +244,10 @@ int main(int argc, char **argv)
 		(xdrproc_t) xdr_void, NULL,
 		(xdrproc_t) xdr_mountlist, (caddr_t) &dumplist,
 		total_timeout);
+	if (clnt_stat == RPC_PROGVERSMISMATCH) {
+		if (++vers <  max_vers)
+			goto again;
+	}
 	if (clnt_stat != RPC_SUCCESS) {
 		clnt_perror(mclient, "rpc mount dump");
 		clnt_destroy(mclient);
Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#5 lore raw
Quoted 21 lines · Chuck Lever
Quoted 21 lines
Author: Steve Dickson <[email protected]>
Date:   Tue Jan 5 13:29:07 2010 -0500

showmount: Try the highest mount version then fall back to lower ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a RPC_PROGVERSMISMATCH error. The idea being not using the lower mount versions will begin the process of moving away from NFSv2 support.

utils/showmount/showmount.c +0 -0
index 418e8b9..17224a6 100644
@@ -85,22 +85,29 @@ static const char *nfs_sm_pgmtbl[] = {
    NULL,
 };
 
Quoted 3 lines

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] = { · line 88

Quoted 1 line
utils/showmount/showmount.c +1 -0
+static const int mount_vers[] = {

RPC version numbers are rpcvers_t, not int.

True..

Quoted 11 lines

The array above this one is called "nfs_sm_pgmtbl"... to be consistent with existing code you should name this one "nfs_sm_verstbl".

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] = { · line 93

Quoted 5 lines
utils/showmount/showmount.c +5 -0
+    MOUNTVERS_NFSV3,
+    MOUNTVERS_POSIX,
+    MOUNTVERS,
+};
+static const int max_vers = (sizeof(mount_vers)/sizeof(mount_vers[0]));

Array indices are unsigned.

Sure...

Quoted 3 lines

Calling this "max_vers" suggests it's actually a version number, and not an index into the version array; that's confusing.

Its pretty simple code.. don't see this confusion.... but.. It changed to mount_vers_tbl

Quoted 4 lines

The array above this one has a NULL entry as it's final element. To be consistent with existing code, you should copy that logic for this array instead of using a separate "max_vers" variable.

Having the NULL messes up the sizeof calculations

Quoted 37 lines

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] = { · line 101

Quoted 10 lines
utils/showmount/showmount.c +2 -1
+
 /*
 * Generate an RPC client handle connected to the mountd service
 * at @hostname, or die trying.
 *
 * Supports both AF_INET and AF_INET6 server addresses.
 */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, int vers)

RPC version numbers are rpcvers_t, not int. At the very least, clnt_create(3t) takes an unsigned version number argument.

Quoted 15 lines
utils/showmount/showmount.c +2 -2
 {
    rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
    CLIENT *client;
 
-    client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+    client = clnt_create(hostname, program, vers, "tcp");
    if (client)
        return client;
 
-    client = clnt_create(hostname, program, MOUNTVERS, "udp");
+    client = clnt_create(hostname, program, vers, "udp");
    if (client)
        return client;
 

utils/showmount/showmount.c int main(int argc, char **argv) · line 132

Quoted 5 lines
utils/showmount/showmount.c +1 -1
@@ -122,7 +129,7 @@ int main(int argc, char **argv)
    mountlist list;
    int i;
    int n;
-    int maxlen;
+    int maxlen, vers=0;

Array indices are unsigned integers.

Quoted 2 lines

Calling this "vers" suggests it's actually a version number, and not an index into the version array; that's confusing.

and an index call 'j' or 'i' is descriptive?? vers is clear enough imho...

Quoted 19 lines
Quoted 19 lines
utils/showmount/showmount.c +3 -1
    char **dumpv;
 
    program_name = argv[0];
@@ -185,7 +192,8 @@ int main(int argc, char **argv)
        break;
    }
 
-    mclient = nfs_get_mount_client(hostname);
+again:
+    mclient = nfs_get_mount_client(hostname, mount_vers[vers]);
    mclient->cl_auth = authunix_create_default();
    total_timeout.tv_sec = TOTAL_TIMEOUT;
    total_timeout.tv_usec = 0;
@@ -197,6 +205,10 @@ int main(int argc, char **argv)
            (xdrproc_t) xdr_void, NULL,
            (xdrproc_t) xdr_exports, (caddr_t) &exportlist,
            total_timeout);
+        if (clnt_stat == RPC_PROGVERSMISMATCH) {
Quoted 8 lines

utils/showmount/showmount.c int main(int argc, char **argv) · line 211

Quoted 3 lines
utils/showmount/showmount.c +3 -0
+            if (++vers <  max_vers)
+                goto again;
+        }

clnt_create(3t) should tell you if the requested version number is working; libtirpc's version does a NULLPROC call, for example. It would be simpler to move this check into nfs_get_mount_client(), then you wouldn't need the retry loop here.

clnt_create() does not fail.. the clnt_call() does..

Quoted 7 lines

But nfs_get_mount_client() already doesn't care why the clnt_create(3t) call fails, it just retries the creation with different parameters if it didn't succeed. Do we really care enough to retry _only_ if the specific RPC version isn't available? We use the same procedure number with the same arguments and the same results for all three protocol versions, yes?

Let not make a mountain out of mole hill... the goal of this patch is stop using the lower mount protocol version so server can only support v3 and v4... nothing more...

steved.

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#7 lore raw

On Tue, Jan 5, 2010 at 2:38 PM, Steve Dickson <[email protected]> wrote= :

Quoted 6 lines · Chuck Lever
Quoted 5 lines
Quoted 5 lines

Author: Steve Dickson <[email protected]>
Date: =C2=A0 Tue Jan 5 13:29:07 2010 -0500

=C2=A0 =C2=A0showmount: Try the highest mount version then fall bac=

k to lower ones

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=C2=A0 =C2=A0Showmount should try the highest mount version first t=

hen fall

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0back to the lower ones when the server returns a RPC_P=

ROGVERSMISMATCH

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0error. The idea being not using the lower mount versio=

ns will begin

Quoted 5 lines
Quoted 5 lines

utils/showmount/showmoun=

Quoted 5 lines

=C2=A0 =C2=A0the process of moving away from NFSv2 support.

=C2=A0 =C2=A0Signed-off-by: Steve Dickson <[email protected]>

utils/showmount/showmoun= renamed from utils/showmount/showmount.c +0 -0

t.c

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

index 418e8b9..17224a6 100644

utils/showmount/showmount.c +0 -0
@@ -85,22 +85,29 @@ static const char *nfs_sm_pgmtbl[] =3D {
 =C2=A0 =C2=A0 NULL,
 };
 
Quoted 7 lines
Quoted 3 lines

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] =3D { · line 88

Quoted 1 line
utils/showmount/showmount.c +1 -0
+static const int mount_vers[] =3D {

RPC version numbers are rpcvers_t, not int.

True..

Quoted 2 lines

The array above this one is called "nfs_sm_pgmtbl"... to be consiste=

nt

Quoted 7 lines
Quoted 7 lines

with existing code you should name this one "nfs_sm_verstbl".

utils/showmount/showmount.c static const char *nfs_sm_pgmtbl[] =3D { · line 93

Quoted 5 lines
utils/showmount/showmount.c +5 -0
+ =C2=A0 =C2=A0MOUNTVERS_NFSV3,
+ =C2=A0 =C2=A0MOUNTVERS_POSIX,
+ =C2=A0 =C2=A0MOUNTVERS,
+};
+static const int max_vers =3D (sizeof(mount_vers)/sizeof(mount_ver=

s[0]));

Quoted 6 lines
Quoted 2 lines

Array indices are unsigned.

Sure...

Quoted 2 lines

Calling this "max_vers" suggests it's actually a version number, and=

not

Quoted 5 lines
Quoted 1 line

an index into the version array; that's confusing.

Its pretty simple code.. don't see this confusion.... but.. It changed to =C2=A0mount_vers_tbl

Quoted 2 lines

The array above this one has a NULL entry as it's final element. =C2=

=A0To be

Quoted 1 line
Quoted 1 line

consistent with existing code, you should copy that logic for this a=

rray

Quoted 7 lines
Quoted 1 line

instead of using a separate "max_vers" variable.

Having the NULL messes up the sizeof calculations

Quoted 4 lines
Quoted 4 lines
utils/showmount/showmount.c +1 -0
+
 /*

=C2=A0* Generate an RPC client handle connected to the mountd servi=

ce

Quoted 6 lines
Quoted 6 lines

utils/showmount/showmount.c

Quoted 6 lines

=C2=A0* at @hostname, or die trying.
=C2=A0*
=C2=A0* Supports both AF_INET and AF_INET6 server addresses. =C2=A0*/

utils/showmount/showmount.c +1 -1
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, int vers=

)

Quoted 6 lines
Quoted 6 lines

RPC version numbers are rpcvers_t, not int. =C2=A0At the very least, clnt_create(3t) takes an unsigned version number argument.

Quoted 2 lines

{
=C2=A0 =C2=A0 rpcprog_t program =3D nfs_getrpcbyname(MOUNTPROG, nfs=

_sm_pgmtbl);

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

=C2=A0 =C2=A0 CLIENT *client;

- =C2=A0 =C2=A0client =3D clnt_create(hostname, program, MOUNTVERS,=

"tcp");

Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0client =3D clnt_create(hostname, program, vers, "tcp=

");

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

=C2=A0 =C2=A0 if (client)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return client;

- =C2=A0 =C2=A0client =3D clnt_create(hostname, program, MOUNTVERS,=

"udp");

Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0client =3D clnt_create(hostname, program, vers, "udp=

");

Quoted 12 lines
Quoted 12 lines

utils/showmount/showmount.c int main(int argc, char **argv) · line 132

Quoted 9 lines

=C2=A0 =C2=A0 if (client)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return client;

utils/showmount/showmount.c +1 -1
@@ -122,7 +129,7 @@ int main(int argc, char **argv)
 =C2=A0 =C2=A0 mountlist list;
 =C2=A0 =C2=A0 int i;
 =C2=A0 =C2=A0 int n;
- =C2=A0 =C2=A0int maxlen;
+ =C2=A0 =C2=A0int maxlen, vers=3D0;

Array indices are unsigned integers.

Quoted 1 line
Quoted 1 line

Calling this "vers" suggests it's actually a version number, and not=

an

Quoted 2 lines
Quoted 1 line

index into the version array; that's confusing.

and an index call 'j' or 'i' is descriptive?? vers is clear enough im=

ho...

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
utils/showmount/showmount.c +0 -0
 =C2=A0 =C2=A0 char **dumpv;
 
Quoted 8 lines
Quoted 8 lines

utils/showmount/showmount.c int main(int argc, char **argv) · line 196

Quoted 8 lines
utils/showmount/showmount.c +2 -1
 =C2=A0 =C2=A0 program_name =3D argv[0];
@@ -185,7 +192,8 @@ int main(int argc, char **argv)
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
 =C2=A0 =C2=A0 }
 
- =C2=A0 =C2=A0mclient =3D nfs_get_mount_client(hostname);
+again:
+ =C2=A0 =C2=A0mclient =3D nfs_get_mount_client(hostname, mount_ver=

s[vers]);

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
utils/showmount/showmount.c +0 -0
 =C2=A0 =C2=A0 mclient->cl_auth =3D authunix_create_default();
 =C2=A0 =C2=A0 total_timeout.tv_sec =3D TOTAL_TIMEOUT;
 =C2=A0 =C2=A0 total_timeout.tv_usec =3D 0;
@@ -197,6 +205,10 @@ int main(int argc, char **argv)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (xdrproc_t) xdr_void, NUL=

L,

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (xdrproc_t) xdr_exports, =

(caddr_t) &exportlist,

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 total_timeout);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0if (clnt_stat =3D=3D RPC_PROGVERSMISMA=

TCH) {

Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (++vers < =C2=A0max_v=

ers)

Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto again=

;

Quoted 4 lines
Quoted 4 lines
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0}

clnt_create(3t) should tell you if the requested version number is working; libtirpc's version does a NULLPROC call, for example. =C2=A0=

It would

Quoted 6 lines
Quoted 2 lines

be simpler to move this check into nfs_get_mount_client(), then you wouldn't need the retry loop here.

clnt_create() does not fail.. the clnt_call() does..

Quoted 2 lines

But nfs_get_mount_client() already doesn't care why the clnt_create(=

3t)

Quoted 1 line
Quoted 1 line

call fails, it just retries the creation with different parameters i=

f it

Quoted 1 line
Quoted 1 line

didn't succeed. =C2=A0Do we really care enough to retry _only_ if th=

e

Quoted 1 line
Quoted 1 line

specific RPC version isn't available? =C2=A0We use the same procedur=

e number

Quoted 3 lines
Quoted 2 lines

with the same arguments and the same results for all three protocol versions, yes?

Let not make a mountain out of mole hill... the goal of this patch is=

stop using the lower mount protocol version so server can only support= v3 and v4... nothing

Quoted 1 line

more...

As a side note, I don't care what gets accepted as long as it works with modern versions of NFS. Let me know if you need anything more from me, but I'm glad I got the discussion going.

-Dan

#8 lore raw
In reply to Dan McGee

Revised patch incorporating the code review comments...

steved.

commit a96b79f57f49ce5b4d05b6b9da79bdec03b13764

Author: Steve Dickson <[email protected]>
Date:   Tue Jan 5 15:39:00 2010 -0500

showmount: Try the highest mount version then fall back to lower ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a RPC_PROGVERSMISMATCH error. The idea being not using the lower mount versions will begin the process of moving away from NFSv2 support.

utils/showmount/showmount.c +24 -7
index 418e8b9..74cf116 100644
@@ -78,29 +78,36 @@ static void usage(FILE *fp, int n)
 	exit(n);
 }
 
-static const char *nfs_sm_pgmtbl[] = {
+static const char *mount_pgm_tbl[] = {
 	"showmount",
 	"mount",
 	"mountd",
 	NULL,
 };
 
+static const rpcvers_t mount_vers_tbl[] = {
+	MOUNTVERS_NFSV3,
+	MOUNTVERS_POSIX,
+	MOUNTVERS,
+};
+static const int max_vers_tblsz = 
+	(sizeof(mount_vers_tbl)/sizeof(mount_vers_tbl[0]));
+
 /*
  * Generate an RPC client handle connected to the mountd service
  * at @hostname, or die trying.
  *
  * Supports both AF_INET and AF_INET6 server addresses.
  */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, rpcvers_t vers)
 {
-	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
+	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, mount_pgm_tbl);
 	CLIENT *client;
 
-	client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+	client = clnt_create(hostname, program, vers, "tcp");
 	if (client)
 		return client;
-
-	client = clnt_create(hostname, program, MOUNTVERS, "udp");
+	client = clnt_create(hostname, program, vers, "udp");
 	if (client)
 		return client;
 
@@ -123,6 +130,7 @@ int main(int argc, char **argv)
 	int i;
 	int n;
 	int maxlen;
+	int unsigned vers=0;
 	char **dumpv;
 
 	program_name = argv[0];
@@ -185,7 +193,8 @@ int main(int argc, char **argv)
 		break;
 	}
 
-	mclient = nfs_get_mount_client(hostname);
+again:
+	mclient = nfs_get_mount_client(hostname, mount_vers_tbl[vers]);
 	mclient->cl_auth = authunix_create_default();
 	total_timeout.tv_sec = TOTAL_TIMEOUT;
 	total_timeout.tv_usec = 0;
@@ -197,6 +206,10 @@ int main(int argc, char **argv)
 			(xdrproc_t) xdr_void, NULL,
 			(xdrproc_t) xdr_exports, (caddr_t) &exportlist,
 			total_timeout);
+		if (clnt_stat == RPC_PROGVERSMISMATCH) {
+			if (++vers <  max_vers_tblsz)
+				goto again;
+		}
 		if (clnt_stat != RPC_SUCCESS) {
 			clnt_perror(mclient, "rpc mount export");
 			clnt_destroy(mclient);
@@ -232,6 +245,10 @@ int main(int argc, char **argv)
 		(xdrproc_t) xdr_void, NULL,
 		(xdrproc_t) xdr_mountlist, (caddr_t) &dumplist,
 		total_timeout);
+	if (clnt_stat == RPC_PROGVERSMISMATCH) {
+		if (++vers <  max_vers_tblsz)
+			goto again;
+	}
 	if (clnt_stat != RPC_SUCCESS) {
 		clnt_perror(mclient, "rpc mount dump");
 		clnt_destroy(mclient);

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw
Quoted 86 lines · Steve Dickson

Revised patch incorporating the code review comments...

steved.

commit a96b79f57f49ce5b4d05b6b9da79bdec03b13764

Author: Steve Dickson <[email protected]>
Date:   Tue Jan 5 15:39:00 2010 -0500

showmount: Try the highest mount version then fall back to lower
ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a
RPC_PROGVERSMISMATCH
error. The idea being not using the lower mount versions will begin the process of moving away from NFSv2 support.

utils/showmount/showmount.c +16 -7
index 418e8b9..74cf116 100644
@@ -78,29 +78,36 @@ static void usage(FILE *fp, int n)
 	exit(n);
 }
 
-static const char *nfs_sm_pgmtbl[] = {
+static const char *mount_pgm_tbl[] = {
 	"showmount",
 	"mount",
 	"mountd",
 	NULL,
 };
 
+static const rpcvers_t mount_vers_tbl[] = {
+	MOUNTVERS_NFSV3,
+	MOUNTVERS_POSIX,
+	MOUNTVERS,
+};
+static const int max_vers_tblsz =
+	(sizeof(mount_vers_tbl)/sizeof(mount_vers_tbl[0]));
+
 /*
 * Generate an RPC client handle connected to the mountd service
 * at @hostname, or die trying.
 *
 * Supports both AF_INET and AF_INET6 server addresses.
 */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, rpcvers_t  
 vers)
 {
-	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
+	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, mount_pgm_tbl);
 	CLIENT *client;
 
-	client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+	client = clnt_create(hostname, program, vers, "tcp");
 	if (client)
 		return client;
-
-	client = clnt_create(hostname, program, MOUNTVERS, "udp");
+	client = clnt_create(hostname, program, vers, "udp");
 	if (client)
 		return client;
@@ -123,6 +130,7 @@ int main(int argc, char **argv)
 	int i;
 	int n;
 	int maxlen;
+	int unsigned vers=0;
 	char **dumpv;
 
 	program_name = argv[0];
@@ -185,7 +193,8 @@ int main(int argc, char **argv)
 		break;
 	}
 
-	mclient = nfs_get_mount_client(hostname);
+again:
+	mclient = nfs_get_mount_client(hostname, mount_vers_tbl[vers]);
 	mclient->cl_auth = authunix_create_default();
 	total_timeout.tv_sec = TOTAL_TIMEOUT;
 	total_timeout.tv_usec = 0;

utils/showmount/showmount.c int main(int argc, char **argv) · line 212

Quoted 7 lines
utils/showmount/showmount.c +4 -0
@@ -197,6 +206,10 @@ int main(int argc, char **argv)
 			(xdrproc_t) xdr_void, NULL,
 			(xdrproc_t) xdr_exports, (caddr_t) &exportlist,
 			total_timeout);
+		if (clnt_stat == RPC_PROGVERSMISMATCH) {
+			if (++vers <  max_vers_tblsz)
+				goto again;
+		}

If you're going to do this, you should destroy the clnt before retrying. Better yet you can use CLNT_CONTROL() to change the RPC version of the clnt on the fly... that way you don't have to destroy the clnt and create a new one, which has to try creating both "tcp" and "udp" in some cases, making for a long timeout each time if the server doesn't support "tcp".

My preference however, is to do the simple thing, and use clnt_create_vers(3t) in nfs_get_mount_client(). AFAICT you wouldn't need any of this extra logic or the new table.

Quoted 19 lines
utils/showmount/showmount.c +4 -1
 		if (clnt_stat != RPC_SUCCESS) {
 			clnt_perror(mclient, "rpc mount export");
 			clnt_destroy(mclient);
@@ -232,6 +245,10 @@ int main(int argc, char **argv)
 		(xdrproc_t) xdr_void, NULL,
 		(xdrproc_t) xdr_mountlist, (caddr_t) &dumplist,
 		total_timeout);
+	if (clnt_stat == RPC_PROGVERSMISMATCH) {
+		if (++vers <  max_vers_tblsz)
+			goto again;
+	}
 	if (clnt_stat != RPC_SUCCESS) {
 		clnt_perror(mclient, "rpc mount dump");
 		clnt_destroy(mclient);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
#11 lore raw
Quoted 93 lines · Chuck Lever
Quoted 90 lines · Steve Dickson

Revised patch incorporating the code review comments...

steved.

commit a96b79f57f49ce5b4d05b6b9da79bdec03b13764

Author: Steve Dickson <[email protected]>
Date:   Tue Jan 5 15:39:00 2010 -0500

showmount: Try the highest mount version then fall back to lower ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a RPC_PROGVERSMISMATCH error. The idea being not using the lower mount versions will begin the process of moving away from NFSv2 support.

utils/showmount/showmount.c +19 -7
index 418e8b9..74cf116 100644
@@ -78,29 +78,36 @@ static void usage(FILE *fp, int n)
    exit(n);
 }
 
-static const char *nfs_sm_pgmtbl[] = {
+static const char *mount_pgm_tbl[] = {
    "showmount",
    "mount",
    "mountd",
    NULL,
 };
 
+static const rpcvers_t mount_vers_tbl[] = {
+    MOUNTVERS_NFSV3,
+    MOUNTVERS_POSIX,
+    MOUNTVERS,
+};
+static const int max_vers_tblsz =
+    (sizeof(mount_vers_tbl)/sizeof(mount_vers_tbl[0]));
+
 /*
 * Generate an RPC client handle connected to the mountd service
 * at @hostname, or die trying.
 *
 * Supports both AF_INET and AF_INET6 server addresses.
 */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, rpcvers_t
 vers)
 {
-    rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
+    rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, mount_pgm_tbl);
    CLIENT *client;
 
-    client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+    client = clnt_create(hostname, program, vers, "tcp");
    if (client)
        return client;
-
-    client = clnt_create(hostname, program, MOUNTVERS, "udp");
+    client = clnt_create(hostname, program, vers, "udp");
    if (client)
        return client;
@@ -123,6 +130,7 @@ int main(int argc, char **argv)
    int i;
    int n;
    int maxlen;
+    int unsigned vers=0;
    char **dumpv;
 
    program_name = argv[0];
@@ -185,7 +193,8 @@ int main(int argc, char **argv)
        break;
    }
 
-    mclient = nfs_get_mount_client(hostname);
+again:
+    mclient = nfs_get_mount_client(hostname, mount_vers_tbl[vers]);
    mclient->cl_auth = authunix_create_default();
    total_timeout.tv_sec = TOTAL_TIMEOUT;
    total_timeout.tv_usec = 0;
@@ -197,6 +206,10 @@ int main(int argc, char **argv)
            (xdrproc_t) xdr_void, NULL,
            (xdrproc_t) xdr_exports, (caddr_t) &exportlist,
            total_timeout);
+        if (clnt_stat == RPC_PROGVERSMISMATCH) {
+            if (++vers <  max_vers_tblsz)
+                goto again;
Quoted 8 lines

utils/showmount/showmount.c int main(int argc, char **argv) · line 212

Quoted 1 line
utils/showmount/showmount.c +1 -0
+        }

If you're going to do this, you should destroy the clnt before retrying. Better yet you can use CLNT_CONTROL() to change the RPC version of the clnt on the fly... that way you don't have to destroy the clnt and create a new one, which has to try creating both "tcp" and "udp" in some cases, making for a long timeout each time if the server doesn't support "tcp".

This is a very short lived command and 99.9% of the time this code will not executed... so I guess I was not too worried about opening a couple extra fd in the every unlikely case a server does not support v3...

Quoted 4 lines

My preference however, is to do the simple thing, and use clnt_create_vers(3t) in nfs_get_mount_client(). AFAICT you wouldn't need any of this extra logic or the new table.

Yes I saw clnt_create_vers()... but I thought staying backwards compatible with the glibc was a better idea...

steved.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#13 lore raw
Quoted 105 lines · Steve Dickson
Quoted 96 lines · Chuck Lever
Quoted 93 lines · Steve Dickson

Revised patch incorporating the code review comments...

steved.

commit a96b79f57f49ce5b4d05b6b9da79bdec03b13764

Author: Steve Dickson <[email protected]>
Date:   Tue Jan 5 15:39:00 2010 -0500

showmount: Try the highest mount version then fall back to lower
ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a
RPC_PROGVERSMISMATCH
error. The idea being not using the lower mount versions will
begin
the process of moving away from NFSv2 support.

utils/showmount/showmount.c +19 -7
index 418e8b9..74cf116 100644
@@ -78,29 +78,36 @@ static void usage(FILE *fp, int n)
   exit(n);
 }
 
-static const char *nfs_sm_pgmtbl[] = {
+static const char *mount_pgm_tbl[] = {
   "showmount",
   "mount",
   "mountd",
   NULL,
 };
 
+static const rpcvers_t mount_vers_tbl[] = {
+    MOUNTVERS_NFSV3,
+    MOUNTVERS_POSIX,
+    MOUNTVERS,
+};
+static const int max_vers_tblsz =
+    (sizeof(mount_vers_tbl)/sizeof(mount_vers_tbl[0]));
+
 /*
 * Generate an RPC client handle connected to the mountd service
 * at @hostname, or die trying.
 *
 * Supports both AF_INET and AF_INET6 server addresses.
 */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, rpcvers_t
 vers)
 {
-    rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
+    rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, mount_pgm_tbl);
   CLIENT *client;
 
-    client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+    client = clnt_create(hostname, program, vers, "tcp");
   if (client)
       return client;
-
-    client = clnt_create(hostname, program, MOUNTVERS, "udp");
+    client = clnt_create(hostname, program, vers, "udp");
   if (client)
       return client;
@@ -123,6 +130,7 @@ int main(int argc, char **argv)
   int i;
   int n;
   int maxlen;
+    int unsigned vers=0;
   char **dumpv;
 
   program_name = argv[0];
@@ -185,7 +193,8 @@ int main(int argc, char **argv)
       break;
   }
 
-    mclient = nfs_get_mount_client(hostname);
+again:
+    mclient = nfs_get_mount_client(hostname, mount_vers_tbl[vers]);
   mclient->cl_auth = authunix_create_default();
   total_timeout.tv_sec = TOTAL_TIMEOUT;
   total_timeout.tv_usec = 0;
@@ -197,6 +206,10 @@ int main(int argc, char **argv)
           (xdrproc_t) xdr_void, NULL,
           (xdrproc_t) xdr_exports, (caddr_t) &exportlist,
           total_timeout);
+        if (clnt_stat == RPC_PROGVERSMISMATCH) {
+            if (++vers <  max_vers_tblsz)
+                goto again;
Quoted 8 lines

utils/showmount/showmount.c int main(int argc, char **argv) · line 212

Quoted 1 line
utils/showmount/showmount.c +1 -0
+        }

If you're going to do this, you should destroy the clnt before retrying. Better yet you can use CLNT_CONTROL() to change the RPC version of the clnt on the fly... that way you don't have to destroy the clnt and create a new one, which has to try creating both "tcp" and "udp" in some cases, making for a long timeout each time if the server doesn't support "tcp".

Quoted 3 lines

This is a very short lived command and 99.9% of the time this code will not executed... so I guess I was not too worried about opening a couple extra fd in the every unlikely case a server does not support v3...

Dude, come on. How hard is it to add two extra clnt_destroy(3) calls?

What about the potential for additional hangs during retries when neither v3 nor TCP is supported? CLNT_CONTROL() would allow you to create the client once, and then reuse it. I can send you a patch on top of this one that does that.

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

#14 lore raw
In reply to Dan McGee

Addressed the not freeing of the client handle when the RPC_PROGVERSMISMATCH error occurs. Also discovered there no hang when the server does not support TCP connections due to the fact the clnt_create() quires the server on what transports are supported...

steved.

Author: Steve Dickson <[email protected]>
Date:   Thu Jan 7 11:30:03 2010 -0500

showmount: Try the highest mount version then fall back to lower ones

Showmount should try the highest mount version first then fall back to the lower ones when the server returns a RPC_PROGVERSMISMATCH error. The idea being not using the lower mount versions will begin the process of moving away from NFSv2 support.

utils/showmount/showmount.c +28 -7
index 418e8b9..80fd47a 100644
@@ -78,29 +78,36 @@ static void usage(FILE *fp, int n)
 	exit(n);
 }
 
-static const char *nfs_sm_pgmtbl[] = {
+static const char *mount_pgm_tbl[] = {
 	"showmount",
 	"mount",
 	"mountd",
 	NULL,
 };
 
+static const rpcvers_t mount_vers_tbl[] = {
+	MOUNTVERS_NFSV3,
+	MOUNTVERS_POSIX,
+	MOUNTVERS,
+};
+static const int max_vers_tblsz = 
+	(sizeof(mount_vers_tbl)/sizeof(mount_vers_tbl[0]));
+
 /*
  * Generate an RPC client handle connected to the mountd service
  * at @hostname, or die trying.
  *
  * Supports both AF_INET and AF_INET6 server addresses.
  */
-static CLIENT *nfs_get_mount_client(const char *hostname)
+static CLIENT *nfs_get_mount_client(const char *hostname, rpcvers_t vers)
 {
-	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, nfs_sm_pgmtbl);
+	rpcprog_t program = nfs_getrpcbyname(MOUNTPROG, mount_pgm_tbl);
 	CLIENT *client;
 
-	client = clnt_create(hostname, program, MOUNTVERS, "tcp");
+	client = clnt_create(hostname, program, vers, "tcp");
 	if (client)
 		return client;
-
-	client = clnt_create(hostname, program, MOUNTVERS, "udp");
+	client = clnt_create(hostname, program, vers, "udp");
 	if (client)
 		return client;
 
@@ -123,6 +130,7 @@ int main(int argc, char **argv)
 	int i;
 	int n;
 	int maxlen;
+	int unsigned vers=0;
 	char **dumpv;
 
 	program_name = argv[0];
@@ -185,7 +193,8 @@ int main(int argc, char **argv)
 		break;
 	}
 
-	mclient = nfs_get_mount_client(hostname);
+again:
+	mclient = nfs_get_mount_client(hostname, mount_vers_tbl[vers]);
 	mclient->cl_auth = authunix_create_default();
 	total_timeout.tv_sec = TOTAL_TIMEOUT;
 	total_timeout.tv_usec = 0;
@@ -197,6 +206,12 @@ int main(int argc, char **argv)
 			(xdrproc_t) xdr_void, NULL,
 			(xdrproc_t) xdr_exports, (caddr_t) &exportlist,
 			total_timeout);
+		if (clnt_stat == RPC_PROGVERSMISMATCH) {
+			if (++vers <  max_vers_tblsz) {
+				clnt_destroy(mclient);
+				goto again;
+			}
+		}
 		if (clnt_stat != RPC_SUCCESS) {
 			clnt_perror(mclient, "rpc mount export");
 			clnt_destroy(mclient);
@@ -232,6 +247,12 @@ int main(int argc, char **argv)
 		(xdrproc_t) xdr_void, NULL,
 		(xdrproc_t) xdr_mountlist, (caddr_t) &dumplist,
 		total_timeout);
+	if (clnt_stat == RPC_PROGVERSMISMATCH) {
+		if (++vers <  max_vers_tblsz) {
+			clnt_destroy(mclient);
+			goto again;
+		}
+	}
 	if (clnt_stat != RPC_SUCCESS) {
 		clnt_perror(mclient, "rpc mount dump");
 		clnt_destroy(mclient);