Re: [PATCH 2/3] SUNRPC client: add interface for binding to a local address

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

Trond Myklebust wrote:
> On Thu, 2007-07-05 at 17:26 +0200, Frank van Maarseveen wrote:
>> @@ -1146,31 +1147,33 @@ static void xs_set_port(struct rpc_xprt 
>>  	sap->sin_port = htons(port);
>>  }
>>  
>> -static int xs_bindresvport(struct sock_xprt *transport, struct socket *sock)
>> +static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>>  {
>>  	struct sockaddr_in myaddr = {
>>  		.sin_family = AF_INET,
>>  	};
>> +	struct sockaddr_in *sa;
>>  	int err;
>>  	unsigned short port = transport->port;
> 
> Instead of forcing two tests of transport->xprt.resvport per loop, why
> not just set port to 0 in the case of !transport->xprt.resvport?

This logic seems unnecessarily complicated to me, too.  I'm not so 
concerned about the loop optimization, but it certainly isn't immediate 
from looking at these changes exactly what is going on here in the two 
important cases ("yes we want a reserved port" and "no, any old port 
will do, thanks").  Anything that clarifies this would be welcome, but 
I'd encourage code clarity over just adding a comment or two.

>>  
>> +	sa = (struct sockaddr_in *)&transport->saddr;
>> +	myaddr.sin_addr = sa->sin_addr;
>>  	do {
>> -		myaddr.sin_port = htons(port);
>> +		if (transport->xprt.resvport)
>> +			myaddr.sin_port = htons(port);
>>  		err = kernel_bind(sock, (struct sockaddr *) &myaddr,
>>  						sizeof(myaddr));
>> -		if (err == 0) {
>> +		if (err == 0 || !transport->xprt.resvport) {
>>  			transport->port = port;
>> -			dprintk("RPC:       xs_bindresvport bound to port %u\n",
>> -					port);
>> -			return 0;
>> +			break;
>>  		}
>>  		if (port <= xprt_min_resvport)
>>  			port = xprt_max_resvport;
>>  		else
>>  			port--;
>>  	} while (err == -EADDRINUSE && port != transport->port);
>> -
>> -	dprintk("RPC:       can't bind to reserved port (%d).\n", -err);
>> +	dprintk("RPC:       xs_bind "NIPQUAD_FMT":%u: %d\n",
>> +		NIPQUAD(myaddr.sin_addr), port, err);
> 
> Hmm... I thought you said this was IPv6 clean :-)

Well, IPv6 uses a separate xs_bindresvport routine, so IPv4-specific 
stuff can be added in here with aplomb.  We just have to remember to 
make Frank's changes to the xs_bindresvport6 when it is integrated.

Frank, can you make this debugging message a little more friendly?

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 315 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
email;internet:chuck dot lever at nospam oracle dot com
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs