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

[Frank van Maarseveen <frankvm@frankvm.com>]

On Fri, Jul 06, 2007 at 11:40:25AM -0400, Chuck Lever wrote:
> 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.

ok, I'll rephrase it a bit.

[snip]
> >>-	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?

ok,

I'll first create a cleanup patch as Trond suggested.


-- 
Frank

-------------------------------------------------------------------------
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/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs