Re: [PATCH v10 10/23] pesto, conf: Have pesto connect to passt and check versions

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 6 May 2026 19:52:27 +0200
Paul Holzinger <pholzing@redhat.com> wrote:

> Hi,
> 
> so I was testing these patches and found one small "problem".
> 
> On 06/05/2026 15:23, Stefano Brivio wrote:
> > From: David Gibson <david@gibson.dropbear.id.au>
> >
> > Start implementing pesto in earnest.  Create a control/configuration
> > socket in passt.  Have pesto connect to it and retrieve a server greeting
> > Perform some basic version checking.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > [sbrivio: Avoid potential recursive calling between conf_accept() and
> >   conf_close(), reported by clang-tidy]
> > [sbrivio: In conf(), check we're not exceeding sizeof(c->control_path)
> >   instead of sizeof(c->socket_path), and, in pesto's main(), print
> >   argv[optind] instead of argv[1] to indicate an invalid socket path,
> >   both reported by Jon Maloy]
> > [sbrivio: In pesto's main(), drop unnecessary newline from error
> >   message, reported by Laurent]
> > [sbrivio: Don't use SOCK_NONBLOCK on accept4(), as that only applies
> >   to the *new* file descriptor, which we don't want -- set O_NONBLOCK
> >   on the listening file descriptor using fcntl()]
> > [sbrivio: Switch to protocol version 1, and reflect the true magic
> >   behind pesto, i.e. basil, into the magic string]
> > [sbrivio: Fix conflicts in the Makefile caused by the fact that I'm
> >   not merging a previous series reworking it]
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >   Makefile     |   2 +-
> >   conf.c       | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   conf.h       |   2 +
> >   epoll_type.h |   4 ++
> >   passt.1      |   5 ++
> >   passt.c      |   8 +++
> >   passt.h      |   6 ++
> >   pesto.c      |  47 ++++++++++++-
> >   pesto.h      |  22 ++++++
> >   serialise.c  |   3 +
> >   10 files changed, 279 insertions(+), 4 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 2639472..b1003d8 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -45,7 +45,7 @@ PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
> >   	vhost_user.c virtio.c vu_common.c
> >   QRAP_SRCS = qrap.c
> >   PASST_REPAIR_SRCS = passt-repair.c
> > -PESTO_SRCS = pesto.c
> > +PESTO_SRCS = pesto.c serialise.c
> >   SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
> >   
> >   MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
> > diff --git a/conf.c b/conf.c
> > index 27aded8..9eed1ec 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -48,6 +48,10 @@
> >   #include "isolation.h"
> >   #include "log.h"
> >   #include "vhost_user.h"
> > +#include "epoll_ctl.h"
> > +#include "conf.h"
> > +#include "pesto.h"
> > +#include "serialise.h"
> >   
> >   #define NETNS_RUN_DIR	"/run/netns"
> >   
> > @@ -541,6 +545,7 @@ static void usage(const char *name, FILE *f, int status)
> >   		"  --runas UID|UID:GID 	Run as given UID, GID, which can be\n"
> >   		"    numeric, or login and group names\n"
> >   		"    default: drop to user \"nobody\"\n"
> > +		"  -c, --conf-path PATH	Configuration socket path\n"
> >   		"  -h, --help		Display this help message and exit\n"
> >   		"  --version		Show version and exit\n");
> >   
> > @@ -779,6 +784,9 @@ static void conf_print(const struct ctx *c)
> >   	char buf[INANY_ADDRSTRLEN];
> >   	int i;
> >   
> > +	if (c->fd_control_listen >= 0)
> > +		info("Configuration socket: %s", c->control_path);
> > +
> >   	if (c->ifi4 > 0 || c->ifi6 > 0) {
> >   		char ifn[IFNAMSIZ];
> >   
> > @@ -1072,6 +1080,19 @@ static void conf_open_files(struct ctx *c)
> >   		if (c->pidfile_fd < 0)
> >   			die_perror("Couldn't open PID file %s", c->pidfile);
> >   	}
> > +
> > +	c->fd_control = -1;
> > +	if (*c->control_path) {
> > +		c->fd_control_listen = sock_unix(c->control_path);
> > +		if (c->fd_control_listen < 0) {
> > +			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;
> > +	}
> >   }
> >   
> >   /**
> > @@ -1107,6 +1128,25 @@ fail:
> >   	die("Invalid MAC address: %s", str);
> >   }
> >   
> > +/**
> > + * conf_sock_listen() - Start listening for connections on configuration socket
> > + * @c:		Execution context
> > + */
> > +static void conf_sock_listen(const struct ctx *c)
> > +{
> > +	union epoll_ref ref = { .type = EPOLL_TYPE_CONF_LISTEN };
> > +
> > +	if (c->fd_control_listen < 0)
> > +		return;
> > +
> > +	if (listen(c->fd_control_listen, 0))
> > +		die_perror("Couldn't listen on configuration socket");
> > +
> > +	ref.fd = c->fd_control_listen;
> > +	if (epoll_add(c->epollfd, EPOLLIN | EPOLLET, ref))
> > +		die_perror("Couldn't add configuration socket to epoll");
> > +}
> > +
> >   /**
> >    * conf() - Process command-line arguments and set configuration
> >    * @c:		Execution context
> > @@ -1189,9 +1229,10 @@ void conf(struct ctx *c, int argc, char **argv)
> >   		{"migrate-exit", no_argument,		NULL,		29 },
> >   		{"migrate-no-linger", no_argument,	NULL,		30 },
> >   		{"stats", required_argument,		NULL,		31 },
> > +		{"conf-path",	required_argument,	NULL,		'c' },
> >   		{ 0 },
> >   	};
> > -	const char *optstring = "+dqfel:hs:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
> > +	const char *optstring = "+dqfel:hs:c:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
> >   	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> >   	bool opt_t = false, opt_T = false, opt_u = false, opt_U = false;
> >   	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
> > @@ -1449,6 +1490,13 @@ void conf(struct ctx *c, int argc, char **argv)
> >   
> >   			c->fd_tap = -1;
> >   			break;
> > +		case 'c':
> > +			ret = snprintf(c->control_path, sizeof(c->control_path),
> > +				       "%s", optarg);
> > +			if (ret <= 0 || ret >= (int)sizeof(c->control_path))
> > +				die("Invalid configuration path: %s", optarg);
> > +			c->fd_control_listen = c->fd_control = -1;
> > +			break;
> >   		case 'F':
> >   			errno = 0;
> >   			fd_tap_opt = strtol(optarg, NULL, 0);
> > @@ -1871,6 +1919,140 @@ void conf(struct ctx *c, int argc, char **argv)
> >   			fwd_rule_parse('U', "auto", c->fwd[PIF_SPLICE]);
> >   	}
> >   
> > +	conf_sock_listen(c);
> > +
> >   	if (!c->quiet)
> >   		conf_print(c);
> >   }
> > +
> > +static void conf_accept(struct ctx *c);
> > +
> > +/**
> > + * conf_close() - Close configuration / control socket and clean up
> > + * @c:		Execution context
> > + */
> > +static void conf_close(struct ctx *c)
> > +{
> > +	debug("Closing configuration socket");
> > +	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_control, NULL);
> > +	close(c->fd_control);
> > +	c->fd_control = -1;
> > +}
> > +
> > +/**
> > + * conf_listen_handler() - Handle events on configuration listening socket
> > + * @c:		Execution context
> > + * @events:	epoll events
> > + */
> > +void conf_listen_handler(struct ctx *c, uint32_t events)
> > +{
> > +	if (events != EPOLLIN) {
> > +		err("Unexpected event 0x%04x on configuration socket", events);
> > +		return;
> > +	}
> > +
> > +	if (c->fd_control >= 0) {
> > +		/* Ignore the new connection for now, blocking it until the
> > +		 * current one finishes.
> > +		 */
> > +		return;
> > +	}
> > +
> > +	conf_accept(c);
> > +}
> > +
> > +/**
> > + * conf_accept() - Accept a new control connection
> > + * @c:		Execution context
> > + */
> > +static void conf_accept(struct ctx *c)
> > +{
> > +	struct pesto_hello hello = {
> > +		.magic = PESTO_SERVER_MAGIC,
> > +		.version = htonl(PESTO_PROTOCOL_VERSION),
> > +	};
> > +	union epoll_ref ref = { .type = EPOLL_TYPE_CONF };
> > +	struct ucred uc = { 0 };
> > +	socklen_t len = sizeof(uc);
> > +	int fd, rc;
> > +
> > +retry:
> > +	err("%s: %i", __func__, __LINE__);
> > +	fd = accept4(c->fd_control_listen, NULL, NULL, SOCK_CLOEXEC);
> > +	if (fd < 0) {
> > +		err("%s: %i", __func__, __LINE__);
> > +		if (errno != EAGAIN)
> > +			warn_perror("accept4() on configuration listening socket");
> > +		return;
> > +	}
> > +
> > +	err("%s: %i", __func__, __LINE__);  
> I assume the three err() calls are debug leftovers? I was wondering why 
> my journal was getting spammed with "conf_accept: XXX".

Oops, definitely left-overs, I spotted those as well and I was fairly
sure I dropped them but here they are again... fixing in v11.

-- 
Stefano