Re: [PATCH 2/3] conf, tap, repair: Uniformly use non-blocking accept() on Unix sockets

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

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

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 pending
> connection request.
> 
> Control connections (pesto) are an exception, because the way we queue
> connections requires that we call accept() when we close one connection to
> see if there's another one waiting.  We rely on an EAGAIN here to know that
> there's nothing waiting.  To handle these we have an explicit fcntl() to
> enable NONBLOCK on the control listening socket.
> 
> However, always using non-blocking accept() for Unix sockets would make
> things a bit more uniform, and should be a bit less fragile in the case
> 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.

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).

> we will need non-blocking accept() for the upcoming control/configuration
> socket.  Always add SOCK_NONBLOCK, which is more robust and in keeping with
> the normal non-blocking style of passt.

Oops.  This paragraph is left over from a previous version.  Can you
remove on merge, if there's no other reason to respin?

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c   | 4 +---
>  repair.c | 4 ++--
>  tap.c    | 5 +++++
>  util.c   | 2 +-
>  4 files changed, 9 insertions(+), 6 deletions(-)
> 
> 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 = -1;
>  	}
> @@ -2087,7 +2085,7 @@ retry:
>  	fd = accept4(c->fd_control_listen, NULL, NULL, SOCK_CLOEXEC);
>  	if (fd < 0) {
>  		if (errno != EAGAIN)
> -			warn_perror("accept4() on configuration listening socket");
> +			warn_perror("Error accept()ing configuration socket");
>  		return;
>  	}
>  
> 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)
>  
>  	if ((c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL,
>  				    SOCK_CLOEXEC)) < 0) {
> -		rc = errno;
> -		debug_perror("accept4() on TCP_REPAIR helper listening socket");
> +		if ((rc = errno) != EAGAIN)
> +			warn_perror("Error accept()ing repair helper");
>  		return rc;
>  	}
>  
> diff --git a/tap.c b/tap.c
> index e7cac9df..fda2da9b 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1491,6 +1491,11 @@ void tap_listen_handler(struct ctx *c, uint32_t events)
>  	}
>  
>  	c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, SOCK_CLOEXEC);
> +	if (c->fd_tap < 0) {
> +		if (errno != EAGAIN)
> +			warn_perror("Error accepting tap client");
> +		return;
> +	}
>  
>  	if (!getsockopt(c->fd_tap, SOL_SOCKET, SO_PEERCRED, &ucred, &len))
>  		info("accepted connection from PID %i", ucred.pid);
> diff --git a/util.c b/util.c
> index 73c9d51d..204391c7 100644
> --- a/util.c
> +++ b/util.c
> @@ -238,7 +238,7 @@ int sock_l4_dualstack_any(const struct ctx *c, enum epoll_type type,
>   */
>  int sock_unix(char *sock_path)
>  {
> -	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
> +	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0);
>  	struct sockaddr_un addr = {
>  		.sun_family = AF_UNIX,
>  	};
> -- 
> 2.54.0
> 

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