Re: [PATCH v6 11/18] pesto: Expose list of pifs to pesto and optionally display

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 4 May 2026 16:34:40 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 5/3/26 23:55, Stefano Brivio wrote:
> > From: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Extend the dynamic update protocol to expose the pif indices and names
> > from a running passt/pasta to the pesto tool.  pesto records that data
> > and, if requested with a new --show flag, prints it out.  
> 
> There is no --show flag

Ah, right, that comes later in the series now. I dropped that from the
commit message, and reworded the title.

> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > [sbrivio: In read_pif_conf(), force a redundant termination of the
> >   interface name, the existing check isn't obvious enough for static
> >   checkers]
> > [sbrivio: Drop @resv_ left-over in description of struct
> >   pesto_pif_info, reported by Jon Maloy]
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 
> But I've noticed 3 minor cosmetic flaws below.
> 
> 
> > ---
> >   Makefile    |   1 +
> >   common.h    |   2 +
> >   conf.c      |  41 ++++++++++++++++
> >   pesto.c     | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   pesto.h     |  18 ++++++-
> >   pif.h       |   4 +-
> >   serialise.c |   4 ++
> >   serialise.h |   1 +
> >   util.h      |   2 -
> >   9 files changed, 201 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 1718ddb..6da76b4 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -223,6 +223,7 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck
> >   	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
> >   
> >   passt.cppcheck: BASE_CPPFLAGS += -UPESTO
> > +passt.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
> >   passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
> >   
> >   passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
> > diff --git a/common.h b/common.h
> > index 2f2e6f1..45f66ea 100644
> > --- a/common.h
> > +++ b/common.h
> > @@ -53,4 +53,6 @@ static inline const char *strerror_(int errnum)
> >   
> >   #define strerror(x) @ "Don't call strerror() directly, use strerror_() instead"
> >   
> > +#define ARRAY_SIZE(a)		((int)(sizeof(a) / sizeof((a)[0])))
> > +
> >   #endif /* _COMMON_H */
> > diff --git a/conf.c b/conf.c
> > index 823e08d..3b2fe42 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1925,6 +1925,43 @@ void conf(struct ctx *c, int argc, char **argv)
> >   
> >   static void conf_accept(struct ctx *c);
> >   
> > +/**
> > + * conf_send_rules() - Send current forwarding rules to config client (pesto)
> > + * @c:		Execution context
> > + * @fd:		Socket to the client
> > + *
> > + * Return: 0 on success, -1 on failure
> > + *
> > + * FIXME: So far only sends pif ids and names
> > + */
> > +static int conf_send_rules(const struct ctx *c, int fd)
> > +{
> > +	unsigned pif;
> > +
> > +	for (pif = 0; pif < PIF_NUM_TYPES; pif++) {
> > +		struct pesto_pif_info info;
> > +		int rc;
> > +
> > +		if (!c->fwd[pif])
> > +			continue;
> > +
> > +		assert(pif != PIF_NONE);
> > +
> > +		rc = snprintf(info.name, sizeof(info.name), "%s", pif_name(pif));
> > +		assert(rc >= 0 && (size_t)rc < sizeof(info.name));
> > +
> > +		if (write_u8(fd, pif) < 0)
> > +			return -1;
> > +		if (write_all_buf(fd, &info, sizeof(info)) < 0)
> > +			return -1;
> > +	}
> > +
> > +	if (write_u8(fd, PIF_NONE) < 0)
> > +		return -1;
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * conf_close() - Close configuration / control socket and clean up
> >    * @c:		Execution context
> > @@ -1968,6 +2005,7 @@ static void conf_accept(struct ctx *c)
> >   	struct pesto_hello hello = {
> >   		.magic = PESTO_SERVER_MAGIC,
> >   		.version = htonl(PESTO_PROTOCOL_VERSION),
> > +		.pif_name_size = htonl(PIF_NAME_SIZE),
> >   	};
> >   	union epoll_ref ref = { .type = EPOLL_TYPE_CONF };
> >   	struct ucred uc = { 0 };
> > @@ -2009,6 +2047,9 @@ retry:
> >   "Warning: Using experimental unsupported configuration protocol");
> >   	}
> >   
> > +	if (conf_send_rules(c, fd) < 0)
> > +		goto fail;
> > +
> >   	return;
> >   
> >   fail:
> > diff --git a/pesto.c b/pesto.c
> > index 762cfe9..77244b3 100644
> > --- a/pesto.c
> > +++ b/pesto.c
> > @@ -60,6 +60,127 @@ static void usage(const char *name, FILE *f, int status)
> >   	exit(status);
> >   }
> >   
> > +/* Maximum number of pifs with rule tables */
> > +#define MAX_PIFS	3
> > +
> > +struct pif_configuration {
> > +	uint8_t pif;
> > +	char name[PIF_NAME_SIZE];
> > +};
> > +
> > +struct configuration {
> > +	uint32_t npifs;
> > +	struct pif_configuration pif[MAX_PIFS];
> > +};
> > +
> > +/**
> > + * pif_conf_by_num() - Find a pif's configuration by pif id
> > + * @conf:	Configuration description
> > + * @pif:	pif id
> > + *
> > + * Return: pointer to the pif_configuration for @pif, or NULL if not found
> > + */
> > +static struct pif_configuration *pif_conf_by_num(struct configuration *conf,
> > +						 uint8_t pif)
> > +{
> > +	unsigned i;
> > +
> > +	for (i = 0; i < conf->npifs; i++) {
> > +		if (conf->pif[i].pif == pif)
> > +			return &conf->pif[i];
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +/**
> > + * pif_conf_by_name() - Find a pif's configuration by name
> > + * @conf:	Configuration description
> > + * @name:	Interface name
> > + *
> > + * Return: pif_configuration for pif named @name, or NULL if not found
> > + */
> > +static struct pif_configuration *pif_conf_by_name(struct configuration *conf,
> > +						  const char *name)
> > +{
> > +	unsigned i;
> > +
> > +	for (i = 0; i < conf->npifs; i++) {
> > +		if (strcmp(conf->pif[i].name, name) == 0)
> > +			return &conf->pif[i];
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +/**
> > + * pesto_read_rules() - Read rulestate from passt/pasta
> > + * @fd:		Control socket
> > + * @conf:	Configuration description to update
> > + */
> > +static bool read_pif_conf(int fd, struct configuration *conf)
> > +{
> > +	struct pif_configuration *pc;
> > +	struct pesto_pif_info info;
> > +	uint8_t pif;
> > +
> > +	if (read_u8(fd, &pif) < 0)
> > +		die("Error reading from control socket");
> > +
> > +	if (pif == PIF_NONE)
> > +		return false;
> > +
> > +	debug("Receiving config for PIF %"PRIu8, pif);
> > +
> > +	if (conf->npifs >= ARRAY_SIZE(conf->pif)) {
> > +		die("passt has more pifs than pesto can manage (max %d)",
> > +		    ARRAY_SIZE(conf->pif));
> > +	}
> > +
> > +	pc = &conf->pif[conf->npifs];
> > +	pc->pif = pif;
> > +
> > +	if (read_all_buf(fd, &info, sizeof(info)) < 0)
> > +		die("Error reading from control socket");
> > +
> > +	if (info.name[sizeof(info.name)-1])
> > +		die("Interface name was not NULL terminated");
> > +	/* Redundant, to make static checkers happy */
> > +	info.name[sizeof(info.name) - 1] = '\0';
> > +
> > +	static_assert(sizeof(info.name) == sizeof(pc->name),
> > +		      "Mismatching pif name lengths");
> > +	memcpy(pc->name, info.name, sizeof(pc->name));
> > +
> > +	debug("PIF %"PRIu8": %s", pc->pif, pc->name);
> > +
> > +	/* O(n^2), but n is bounded by MAX_PIFS */
> > +	if (pif_conf_by_num(conf, pc->pif))
> > +		die("Received duplicate interface identifier");
> > +
> > +	/* O(n^2), but n is bounded by MAX_PIFS */
> > +	if (pif_conf_by_name(conf, pc->name))
> > +		die("Received duplicate interface name");
> > +
> > +	conf->npifs++;
> > +	return true;
> > +}
> > +
> > +/**
> > + * show_conf() - Show current configuration obtained from passt/pasta
> > + * @conf:	Configuration description
> > + */
> > +static void show_conf(const struct configuration *conf)
> > +{
> > +	unsigned i;
> > +
> > +	for (i = 0; i < conf->npifs; i++) {
> > +		const struct pif_configuration *pc = &conf->pif[i];
> > +		printf("  %s\n", pc->name);
> > +		printf("    TBD\n");
> > +	}
> > +}
> > +
> >   /**
> >    * main() - Dynamic reconfiguration client main program
> >    * @argc:	Argument count
> > @@ -80,6 +201,7 @@ int main(int argc, char **argv)
> >   		{ 0 },
> >   	};
> >   	struct sockaddr_un a = { AF_UNIX, "" };
> > +	struct configuration conf = { 0 };
> >   	const char *optstring = "dh";
> >   	struct pesto_hello hello;
> >   	struct sock_fprog prog;
> > @@ -162,6 +284,18 @@ int main(int argc, char **argv)
> >   "Warning: Using experimental protocol version, client and server must match\n");
> >   	}
> >   
> > +	if (ntohl(hello.pif_name_size) != PIF_NAME_SIZE) {
> > +		die("Server has unexpected pif name size (%"
> > +		    PRIu32" not %"PRIu32"\n",  
> 
> trailing '\n'

Fixed in v7.

> > +		    ntohl(hello.pif_name_size), PIF_NAME_SIZE);
> > +	}
> > +
> > +	while (read_pif_conf(s, &conf))
> > +		;
> > +
> > +	printf("passt/pasta configuration (%s)\n", a.sun_path);
> > +	show_conf(&conf);
> > +
> >   	if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0)
> >   		die_perror("Error shutting down control socket");
> >   
> > diff --git a/pesto.h b/pesto.h
> > index 92d4df3..1879759 100644
> > --- a/pesto.h
> > +++ b/pesto.h
> > @@ -17,18 +17,32 @@
> >   /* Version 0 is reserved for unreleased / unsupported experimental versions */
> >   #define PESTO_PROTOCOL_VERSION	0
> >   
> > +/* Maxmimum size of a pif name, including \0 */  
> 
> Typo copied from pif.h, should be "Maximum"

Fixed in v7.

> > +#define	PIF_NAME_SIZE	(128)
> > +#define PIF_NONE	0
> > +
> >   /**
> >    * struct pesto_hello - Server introduction message
> > - * @magic:	PESTO_SERVER_MAGIC
> > - * @version:	Version number
> > + * @magic:		PESTO_SERVER_MAGIC
> > + * @version:		Version number
> > + * @pif_name_size:	Server's value for PIF_NAME_SIZE
> >    */
> >   struct pesto_hello {
> >   	char magic[8];
> >   	uint32_t version;
> > +	uint32_t pif_name_size;
> >   } __attribute__ ((__packed__));
> >   
> >   static_assert(sizeof(PESTO_SERVER_MAGIC)
> >   	      == sizeof(((struct pesto_hello *)0)->magic),
> >   	      "PESTO_SERVER_MAGIC has wrong size");
> >   
> > +/**
> > + * struct pesto_pif_info - Message with basic metadata about a pif
> > + * @name:	Name (\0 terminated)
> > + */
> > +struct pesto_pif_info {
> > +	char name[PIF_NAME_SIZE];
> > +} __attribute__ ((__packed__));
> > +
> >   #endif /* PESTO_H */
> > diff --git a/pif.h b/pif.h
> > index 90dd3a3..d770860 100644
> > --- a/pif.h
> > +++ b/pif.h
> > @@ -11,6 +11,7 @@
> >   
> >   #include <netinet/in.h>
> >   
> > +#include "pesto.h"
> >   #include "epoll_type.h"
> >   
> >   union inany_addr;
> > @@ -24,7 +25,7 @@ union sockaddr_inany;
> >    */
> >   enum pif_type {
> >   	/* Invalid or not present pif */
> > -	PIF_NONE = 0,
> > +	PIF_NONE_ = PIF_NONE,
> >   	/* Host socket interface */
> >   	PIF_HOST,
> >   	/* Qemu socket or namespace tuntap interface */
> > @@ -36,7 +37,6 @@ enum pif_type {
> >   };
> >   
> >   /* Maxmimum size of a pif name, including \0 */  
> 
> Perhaps you could remove the above comment too?

Fixed in v7.

> > -#define	PIF_NAME_SIZE	(128)
> >   extern const char pif_type_str[][PIF_NAME_SIZE];

-- 
Stefano