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

[Laurent Vivier <lvivier@redhat.com>]

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

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

> +		    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"

> +#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?

> -#define	PIF_NAME_SIZE	(128)
>   extern const char pif_type_str[][PIF_NAME_SIZE];
>   
>   static inline const char *pif_type(enum pif_type pt)
> diff --git a/serialise.c b/serialise.c
> index 346df99..e083112 100644
> --- a/serialise.c
> +++ b/serialise.c
> @@ -121,6 +121,10 @@ int write_all_buf(int fd, const void *buf, size_t len)
>   		return write_all_buf(fd, &beval, sizeof(beval));	\
>   	}
>   
> +#define	be8toh(x)	(x)
> +#define	htobe8(x)	(x)
> +
> +SERIALISE_UINT(8)
>   SERIALISE_UINT(32)
>   
>   #undef SERIALISE_UINT
> diff --git a/serialise.h b/serialise.h
> index a88f3de..4714f4c 100644
> --- a/serialise.h
> +++ b/serialise.h
> @@ -16,6 +16,7 @@ int write_all_buf(int fd, const void *buf, size_t len);
>   	int read_u##bits(int fd, uint##bits##_t *val);			\
>   	int write_u##bits(int fd, uint##bits##_t val);
>   
> +SERIALISE_UINT_DECL(8)
>   SERIALISE_UINT_DECL(32)
>   
>   #endif /* SERIALISE_H */
> diff --git a/util.h b/util.h
> index e90be47..c788382 100644
> --- a/util.h
> +++ b/util.h
> @@ -87,8 +87,6 @@ void abort_with_msg(const char *fmt, ...)
>   #define V6		1
>   #define IP_VERSIONS	2
>   
> -#define ARRAY_SIZE(a)		((int)(sizeof(a) / sizeof((a)[0])))
> -
>   #define foreach(item, array)						\
>   	for ((item) = (array); (item) - (array) < ARRAY_SIZE(array); (item)++)
>