Re: [PATCH 3/4] flow: Safer errno handling in flowside_connect() callers

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

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

On Wed, Jun 17, 2026 at 01:09:18AM +0200, Stefano Brivio wrote:
> On Tue,  9 Jun 2026 12:32:25 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > flowside_connect() behaves much like connect(2) itself, returning -1 on
> > error with errno set to the error code.  One of the callers, in
> > udp_flow_sock(), uses the errno code with flow_dbg_perror() *after* it's
> > called epoll_del() and close() either of which could clobber errno.
> > 
> > Change flowside_connect() to use the more regular convention for internal
> > functions: return a negative errno code on error, rather than just -1.
> > Save it in the callers and use that rather than raw errno to print the
> > message.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.c     | 6 ++++--
> >  tcp.c      | 4 ++--
> >  udp_flow.c | 7 +++----
> >  3 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/flow.c b/flow.c
> > index dd92bad7..98828430 100644
> > --- a/flow.c
> > +++ b/flow.c
> > @@ -259,7 +259,7 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
> >   *
> >   * Connect @s to the endpoint address and port from @tgt.
> >   *
> > - * Return: 0 on success, negative on error
> > + * Return: 0 on success, negative error code on error
> >   */
> >  int flowside_connect(const struct ctx *c, int s,
> >  		     uint8_t pif, const struct flowside *tgt)
> > @@ -267,7 +267,9 @@ int flowside_connect(const struct ctx *c, int s,
> >  	union sockaddr_inany sa;
> >  
> >  	pif_sockaddr(c, &sa, pif, &tgt->eaddr, tgt->eport);
> > -	return connect(s, &sa.sa, socklen_inany(&sa));
> > +	if (connect(s, &sa.sa, socklen_inany(&sa)) < 0)
> > +		return -errno;
> > +	return 0;
> 
> This looks like a good idea nevertheless, but:
> 
> >  }
> >  
> >  /** flow_log__ - Log flow-related message, internal helper
> > diff --git a/tcp.c b/tcp.c
> > index 6fba865f..81813643 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -3744,8 +3744,8 @@ static int tcp_flow_repair_connect(const struct ctx *c,
> >  
> >  	rc = flowside_connect(c, conn->sock, PIF_HOST, tgt);
> >  	if (rc) {
> > -		rc = -errno;
> > -		flow_perror(conn, "Failed to connect migrated socket");
> > +		flow_err(conn, "Failed to connect migrated socket: %s",
> > +			 strerror_(-rc));
> 
> ...wouldn't it be more convenient to establish that flowside_connect()
> behaves like connect() also by setting errno (by updating the comment
> to flowside_connect() accordingly) and use that fact here to keep
> calling flow_perror(), so that we don't need to open code the
> strerror_() call?

I was working on the basis that returning an error code is a more
common convention for internal functions than setting errno.  But..

> 
> >  		return rc;
> >  	}
> >  
> > diff --git a/udp_flow.c b/udp_flow.c
> > index 35417bc4..6edfa65a 100644
> > --- a/udp_flow.c
> > +++ b/udp_flow.c
> > @@ -88,13 +88,12 @@ static int udp_flow_sock(const struct ctx *c,
> >  		return rc;
> >  	}
> >  
> > -	if (flowside_connect(c, s, pif, side) < 0) {
> > -		rc = -errno;
> > -
> > +	if ((rc = flowside_connect(c, s, pif, side)) < 0) {
> >  		epoll_del(flow_epollfd(&uflow->f), s);
> >  		close(s);
> >  
> > -		flow_dbg_perror(uflow, "Couldn't connect flow socket");
> > +		flow_dbg(uflow, "Couldn't connect flow socket: %s",
> > +			 strerror_(-rc));
> 
> For the same reason, couldn't we just move the existing
> flow_dbg_perror() call before epoll_del() instead?

..duh.  That's a much easier way of solving the problem.  I'll do that
instead.

> 
> >  		return rc;
> >  	}
> >  	uflow->s[sidei] = s;
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| 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 --]