Skip to contentSkip to conversation

SUNRPC client: add interface for binding to a local address

PATCH In review Frank van Maarseveen 3 participants 6 messages Jul 5, 2007 – Jul 6, 2007

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

Frank van Maarseveen

author
#0 lore raw

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

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 <[email protected]>

---

include/linux/sunrpc/clnt.h +1 -0
diff -urp a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
@@ -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;
include/linux/sunrpc/xprt.h +6 -3
diff -urp a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
@@ -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
net/sunrpc/clnt.c +2 -1
diff -urp a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
@@ -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;
net/sunrpc/xprt.c +5 -3
diff -urp a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
@@ -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)
 {
 	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",
net/sunrpc/xprtsock.c +23 -15
diff -urp a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
@@ -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;
 
+	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);
 	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);
 	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);
Signature · 10 lines
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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#1 lore raw
Quoted 61 lines · Frank van Maarseveen

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 <[email protected]>

---

diff -urp a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h

include/linux/sunrpc/clnt.h +1 -0
@@ -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

include/linux/sunrpc/xprt.h +6 -3
@@ -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

net/sunrpc/clnt.c +2 -1
@@ -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

net/sunrpc/xprt.c void xprt_set_timeout(struct rpc_timeout · line 898

Quoted 7 lines
net/sunrpc/xprt.c +3 -1
@@ -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.

Quoted 29 lines
net/sunrpc/xprt.c +2 -2
 {
 	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

net/sunrpc/xprtsock.c +1 -0
@@ -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);
 }

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1157

Quoted 10 lines
net/sunrpc/xprtsock.c +2 -1
 
-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?

Quoted 16 lines
net/sunrpc/xprtsock.c +6 -5
+	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;

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1176

Quoted 10 lines
net/sunrpc/xprtsock.c +2 -2
 		}
 		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 :-)

Quoted 32 lines
net/sunrpc/xprtsock.c +4 -3
 	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;

net/sunrpc/xprtsock.c static struct rpc_xprt *xs_setup_xprt(st · line 1542

Quoted 5 lines
net/sunrpc/xprtsock.c +2 -0
@@ -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?

Quoted 25 lines
net/sunrpc/xprtsock.c +4 -3
 	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,

net/sunrpc/xprtsock.c struct rpc_xprt *xs_setup_udp(struct soc · line 1610

Quoted 10 lines
net/sunrpc/xprtsock.c +2 -1
+			      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);

-------------------------------------------------------------------------
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#2 lore raw
Quoted 58 lines · Trond Myklebust
Quoted 57 lines · Frank van Maarseveen

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 <[email protected]>

---

diff -urp a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h

include/linux/sunrpc/clnt.h +1 -0
@@ -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

include/linux/sunrpc/xprt.h +6 -3
@@ -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

net/sunrpc/clnt.c +2 -1
@@ -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;
Quoted 14 lines

net/sunrpc/xprt.c void xprt_set_timeout(struct rpc_timeout · line 898

Quoted 11 lines

diff -urp a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c

net/sunrpc/xprt.c +3 -1
@@ -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.

Quoted 31 lines
Quoted 31 lines
net/sunrpc/xprt.c +2 -2
 {
 	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

net/sunrpc/xprtsock.c +1 -0
@@ -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);
 }
Quoted 12 lines

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1157

Quoted 9 lines
net/sunrpc/xprtsock.c +2 -1
-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.

Quoted 21 lines
Quoted 21 lines
net/sunrpc/xprtsock.c +6 -5
+	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
Quoted 8 lines

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1176

Quoted 6 lines
net/sunrpc/xprtsock.c +2 -2
 			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).

Quoted 34 lines
Quoted 34 lines
net/sunrpc/xprtsock.c +4 -3
 	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
Quoted 7 lines

net/sunrpc/xprtsock.c static struct rpc_xprt *xs_setup_xprt(st · line 1542

Quoted 4 lines
net/sunrpc/xprtsock.c +2 -0
 	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.

Quoted 36 lines
Quoted 36 lines
net/sunrpc/xprtsock.c +6 -4
 	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);
Signature · 10 lines
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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#3 lore raw
Quoted 2 lines · Frank van Maarseveen
Quoted 1 line · Trond Myklebust

On Thu, 2007-07-05 at 17:26 +0200, Frank van Maarseveen wrote:

<snip>

Quoted 9 lines
Quoted 7 lines

net/sunrpc/xprt.c void xprt_set_timeout(struct rpc_timeout · line 898

Quoted 4 lines
net/sunrpc/xprt.c +3 -1
-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.

It should definitely be a separate patch. Ideally we should perhaps do it before this patch since it is a nice cleanup that stands on its own merits.

<snip>

Quoted 8 lines
Quoted 5 lines

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1176

Quoted 3 lines
net/sunrpc/xprtsock.c +2 -1
-	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).

I think Chuck is working on this as part of his IPv6 patchsets.

<snip>

Quoted 6 lines
Quoted 2 lines

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.

OK.

Trond

-------------------------------------------------------------------------
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

#4 lore raw
In reply to Trond Myklebust
Quoted 5 lines · Trond Myklebust
Quoted 4 lines · Frank van Maarseveen
net/sunrpc/xprtsock.c +0 -0
@@ -1146,31 +1147,33 @@ static void xs_set_port(struct rpc_xprt 
 	sap->sin_port = htons(port);
 }
Quoted 12 lines

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1157

Quoted 9 lines
net/sunrpc/xprtsock.c +2 -1
-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.

Quoted 20 lines
Quoted 20 lines
net/sunrpc/xprtsock.c +6 -5
+	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
Quoted 8 lines

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1176

Quoted 6 lines
net/sunrpc/xprtsock.c +2 -2
 			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?

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

-------------------------------------------------------------------------
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#5 lore raw
Quoted 20 lines · Chuck Lever
Quoted 5 lines · Trond Myklebust
Quoted 4 lines · Frank van Maarseveen
net/sunrpc/xprtsock.c +0 -0
@@ -1146,31 +1147,33 @@ static void xs_set_port(struct rpc_xprt 
 	sap->sin_port = htons(port);
 }
Quoted 13 lines

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1157

Quoted 10 lines
net/sunrpc/xprtsock.c +2 -1
-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?

Quoted 6 lines

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]

Quoted 11 lines
Quoted 5 lines

net/sunrpc/xprtsock.c static void xs_set_port(struct rpc_xprt · line 1176

Quoted 3 lines
net/sunrpc/xprtsock.c +2 -1
-	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.

Signature · 10 lines
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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs