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 09:46:16AM -0400, Trond Myklebust wrote:
> On Thu, 2007-07-05 at 17:26 +0200, Frank van Maarseveen wrote:
> > In addition to binding to a local privileged port the NFS client should
> > allow binding to a specific local address. This is used by the server
> > for callbacks. The patch adds the necessary interface.
> > 
> > Signed-off-by: Frank van Maarseveen <frankvm@frankvm.com>
> > ---
> > 
> > diff -urp a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> > --- a/include/linux/sunrpc/clnt.h	2007-04-27 18:05:33.000000000 +0200
> > +++ b/include/linux/sunrpc/clnt.h	2007-07-05 15:33:11.000000000 +0200
> > @@ -97,6 +97,7 @@ struct rpc_create_args {
> >  	int			protocol;
> >  	struct sockaddr		*address;
> >  	size_t			addrsize;
> > +	struct sockaddr		*saddress;
> >  	struct rpc_timeout	*timeout;
> >  	char			*servername;
> >  	struct rpc_program	*program;
> > diff -urp a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> > --- a/include/linux/sunrpc/xprt.h	2007-02-27 22:55:35.000000000 +0100
> > +++ b/include/linux/sunrpc/xprt.h	2007-07-05 15:33:11.000000000 +0200
> > @@ -201,7 +201,8 @@ void			xprt_set_timeout(struct rpc_timeo
> >  /*
> >   * Generic internal transport functions
> >   */
> > -struct rpc_xprt *	xprt_create_transport(int proto, struct sockaddr *addr, size_t size, struct rpc_timeout *toparms);
> > +struct rpc_xprt *	xprt_create_transport(int proto, struct sockaddr *addr, size_t size,
> > +					      struct sockaddr *saddr, struct rpc_timeout *toparms);
> >  void			xprt_connect(struct rpc_task *task);
> >  void			xprt_reserve(struct rpc_task *task);
> >  int			xprt_reserve_xprt(struct rpc_task *task);
> > @@ -239,8 +240,10 @@ void			xprt_disconnect(struct rpc_xprt *
> >  /*
> >   * Socket transport setup operations
> >   */
> > -struct rpc_xprt *	xs_setup_udp(struct sockaddr *addr, size_t addrlen, struct rpc_timeout *to);
> > -struct rpc_xprt *	xs_setup_tcp(struct sockaddr *addr, size_t addrlen, struct rpc_timeout *to);
> > +struct rpc_xprt *	xs_setup_udp(struct sockaddr *addr, size_t addrlen,
> > +				     struct sockaddr *saddr, struct rpc_timeout *to);
> > +struct rpc_xprt *	xs_setup_tcp(struct sockaddr *addr, size_t addrlen,
> > +				     struct sockaddr *saddr, struct rpc_timeout *to);
> >  
> >  /*
> >   * Reserved bit positions in xprt->state
> > diff -urp a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > --- a/net/sunrpc/clnt.c	2007-04-27 18:05:39.000000000 +0200
> > +++ b/net/sunrpc/clnt.c	2007-07-05 15:33:11.000000000 +0200
> > @@ -209,7 +209,8 @@ struct rpc_clnt *rpc_create(struct rpc_c
> >  	struct rpc_clnt *clnt;
> >  
> >  	xprt = xprt_create_transport(args->protocol, args->address,
> > -					args->addrsize, args->timeout);
> > +				     args->addrsize, args->saddress,
> > +				     args->timeout);
> >  	if (IS_ERR(xprt))
> >  		return (struct rpc_clnt *)xprt;
> >  
> > diff -urp a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > --- a/net/sunrpc/xprt.c	2007-04-27 18:05:39.000000000 +0200
> > +++ b/net/sunrpc/xprt.c	2007-07-05 15:33:11.000000000 +0200
> > @@ -893,17 +893,19 @@ void xprt_set_timeout(struct rpc_timeout
> >   * @to: timeout parameters
> >   *
> >   */
> > -struct rpc_xprt *xprt_create_transport(int proto, struct sockaddr *ap, size_t size, struct rpc_timeout *to)
> > +struct rpc_xprt *xprt_create_transport(int proto, struct sockaddr *ap,
> > +				       size_t size, struct sockaddr *saddr,
> > +				       struct rpc_timeout *to)
> 
> Urgh... It might be time to fold all these arguments into a "struct
> rpc_xprtsock_create" instead of copying them on the stack.

Can be done before or after this patch. Or combined, just tell me.

> 
> >  {
> >  	struct rpc_xprt	*xprt;
> >  	struct rpc_rqst	*req;
> >  
> >  	switch (proto) {
> >  	case IPPROTO_UDP:
> > -		xprt = xs_setup_udp(ap, size, to);
> > +		xprt = xs_setup_udp(ap, size, saddr, to);
> >  		break;
> >  	case IPPROTO_TCP:
> > -		xprt = xs_setup_tcp(ap, size, to);
> > +		xprt = xs_setup_tcp(ap, size, saddr, to);
> >  		break;
> >  	default:
> >  		printk(KERN_ERR "RPC: unrecognized transport protocol: %d\n",
> > diff -urp a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > --- a/net/sunrpc/xprtsock.c	2007-04-27 18:05:39.000000000 +0200
> > +++ b/net/sunrpc/xprtsock.c	2007-07-05 15:39:29.000000000 +0200
> > @@ -235,6 +235,7 @@ struct sock_xprt {
> >  	 * Connection of transports
> >  	 */
> >  	struct delayed_work	connect_worker;
> > +	struct sockaddr_storage	saddr;
> >  	unsigned short		port;
> >  
> >  	/*
> > @@ -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?

ok, that would be better.

> 
> >  
> > +	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 though you said this was IPv6 clean :-)

Only the server socket handling code. The other files are not IPv6 clean
to begin with (I have some ideas for this).

> 
> >  	return err;
> >  }
> >  
> > @@ -1229,7 +1232,7 @@ static void xs_udp_connect_worker(struct
> >  	}
> >  	xs_reclassify_socket(sock);
> >  
> > -	if (xprt->resvport && xs_bindresvport(transport, sock) < 0) {
> > +	if (xs_bind(transport, sock)) {
> >  		sock_release(sock);
> >  		goto out;
> >  	}
> > @@ -1316,7 +1319,7 @@ static void xs_tcp_connect_worker(struct
> >  		}
> >  		xs_reclassify_socket(sock);
> >  
> > -		if (xprt->resvport && xs_bindresvport(transport, sock) < 0) {
> > +		if (xs_bind(transport, sock)) {
> >  			sock_release(sock);
> >  			goto out;
> >  		}
> > @@ -1505,7 +1508,8 @@ static struct rpc_xprt_ops xs_tcp_ops = 
> >  	.print_stats		= xs_tcp_print_stats,
> >  };
> >  
> > -static struct rpc_xprt *xs_setup_xprt(struct sockaddr *addr, size_t addrlen, unsigned int slot_table_size)
> > +static struct rpc_xprt *xs_setup_xprt(struct sockaddr *addr, size_t addrlen,
> > +				      struct sockaddr *saddr, unsigned int slot_table_size)
> >  {
> >  	struct rpc_xprt *xprt;
> >  	struct sock_xprt *new;
> > @@ -1534,6 +1538,8 @@ static struct rpc_xprt *xs_setup_xprt(st
> >  
> >  	memcpy(&xprt->addr, addr, addrlen);
> >  	xprt->addrlen = addrlen;
> > +	if (saddr)
> > +		memcpy(&new->saddr, saddr, addrlen);
> 
> Is it safe to assume that the source and destination sockaddr structures
> are always the same size?

Well, yes unless there are plans for AF_UNIX support. Look into socket
getname(), e.g. inet_getname() and inet6_getname(): both addresses of
a connection must belong to the same address family.

> 
> >  	new->port = xs_get_random_port();
> >  
> >  	return xprt;
> > @@ -1546,12 +1552,13 @@ static struct rpc_xprt *xs_setup_xprt(st
> >   * @to:   timeout parameters
> >   *
> >   */
> > -struct rpc_xprt *xs_setup_udp(struct sockaddr *addr, size_t addrlen, struct rpc_timeout *to)
> > +struct rpc_xprt *xs_setup_udp(struct sockaddr *addr, size_t addrlen,
> > +			      struct sockaddr *saddr, struct rpc_timeout *to)
> >  {
> >  	struct rpc_xprt *xprt;
> >  	struct sock_xprt *transport;
> >  
> > -	xprt = xs_setup_xprt(addr, addrlen, xprt_udp_slot_table_entries);
> > +	xprt = xs_setup_xprt(addr, addrlen, saddr, xprt_udp_slot_table_entries);
> >  	if (IS_ERR(xprt))
> >  		return xprt;
> >  	transport = container_of(xprt, struct sock_xprt, xprt);
> > @@ -1591,12 +1598,13 @@ struct rpc_xprt *xs_setup_udp(struct soc
> >   * @to: timeout parameters
> >   *
> >   */
> > -struct rpc_xprt *xs_setup_tcp(struct sockaddr *addr, size_t addrlen, struct rpc_timeout *to)
> > +struct rpc_xprt *xs_setup_tcp(struct sockaddr *addr, size_t addrlen,
> > +			      struct sockaddr *saddr, struct rpc_timeout *to)
> >  {
> >  	struct rpc_xprt *xprt;
> >  	struct sock_xprt *transport;
> >  
> > -	xprt = xs_setup_xprt(addr, addrlen, xprt_tcp_slot_table_entries);
> > +	xprt = xs_setup_xprt(addr, addrlen, saddr, xprt_tcp_slot_table_entries);
> >  	if (IS_ERR(xprt))
> >  		return xprt;
> >  	transport = container_of(xprt, struct sock_xprt, xprt);

-- 
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