by Pavel Emelyanov

[permalink] [raw]

Subject: Re: [PATCH 0/13] sunrpc: Compact the xprtsock code a bit

>>> I'm assuming Trond is taking these. Let me know if that doesn't
>>> work....
>>
>> When will these appear in your three then? The thing is my next patches
>> depend on this set.
>
> If Trond ACK's them I'd happily merge them into my tree.
>
> (Or Trond could merge them, then one of us could merge the other's tree,
> and you could work on top of that.)

OK. Then I'll wait for Trond's decision.

> --b.
> .
>

2010-10-06 03:06:03

by Chuck Lever III

[permalink] [raw]

Subject: Re: [PATCH v2 9/13] sunrpc: Merge the xs_bind code

On Oct 5, 2010, at 7:53 AM, Pavel Emelyanov wrote:

> There's the only difference betseen the xs_bind4 and the
> xs_bind6 - the size of sockaddr structure they use.
>
> Fortunatelly its size can be indirectly get from the transport.
>
> Change since v1:
> * use sockaddr_storage instead of sockaddr
> * use rpc_set_port instead of manual port assigning
>
> Signed-off-by: Pavel Emelyanov <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 64 +++++++++++++------------------------------------
> 1 files changed, 17 insertions(+), 47 deletions(-)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7fdf2bb..bab808f 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1534,23 +1534,18 @@ static unsigned short xs_next_srcport(struct sock_xprt *transport, unsigned shor
> return xprt_max_resvport;
> return --port;
> }
> -
> -static int xs_bind4(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;
> + struct sockaddr_storage myaddr;
> int err, nloop = 0;
> unsigned short port = xs_get_srcport(transport);
> unsigned short last;
>
> - sa = (struct sockaddr_in *)&transport->srcaddr;
> - myaddr.sin_addr = sa->sin_addr;
> + memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
> do {
> - myaddr.sin_port = htons(port);
> - err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> - sizeof(myaddr));
> + rpc_set_port((struct sockaddr *)&myaddr, port);
> + err = kernel_bind(sock, (struct sockaddr *)&myaddr,
> + transport->xprt.addrlen);

That looks reasonable, and probably even improves your ratio of deletions to additions.

For the series,

Reviewed-by: Chuck Lever <[email protected]>

> if (port == 0)
> break;
> if (err == 0) {
> @@ -1562,44 +1557,19 @@ static int xs_bind4(struct sock_xprt *transport, struct socket *sock)
> if (port > last)
> nloop++;
> } while (err == -EADDRINUSE && nloop != 2);
> - dprintk("RPC: %s %pI4:%u: %s (%d)\n",
> - __func__, &myaddr.sin_addr,
> - port, err ? "failed" : "ok", err);
> - return err;
> -}
> -
> -static int xs_bind6(struct sock_xprt *transport, struct socket *sock)
> -{
> - struct sockaddr_in6 myaddr = {
> - .sin6_family = AF_INET6,
> - };
> - struct sockaddr_in6 *sa;
> - int err, nloop = 0;
> - unsigned short port = xs_get_srcport(transport);
> - unsigned short last;
>
> - sa = (struct sockaddr_in6 *)&transport->srcaddr;
> - myaddr.sin6_addr = sa->sin6_addr;
> - do {
> - myaddr.sin6_port = htons(port);
> - err = kernel_bind(sock, (struct sockaddr *) &myaddr,
> - sizeof(myaddr));
> - if (port == 0)
> - break;
> - if (err == 0) {
> - transport->srcport = port;
> - break;
> - }
> - last = port;
> - port = xs_next_srcport(transport, port);
> - if (port > last)
> - nloop++;
> - } while (err == -EADDRINUSE && nloop != 2);
> - dprintk("RPC: xs_bind6 %pI6:%u: %s (%d)\n",
> - &myaddr.sin6_addr, port, err ? "failed" : "ok", err);
> + if (myaddr.ss_family == PF_INET)
> + dprintk("RPC: %s %pI4:%u: %s (%d)\n", __func__,
> + &((struct sockaddr_in *)&myaddr)->sin_addr,
> + port, err ? "failed" : "ok", err);
> + else
> + dprintk("RPC: %s %pI6:%u: %s (%d)\n", __func__,
> + &((struct sockaddr_in6 *)&myaddr)->sin6_addr,
> + port, err ? "failed" : "ok", err);
> return err;
> }
>
> +
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> static struct lock_class_key xs_key[2];
> static struct lock_class_key xs_slock_key[2];
> @@ -1645,7 +1615,7 @@ static struct socket *xs_create_sock4(struct rpc_xprt *xprt,
> }
> xs_reclassify_socket4(sock);
>
> - if (xs_bind4(transport, sock)) {
> + if (xs_bind(transport, sock)) {
> sock_release(sock);
> goto out;
> }
> @@ -1669,7 +1639,7 @@ static struct socket *xs_create_sock6(struct rpc_xprt *xprt,
> }
> xs_reclassify_socket6(sock);
>
> - if (xs_bind6(transport, sock)) {
> + if (xs_bind(transport, sock)) {
> sock_release(sock);
> goto out;
> }
> --
> 1.5.5.6
>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com