On Fri, Dec 3, 2021 at 11:10 AM David Wysochanski <dwysocha@xxxxxxxxxx> wrote:
>
> On Fri, Oct 29, 2021 at 4:11 PM <trondmy@xxxxxxxxxx> wrote:
> >
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > Move the error handling into a single switch statement for clarity.
> >
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > net/sunrpc/xprtsock.c | 68 ++++++++++++++++++-------------------------
> > 1 file changed, 28 insertions(+), 40 deletions(-)
> >
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index 291b63136c08..7fb302e202bc 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt,
> > static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> > {
> > struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
> > - int ret = -ENOTCONN;
> >
> > if (!transport->inet) {
> > struct sock *sk = sock->sk;
> > @@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> > }
> >
> > if (!xprt_bound(xprt))
> > - goto out;
> > + return -ENOTCONN;
> >
> > xs_set_memalloc(xprt);
> >
> > @@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> >
> > /* Tell the socket layer to start connecting... */
> > set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
> > - ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
> > - switch (ret) {
> > - case 0:
> > - xs_set_srcport(transport, sock);
> > - fallthrough;
> > - case -EINPROGRESS:
> > - /* SYN_SENT! */
> > - if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> > - xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> > - break;
> > - case -EADDRNOTAVAIL:
> > - /* Source port number is unavailable. Try a new one! */
> > - transport->srcport = 0;
> > - }
> > -out:
> > - return ret;
> > + return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
> > }
> >
> > /**
> > @@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> > container_of(work, struct sock_xprt, connect_worker.work);
> > struct socket *sock = transport->sock;
> > struct rpc_xprt *xprt = &transport->xprt;
> > - int status = -EIO;
> > + int status;
> >
> > if (!sock) {
> > sock = xs_create_sock(xprt, transport,
> > xs_addr(xprt)->sa_family, SOCK_STREAM,
> > IPPROTO_TCP, true);
> > if (IS_ERR(sock)) {
> > - status = PTR_ERR(sock);
> > + xprt_wake_pending_tasks(xprt, PTR_ERR(sock));
> > goto out;
> > }
> > }
> > @@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> > xprt, -status, xprt_connected(xprt),
> > sock->sk->sk_state);
> > switch (status) {
> > - default:
> > - printk("%s: connect returned unhandled error %d\n",
> > - __func__, status);
> > - fallthrough;
> > - case -EADDRNOTAVAIL:
> > - /* We're probably in TIME_WAIT. Get rid of existing socket,
> > - * and retry
> > - */
> > - xs_tcp_force_close(xprt);
> > - break;
> > case 0:
> > + xs_set_srcport(transport, sock);
> > + fallthrough;
> > case -EINPROGRESS:
> > + /* SYN_SENT! */
> > + if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> > + xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> > + fallthrough;
> > case -EALREADY:
> > - xprt_unlock_connect(xprt, transport);
> > - return;
> > + goto out_unlock;
> > + case -EADDRNOTAVAIL:
> > + /* Source port number is unavailable. Try a new one! */
> > + transport->srcport = 0;
> > + status = -EAGAIN;
> > + break;
> > case -EINVAL:
> > /* Happens, for instance, if the user specified a link
> > * local IPv6 address without a scope-id.
> > @@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> > case -EHOSTUNREACH:
> > case -EADDRINUSE:
> > case -ENOBUFS:
> > - /* xs_tcp_force_close() wakes tasks with a fixed error code.
> > - * We need to wake them first to ensure the correct error code.
> > - */
> > - xprt_wake_pending_tasks(xprt, status);
> > - xs_tcp_force_close(xprt);
> > - goto out;
> > + break;
> > + default:
> > + printk("%s: connect returned unhandled error %d\n",
> > + __func__, status);
> > + status = -EAGAIN;
> > }
> > - status = -EAGAIN;
> > +
> > + /* xs_tcp_force_close() wakes tasks with a fixed error code.
> > + * We need to wake them first to ensure the correct error code.
> > + */
> > + xprt_wake_pending_tasks(xprt, status);
> > + xs_tcp_force_close(xprt);
>
> Isn't this a problem to call xs_tcp_force_close() unconditionally at
> the bottom of xs_tcp_setup_socket()?
> Before this patch we only called this depending on certain returns
> from kernel_connect().
>
Nevermind - I see the fallthroughs and out_unlock
>
>
> > out:
> > xprt_clear_connecting(xprt);
> > +out_unlock:
> > xprt_unlock_connect(xprt, transport);
> > - xprt_wake_pending_tasks(xprt, status);
> > }
> >
> > /**
> > --
> > 2.31.1
> >
