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

[Stefano Brivio <sbrivio@redhat.com>]

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?

>  		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?

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

-- 
Stefano