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=202602 header.b=mle6C7Jc; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 45BD25A0265 for ; Mon, 18 May 2026 04:54:53 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1779072890; bh=gk2ubbGJ0FxcQDULXh0kUTCBcfAk6kv7/gAe5PBPJJU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mle6C7JcsrXociVfAH2lm0IUNhD0z5xegfOecpdRgF9uoBbTKo4nj3n2el2SNFda8 GGTeyJh7IUaEUVEhVbMV9FqCLz4sv/2M6XO31D4CVTTtBNTaeC4iHPgen9UXcZWj2E taYyPOG/fjUkIsYMcT/32BImgzp8PN/BimZj9DNWrmjppecCcgOKlnluy/tPBC/eS6 PYmKjwqQXMD0EoFYUR1TDFEcRP01O7xnAbXJnJtGD8ffzR7KFTGY4MvTsGGvXWWpj0 +8+bgCx5TFZ7v4N6bBADByVxs2duEw6clnbi2QxF7vSUp1t3qwsHcvgzOAiTD6kDWB x86MgAW+oLDrA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gJj8B48sFz4wTL; Mon, 18 May 2026 12:54:50 +1000 (AEST) Date: Mon, 18 May 2026 12:40:55 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 2/3] conf, tap, repair: Uniformly use non-blocking accept() on Unix sockets Message-ID: References: <20260513041423.2446716-1-david@gibson.dropbear.id.au> <20260513041423.2446716-3-david@gibson.dropbear.id.au> <20260516174627.290aae6e@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="yWxj8PsQOV50xf/C" Content-Disposition: inline In-Reply-To: <20260516174627.290aae6e@elisabeth> Message-ID-Hash: 7ZUCKPRMRGVJGD2AI3RYZFIV4VVG63YO X-Message-ID-Hash: 7ZUCKPRMRGVJGD2AI3RYZFIV4VVG63YO 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: --yWxj8PsQOV50xf/C Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, May 16, 2026 at 05:46:27PM +0200, Stefano Brivio wrote: > On Wed, 13 May 2026 15:51:27 +1000 > David Gibson wrote: >=20 > > On Wed, May 13, 2026 at 02:14:22PM +1000, David Gibson wrote: > > > sock_unix(), which creates a listening Unix socket, doesn't set the > > > SOCK_NONBLOCK flag, meaning that accept() will block if called with no > > > pending connections. Generally, this doesn't matter because we only > > > accept() once we've received an epoll event indicating there's a pend= ing > > > connection request. > > >=20 > > > Control connections (pesto) are an exception, because the way we queue > > > connections requires that we call accept() when we close one connecti= on to > > > see if there's another one waiting. We rely on an EAGAIN here to kno= w that > > > there's nothing waiting. To handle these we have an explicit fcntl()= to > > > enable NONBLOCK on the control listening socket. > > >=20 > > > However, always using non-blocking accept() for Unix sockets would ma= ke > > > things a bit more uniform, and should be a bit less fragile in the ca= se > > > that we ever somehow got a spurious connection event. So, alter > > > sock_unix() to always use the SOCK_NONBLOCK flag. Remove the control > > > socket's special case fcntl(), and adjust the error handling on each > > > Unix socket accept() for the new behaviour. As a bonus the last adds > > > reporting for accept() errors on tap socket connections. =20 > >=20 > > I didn't realise it, but adding that reporting also removes a valid, > > if fairly minor coverity warning (at least with coverity 2026.3.0). > >=20 > > > we will need non-blocking accept() for the upcoming control/configura= tion > > > socket. Always add SOCK_NONBLOCK, which is more robust and in keepin= g with > > > the normal non-blocking style of passt. =20 > >=20 > > Oops. This paragraph is left over from a previous version. Can you > > remove on merge, if there's no other reason to respin? >=20 > I think the comments I'm raising (the one below and the one to 3/3) > actually warrant a respin at this point. Agreed. > > > Signed-off-by: David Gibson > > > --- > > > conf.c | 4 +--- > > > repair.c | 4 ++-- > > > tap.c | 5 +++++ > > > util.c | 2 +- > > > 4 files changed, 9 insertions(+), 6 deletions(-) > > >=20 > > > diff --git a/conf.c b/conf.c > > > index 029b9c7c..dec43fca 100644 > > > --- a/conf.c > > > +++ b/conf.c > > > @@ -1091,8 +1091,6 @@ static void conf_open_files(struct ctx *c) > > > die_perror("Couldn't open control socket %s", > > > c->control_path); > > > } > > > - if (fcntl(c->fd_control_listen, F_SETFL, O_NONBLOCK)) > > > - die_perror("Couldn't set O_NONBLOCK on control socket"); > > > } else { > > > c->fd_control_listen =3D -1; > > > } > > > @@ -2087,7 +2085,7 @@ retry: > > > fd =3D accept4(c->fd_control_listen, NULL, NULL, SOCK_CLOEXEC); > > > if (fd < 0) { > > > if (errno !=3D EAGAIN) > > > - warn_perror("accept4() on configuration listening socket"); > > > + warn_perror("Error accept()ing configuration socket"); > > > return; > > > } > > > =20 > > > diff --git a/repair.c b/repair.c > > > index 3e0e3e0a..42c4ae97 100644 > > > --- a/repair.c > > > +++ b/repair.c > > > @@ -101,8 +101,8 @@ int repair_listen_handler(struct ctx *c, uint32_t= events) > > > =20 > > > if ((c->fd_repair =3D accept4(c->fd_repair_listen, NULL, NULL, > > > SOCK_CLOEXEC)) < 0) { > > > - rc =3D errno; > > > - debug_perror("accept4() on TCP_REPAIR helper listening socket"); > > > + if ((rc =3D errno) !=3D EAGAIN) > > > + warn_perror("Error accept()ing repair helper"); >=20 > See repair_wait() for the reason why this particular listening socket > needs to be blocking (with a timeout). Ah, good point. That makes this whole patch pretty ill-conceived. I'll ditch everything except the error reporting addition. --=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 --yWxj8PsQOV50xf/C Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoKfDYACgkQzQJF27ox 2GcIlg/+M2s7J+aFXANfUylHbFVK3nfey92g7A0kLMA5iF10fr4+CuIR83iSwyXz c+HED4ywKkHfPb56t9TOfAX/85+sGOmb2IJbzxbzF4XCXcr5h+YmaZize2TBNvKT BPMFFUZVdIjGz3IXrITnntM9SZMnATImDydZ9SVJtAgmf9XfT3uvJyYCi1FFJc5+ ApbOzIzDgQdMdW8bQeJVRaW53t6GmQPhoZ0ak/S5Vc31O5Z2Lw8WKaAScZ+N4k5z MYYa3/dBEd7+IdbsUnLuifezMDH/B8U9WFlnNv9IS5NbJYW8gesdbZGiUjItr4PQ KFKyes10cUk4dE6XHWz2kmZxCq3IEvnhL3ZgEme6bXn41T5SXCSKDJ1yCGi0B4o5 L9KOuRfLh+yaRUGJOXnwGM3EvqHSRAT519titprOR3gGjoHIFAjd2gRm/hkt0OxB ZyMf4s/d9WZbkl8wN2KLgNiNgp02OU/lJaGtxqJ0h/SIMdqWJGgBdwmZdoWo8sBJ PaJAV1kApMP9ooNaWinSwuwI2j7QQ7eOI5cluKj0j9OM/NrXtoEPKMtNMKDmTrwj OyKRw/W3zDVrvBlxiYL2fMs4j/ddmfUdtQ4+/QNWSHidjjKJkNSvNfn6EJL2Y1w3 ln5IAR6L6TKNtYLHG9e8GV1XEqqIIayX2oc+XZ8fLyqFFP1ymX4= =anqH -----END PGP SIGNATURE----- --yWxj8PsQOV50xf/C--