From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=eq0K0QyZ; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 418735A026D for ; Wed, 17 Jun 2026 05:08:57 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1781665732; bh=KAn5reO/Ff0YvPBdbXVRD9IItbJifk2PUcORt0MBMnQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eq0K0QyZeMwdzKOoiLdVimXfSIOjlG1v2daFh92P9g16p8mTTz0DQ2ObuqzMSYrtv nWlh+3NiiNBDvbyS0mS5aBhqdKXut1sE2/k/3wECmZeC3kWwc8lDCvJp52c/pAIJh4 OAceU4YvUbr+3SJRP6SHXN9kxhQmx2E/Bnl0U1JK+GIldgw+CL9FEKmWCkUNlpuJxp 5ZtOqxr8NbdtkKLxKWQt+9m9P6vnO3BAP1Um4+OjwxxuU6nZgn5VJvA2t1OSEGLXnk TeOIqwmedwUCGlxt9quubuw7oKMGt8fdM7L7eyFp9isOqILIhDnLZJdXfAVYHg5ypG tb4nKSwOlETeQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gg82X6mvBz58ny; Wed, 17 Jun 2026 13:08:52 +1000 (AEST) Date: Wed, 17 Jun 2026 12:25:22 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/4] flow: Safer errno handling in flowside_connect() callers Message-ID: References: <20260609023226.86058-1-david@gibson.dropbear.id.au> <20260609023226.86058-4-david@gibson.dropbear.id.au> <20260617010918.12f447a5@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="q33FIECc+SbgVQbB" Content-Disposition: inline In-Reply-To: <20260617010918.12f447a5@elisabeth> Message-ID-Hash: 4O4AFXMFRZ2DYMQQU2QSVEEZJZ7QTFT3 X-Message-ID-Hash: 4O4AFXMFRZ2DYMQQU2QSVEEZJZ7QTFT3 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --q33FIECc+SbgVQbB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 17, 2026 at 01:09:18AM +0200, Stefano Brivio wrote: > On Tue, 9 Jun 2026 12:32:25 +1000 > David Gibson wrote: >=20 > > 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. > >=20 > > Change flowside_connect() to use the more regular convention for intern= al > > 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. > >=20 > > Signed-off-by: David Gibson > > --- > > flow.c | 6 ++++-- > > tcp.c | 4 ++-- > > udp_flow.c | 7 +++---- > > 3 files changed, 9 insertions(+), 8 deletions(-) > >=20 > > 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 epol= l_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; > > =20 > > 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; >=20 > This looks like a good idea nevertheless, but: >=20 > > } > > =20 > > /** 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 c= tx *c, > > =20 > > rc =3D flowside_connect(c, conn->sock, PIF_HOST, tgt); > > if (rc) { > > - rc =3D -errno; > > - flow_perror(conn, "Failed to connect migrated socket"); > > + flow_err(conn, "Failed to connect migrated socket: %s", > > + strerror_(-rc)); >=20 > ...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.. >=20 > > return rc; > > } > > =20 > > 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; > > } > > =20 > > - if (flowside_connect(c, s, pif, side) < 0) { > > - rc =3D -errno; > > - > > + if ((rc =3D flowside_connect(c, s, pif, side)) < 0) { > > epoll_del(flow_epollfd(&uflow->f), s); > > close(s); > > =20 > > - flow_dbg_perror(uflow, "Couldn't connect flow socket"); > > + flow_dbg(uflow, "Couldn't connect flow socket: %s", > > + strerror_(-rc)); >=20 > For the same reason, couldn't we just move the existing > flow_dbg_perror() call before epoll_del() instead? =2E.duh. That's a much easier way of solving the problem. I'll do that instead. >=20 > > return rc; > > } > > uflow->s[sidei] =3D s; >=20 > --=20 > Stefano >=20 --=20 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 --q33FIECc+SbgVQbB Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoyBYcACgkQzQJF27ox 2GcubxAAl9fOnm7uxQSuhiPFKErkiU4M+jIrCZ+TP+lLITMTh0/OSn2mCGny4Tzf Kzn2PLRCar6R3dzjNaQflIBAzybIV/sc4NRCoxKq9+K++nzq71/4HNl/n5qcNZIU N/NAtAheTG7lfJjACVic6usCerE2PIVxARwDgaStA3DmGPdRpDyDeNsr+UR0Ciex fHQ9NfBzIYEyVEN7v32Fmi/rYiZeIPY1raBY5H23+3GHKx435xFIotE55+tLN57T +JFT1Fe2axI1AOhxhMCLfSZR35Tbshx4sw+lhKL5pE4jg3i0Oavj/j6sFH+g9uya DvE3bv5OWQ1Fpf1la7CASyvLREvq9epiVgcMzeUi/0zWLEcq0VNLZwd0HZzLeyBj UPm3vXVIr1E7E3HyEXcWUiyFokEObSZQobbLMgYU13e77UtB07kMDFS2n6xu7sbe e2U20h5fQ8Xt6luszi1T0vK4q9Ik1lo4/kJb9PErn/S4SZb2Mwx2G1gHWf7F7t4J 1b7755V+txtxbSZbMzcq9tS2e8V4gE/1vaXwoPGZsopGqz4fonAHzVgIbGIgVyap YENiWnnqUUkFRnrhIBBpX33KZdely3oPOD4KqxaznjocCPtjK7e5c9VJy+rseS4i UNH2o5bsMYcbQKFRGOAqx3Wq8nE5tn/IBCCwbQMSgVtlke9xY6M= =z26e -----END PGP SIGNATURE----- --q33FIECc+SbgVQbB--