On Tue, Jan 5, 2010 at 2:38 PM, Steve Dickson <[email protected]> wrote=
:
> On 01/05/2010 02:23 PM, Chuck Lever wrote:
>>>
>>> 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
>>>
>>> =C2=A0 =C2=A0Showmount should try the highest mount version first t=
hen fall
>>> =C2=A0 =C2=A0back to the lower ones when the server returns a RPC_P=
ROGVERSMISMATCH
>>> =C2=A0 =C2=A0error. The idea being not using the lower mount versio=
ns will begin
>>> =C2=A0 =C2=A0the process of moving away from NFSv2 support.
>>>
>>> =C2=A0 =C2=A0Signed-off-by: Steve Dickson <[email protected]>
>>>
>>> diff --git a/utils/showmount/showmount.c b/utils/showmount/showmoun=
t.c
>>> index 418e8b9..17224a6 100644
>>> --- a/utils/showmount/showmount.c
>>> +++ b/utils/showmount/showmount.c
>>> @@ -85,22 +85,29 @@ static const char *nfs_sm_pgmtbl[] =3D {
>>> =C2=A0 =C2=A0 NULL,
>>> };
>>>
>>> +static const int mount_vers[] =3D {
>>
>> RPC version numbers are rpcvers_t, not int.
> True..
>
>>
>> The array above this one is called "nfs_sm_pgmtbl"... to be consiste=
nt
>> with existing code you should name this one "nfs_sm_verstbl".
>>
>>> + =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]));
>>
>> Array indices are unsigned.
> Sure...
>
>>
>> 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 =C2=A0mount_vers_tbl
>>
>> The array above this one has a NULL entry as it's final element. =C2=
=A0To be
>> consistent with existing code, you should copy that logic for this a=
rray
>> instead of using a separate "max_vers" variable.
> Having the NULL messes up the sizeof calculations
>
>>
>>> +
>>> /*
>>> =C2=A0* Generate an RPC client handle connected to the mountd servi=
ce
>>> =C2=A0* at @hostname, or die trying.
>>> =C2=A0*
>>> =C2=A0* Supports both AF_INET and AF_INET6 server addresses.
>>> =C2=A0*/
>>> -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. =C2=A0At the very least,
>> clnt_create(3t) takes an unsigned version number argument.
>>
>>> {
>>> =C2=A0 =C2=A0 rpcprog_t program =3D nfs_getrpcbyname(MOUNTPROG, nfs=
_sm_pgmtbl);
>>> =C2=A0 =C2=A0 CLIENT *client;
>>>
>>> - =C2=A0 =C2=A0client =3D clnt_create(hostname, program, MOUNTVERS,=
"tcp");
>>> + =C2=A0 =C2=A0client =3D clnt_create(hostname, program, vers, "tcp=
");
>>> =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");
>>> + =C2=A0 =C2=A0client =3D clnt_create(hostname, program, vers, "udp=
");
>>> =C2=A0 =C2=A0 if (client)
>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return client;
>>>
>>> @@ -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.
>>
>> 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 im=
ho...
>
>>
>>> =C2=A0 =C2=A0 char **dumpv;
>>>
>>> =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]);
>>> =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,
>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (xdrproc_t) xdr_exports, =
(caddr_t) &exportlist,
>>> =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) {
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (++vers < =C2=A0max_v=
ers)
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto again=
;
>>> + =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
>> 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..
>
>>
>> But nfs_get_mount_client() already doesn't care why the clnt_create(=
3t)
>> call fails, it just retries the creation with different parameters i=
f it
>> didn't succeed. =C2=A0Do we really care enough to retry _only_ if th=
e
>> specific RPC version isn't available? =C2=A0We use the same procedur=
e 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...
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