[PATCH v2 02/11] tcp_splice: Correct error handling in tcp_splice_epoll_ctl()

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

If we get an error from epoll_ctl() in tcp_splice_epoll_ctl() we goto the
'delete' path where we remove both sockets from the epoll set and return
an error.  There are several problems with this:

- We 'return -errno' after the EPOLL_CTL_DEL operations, which means the
  deleting epoll_ctl() calls may have overwritten the errno values which
  actually triggered the failures.

- The call from conn_flag_do() occurs when the CLOSING flag is set, in
  which case we go do the delete path regardless of error.  In that case
  the 'return errno' is meaningless since we don't expect the EPOLL_CTL_DEL
  operations to fail and we ignore the return code anyway.

- All other calls to tcp_splice_epoll_ctl() check the return code and if
  non-zero immediately call conn_flag(..., CLOSING) which will call
  tcp_splice_epoll_ctl() again explicitly to remove the sockets from epoll.
  That means removing them when the error first occurs is redundant.

- We never specifically report an error on the epoll_ctl() operations.  We
  just set the connection to CLOSING, more or less silently killing it.
  This could make debugging difficult in the unlikely even that we get a
  failure here.

Re-organise tcp_splice_epoll_ctl() to just log a message then return in the
error case, and only EPOLL_CTL_DEL when explicitly asked to with the
CLOSING flag.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp_splice.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tcp_splice.c b/tcp_splice.c
index 87fbd50..6c4c664 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -182,25 +182,27 @@ static int tcp_splice_epoll_ctl(const struct ctx *c,
 	struct epoll_event ev_b = { .data.u64 = ref_b.u64 };
 	uint32_t events_a, events_b;
 
-	if (conn->flags & CLOSING)
-		goto delete;
+	if (conn->flags & CLOSING) {
+		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->a, &ev_a);
+		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->b, &ev_b);
+		return 0;
+	}
 
 	tcp_splice_conn_epoll_events(conn->events, &events_a, &events_b);
 	ev_a.events = events_a;
 	ev_b.events = events_b;
 
 	if (epoll_ctl(c->epollfd, m, conn->a, &ev_a) ||
-	    epoll_ctl(c->epollfd, m, conn->b, &ev_b))
-		goto delete;
+	    epoll_ctl(c->epollfd, m, conn->b, &ev_b)) {
+		int ret = -errno;
+		err("TCP (spliced): index %li, ERROR on epoll_ctl(): %s",
+		    CONN_IDX(conn), strerror(errno));
+		return ret;
+	}
 
 	conn->in_epoll = true;
 
 	return 0;
-
-delete:
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->a, &ev_a);
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->b, &ev_b);
-	return -errno;
 }
 
 /**
-- 
@@ -182,25 +182,27 @@ static int tcp_splice_epoll_ctl(const struct ctx *c,
 	struct epoll_event ev_b = { .data.u64 = ref_b.u64 };
 	uint32_t events_a, events_b;
 
-	if (conn->flags & CLOSING)
-		goto delete;
+	if (conn->flags & CLOSING) {
+		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->a, &ev_a);
+		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->b, &ev_b);
+		return 0;
+	}
 
 	tcp_splice_conn_epoll_events(conn->events, &events_a, &events_b);
 	ev_a.events = events_a;
 	ev_b.events = events_b;
 
 	if (epoll_ctl(c->epollfd, m, conn->a, &ev_a) ||
-	    epoll_ctl(c->epollfd, m, conn->b, &ev_b))
-		goto delete;
+	    epoll_ctl(c->epollfd, m, conn->b, &ev_b)) {
+		int ret = -errno;
+		err("TCP (spliced): index %li, ERROR on epoll_ctl(): %s",
+		    CONN_IDX(conn), strerror(errno));
+		return ret;
+	}
 
 	conn->in_epoll = true;
 
 	return 0;
-
-delete:
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->a, &ev_a);
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->b, &ev_b);
-	return -errno;
 }
 
 /**
-- 
2.41.0