Re: [PATCH v2 09/22] tcp_splice: Simplify clean up logic

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3010 bytes --]

On Sun, Feb 18, 2024 at 09:59:37PM +0100, Stefano Brivio wrote:
> On Tue,  6 Feb 2024 12:17:21 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently tcp_splice_flow_defer() contains specific logic to determine if
> > we're far enough initialised that we need to close pipes and/or sockets.
> > This is potentially fragile if we change something about the order in which
> > we do things.  We can simplify this by initialising the pipe and socket
> > fields to -1 very early, then close()ing them if and only if they're non
> > negative.
> > 
> > This lets us remove a special case cleanup if our connect() fails.  This
> > will already trigger a CLOSING event, and the socket fd in question is
> > populated in the connection structure.  Thus we can let the new cleanup
> > logic handle it rather than requiring an explicit close().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp_splice.c | 27 +++++++++++----------------
> >  1 file changed, 11 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tcp_splice.c b/tcp_splice.c
> > index 40ecb5d4..f0343eb5 100644
> > --- a/tcp_splice.c
> > +++ b/tcp_splice.c
> > @@ -246,16 +246,14 @@ bool tcp_splice_flow_defer(union flow *flow)
> >  		return false;
> >  
> >  	for (side = 0; side < SIDES; side++) {
> > -		if (conn->events & SPLICE_ESTABLISHED) {
> > -			/* Flushing might need to block: don't recycle them. */
> > -			if (conn->pipe[side][0] != -1) {
> > -				close(conn->pipe[side][0]);
> > -				close(conn->pipe[side][1]);
> > -				conn->pipe[side][0] = conn->pipe[side][1] = -1;
> > -			}
> > +		/* Flushing might need to block: don't recycle them. */
> > +		if (conn->pipe[side][0] >= 0) {
> > +			close(conn->pipe[side][0]);
> > +			close(conn->pipe[side][1]);
> > +			conn->pipe[side][0] = conn->pipe[side][1] = -1;
> >  		}
> >  
> > -		if (side == 0 || conn->events & SPLICE_CONNECT) {
> > +		if (conn->s[side] >= 0) {
> >  			close(conn->s[side]);
> >  			conn->s[side] = -1;
> >  		}
> > @@ -284,8 +282,6 @@ static int tcp_splice_connect_finish(const struct ctx *c,
> >  	int i = 0;
> >  
> >  	for (side = 0; side < SIDES; side++) {
> > -		conn->pipe[side][0] = conn->pipe[side][1] = -1;
> > -
> >  		for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) {
> >  			if (splice_pipe_pool[i][0] >= 0) {
> >  				SWAP(conn->pipe[side][0],
> > @@ -361,12 +357,8 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn,
> >  	}
> >  
> >  	if (connect(conn->s[1], sa, sl)) {
> > -		if (errno != EINPROGRESS) {
> > -			int ret = -errno;
> > -
> > -			close(sock_conn);
> > -			return ret;
> > -		}
> > +		if (errno != EINPROGRESS)
> > +			return -errno;
> 
> Nit: a newline here would be nice.

Done, thanks.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]