Re: [PATCH v3 14/25] pesto: Add command line option parsing and debug messages

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 23 Mar 2026 18:37:21 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Add basic command line option parsing using getopt_long() to pesto.
> Implement --help, --version, --quiet and --verbose options, along with a

Two questions:

- do we really need --quiet?

- shouldn't we stick to the passt(1) convention where we avoided -v
  altogether to avoid confusion, and we just have --version and -d /
  --debug?

> debug() macro to print debugging messages when given the verbose option.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  common.h |  8 ++++++
>  conf.c   | 45 +++++++++++++++++----------------
>  passt.h  | 12 ++++-----
>  pesto.c  | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  util.h   |  8 ------
>  5 files changed, 108 insertions(+), 42 deletions(-)
> 
> diff --git a/common.h b/common.h
> index 76a95609..927d20a4 100644
> --- a/common.h
> +++ b/common.h
> @@ -8,6 +8,14 @@
>  #ifndef COMMON_H
>  #define COMMON_H
>  
> +#define VERSION_BLOB							       \
> +	VERSION "\n"							       \
> +	"Copyright Red Hat\n"						       \
> +	"GNU General Public License, version 2 or later\n"		       \
> +	"  <https://www.gnu.org/licenses/old-licenses/gpl-2.0.html>\n"	       \
> +	"This is free software: you are free to change and redistribute it.\n" \
> +	"There is NO WARRANTY, to the extent permitted by law.\n\n"
> +
>  /* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */
>  #define FPRINTF(f, ...)	(void)fprintf(f, __VA_ARGS__)
>  
> diff --git a/conf.c b/conf.c
> index 5e8bc665..7b960fe9 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1140,6 +1140,9 @@ static void conf_print(const struct ctx *c)
>  	char bufmac[ETH_ADDRSTRLEN], ifn[IFNAMSIZ];
>  	int i;
>  
> +	if (c->fd_control_listen >= 0)
> +		info("Configuration socket: %s", c->control_path);
> +
>  	if (c->ifi4 > 0 || c->ifi6 > 0) {
>  		info("Template interface: %s%s%s%s%s",
>  		     c->ifi4 > 0 ? if_indextoname(c->ifi4, ifn) : "",
> @@ -1432,15 +1435,15 @@ static void conf_open_files(struct ctx *c)
>  			die_perror("Couldn't open PID file %s", c->pidfile);
>  	}
>  
> -	c->fd_conf = -1;
> -	if (*c->conf_path) {
> -		c->fd_conf_listen = sock_unix(c->conf_path);
> -		if (c->fd_conf_listen < 0) {
> +	c->fd_control = -1;
> +	if (*c->control_path) {

For later: maybe I guess we should squash the renaming of this file
descriptor with the earlier patch adding it.

> +		c->fd_control_listen = sock_unix(c->control_path);
> +		if (c->fd_control_listen < 0) {
>  			die_perror("Couldn't open control socket %s",
> -				   c->conf_path);
> +				   c->control_path);
>  		}
>  	} else {
> -		c->fd_conf_listen = -1;
> +		c->fd_control_listen = -1;
>  	}
>  }
>  
> @@ -1485,13 +1488,13 @@ static void conf_sock_listen(const struct ctx *c)
>  {
>  	union epoll_ref ref = { .type = EPOLL_TYPE_CONF_LISTEN };
>  
> -	if (c->fd_conf_listen < 0)
> +	if (c->fd_control_listen < 0)
>  		return;
>  
> -	if (listen(c->fd_conf_listen, 0))
> +	if (listen(c->fd_control_listen, 0))
>  		die_perror("Couldn't listen on configuration socket");
>  
> -	ref.fd = c->fd_conf_listen;
> +	ref.fd = c->fd_control_listen;
>  	if (epoll_add(c->epollfd, EPOLLIN | EPOLLET, ref))
>  		die_perror("Couldn't add configuration socket to epoll");
>  }
> @@ -1846,11 +1849,11 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->fd_tap = -1;
>  			break;
>  		case 'c':
> -			ret = snprintf(c->conf_path, sizeof(c->conf_path), "%s",
> -				       optarg);
> +			ret = snprintf(c->control_path, sizeof(c->control_path),
> +				       "%s", optarg);
>  			if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
>  				die("Invalid configuration path: %s", optarg);
> -			c->fd_conf_listen = c->fd_conf = -1;
> +			c->fd_control_listen = c->fd_control = -1;
>  			break;
>  		case 'F':
>  			errno = 0;
> @@ -2294,10 +2297,10 @@ void conf(struct ctx *c, int argc, char **argv)
>  					FWD_SCAN);
>  	}
>  
> +	conf_sock_listen(c);
> +
>  	if (!c->quiet)
>  		conf_print(c);
> -
> -	conf_sock_listen(c);
>  }
>  
>  /**
> @@ -2321,7 +2324,7 @@ void conf_listen_handler(struct ctx *c, uint32_t events)
>  		return;
>  	}
>  
> -	fd = accept4(c->fd_conf_listen, NULL, NULL, SOCK_NONBLOCK);
> +	fd = accept4(c->fd_control_listen, NULL, NULL, SOCK_NONBLOCK);
>  	if (fd < 0) {
>  		warn_perror("accept4() on configuration listening socket");
>  		return;
> @@ -2331,7 +2334,7 @@ void conf_listen_handler(struct ctx *c, uint32_t events)
>  		warn_perror("Can't get configuration client credentials");
>  
>  	/* Another client is already connected: accept and close right away. */
> -	if (c->fd_conf != -1) {
> +	if (c->fd_control != -1) {
>  		info("Discarding configuration client, PID %i", uc.pid);
>  		goto fail;
>  	}
> @@ -2349,7 +2352,7 @@ void conf_listen_handler(struct ctx *c, uint32_t events)
>  		goto fail;
>  	}
>  
> -	c->fd_conf = fd;
> +	c->fd_control = fd;
>  	info("Accepted configuration client, PID %i", uc.pid);
>  	if (!PESTO_PROTOCOL_VERSION) {
>  		warn(
> @@ -2374,7 +2377,7 @@ void conf_handler(struct ctx *c, uint32_t events)
>  		ssize_t n;
>  
>  		do {
> -			n = read(c->fd_conf, discard, sizeof(discard));
> +			n = read(c->fd_control, discard, sizeof(discard));
>  			if (n > 0)
>  				debug("Discarded %zd bytes of config data", n);
>  		} while (n > 0);
> @@ -2397,7 +2400,7 @@ void conf_handler(struct ctx *c, uint32_t events)
>  
>  close:
>  	debug("Closing configuration socket");
> -	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_conf, NULL);
> -	close(c->fd_conf);
> -	c->fd_conf = -1;
> +	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_control, NULL);
> +	close(c->fd_control);
> +	c->fd_control = -1;
>  }
> diff --git a/passt.h b/passt.h
> index c38bb5ae..b3f049de 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -158,7 +158,7 @@ struct ip6_ctx {
>   * @foreground:		Run in foreground, don't log to stderr by default
>   * @nofile:		Maximum number of open files (ulimit -n)
>   * @sock_path:		Path for UNIX domain socket
> - * @conf_path:		Path for configuration UNIX domain socket
> + * @control_path:	Path for control/configuration UNIX domain socket
>   * @repair_path:	TCP_REPAIR helper path, can be "none", empty for default
>   * @pcap:		Path for packet capture file
>   * @pidfile:		Path to PID file, empty string if not configured
> @@ -170,8 +170,8 @@ struct ip6_ctx {
>   * @epollfd:		File descriptor for epoll instance
>   * @fd_tap_listen:	File descriptor for listening AF_UNIX socket, if any
>   * @fd_tap:		AF_UNIX socket, tuntap device, or pre-opened socket
> - * @fd_conf_listen:	Listening configuration socket, if any
> - * @fd_conf:		Configuration socket, if any
> + * @fd_control_listen:	Listening control/configuration socket, if any
> + * @fd_control:		Control/configuration socket, if any
>   * @fd_repair_listen:	File descriptor for listening TCP_REPAIR socket, if any
>   * @fd_repair:		Connected AF_UNIX socket for TCP_REPAIR helper
>   * @our_tap_mac:	Pasta/passt's MAC on the tap link
> @@ -226,7 +226,7 @@ struct ctx {
>  	int foreground;
>  	int nofile;
>  	char sock_path[UNIX_PATH_MAX];
> -	char conf_path[UNIX_PATH_MAX];
> +	char control_path[UNIX_PATH_MAX];
>  	char repair_path[UNIX_PATH_MAX];
>  	char pcap[PATH_MAX];
>  
> @@ -244,8 +244,8 @@ struct ctx {
>  	int epollfd;
>  	int fd_tap_listen;
>  	int fd_tap;
> -	int fd_conf_listen;
> -	int fd_conf;
> +	int fd_control_listen;
> +	int fd_control;
>  	int fd_repair_listen;
>  	int fd_repair;
>  	unsigned char our_tap_mac[ETH_ALEN];
> diff --git a/pesto.c b/pesto.c
> index 6d066084..f8c9e01d 100644
> --- a/pesto.c
> +++ b/pesto.c
> @@ -15,6 +15,7 @@
>  #include <sys/socket.h>
>  #include <sys/un.h>
>  #include <errno.h>
> +#include <getopt.h>
>  #include <inttypes.h>
>  #include <stdbool.h>
>  #include <stddef.h>
> @@ -42,6 +43,32 @@
>  		exit(EXIT_FAILURE);					\
>  	} while (0)
>  
> +#define debug(...)							\
> +	do {								\
> +		if (verbosity > 1) {					\
> +			FPRINTF(stderr, __VA_ARGS__);			\
> +			FPRINTF(stderr, "\n");				\
> +		}							\
> +	} while (0)
> +
> +/**
> + * usage() - Print usage, exit with given status code
> + * @name:	Executable name
> + * @f:		Stream to print usage info to
> + * @status:	Status code for exit(2)
> + */
> +static void usage(const char *name, FILE *f, int status)
> +{
> +	FPRINTF(f, "Usage: %s [OPTION]... PATH\n", name);
> +	FPRINTF(f,
> +		"\n"
> +		"  -v, --verbose		Be more verbose\n"
> +		"  -q, --quiet		Be less verbose\n"
> +		"  -h, --help		Display this help message and exit\n"
> +		"  --version		Show version and exit\n");
> +	exit(status);
> +}
> +
>  /**
>   * main() - Entry point and whole program with loop
>   * @argc:	Argument count
> @@ -56,13 +83,20 @@
>   */
>  int main(int argc, char **argv)
>  {
> +	const struct option options[] = {
> +		{"quiet",	no_argument,		NULL,		'q' },
> +		{"verbose",	no_argument,		NULL,		'v' },
> +		{"help",	no_argument,		NULL,		'h' },
> +		{"version",	no_argument,		NULL,		1 },
> +		{ 0 },
> +	};
>  	struct sockaddr_un a = { AF_UNIX, "" };
> +	const char *optstring = "vh";
>  	struct pesto_hello hello;
>  	struct sock_fprog prog;
> +	int optname, ret, s;
>  	uint32_t s_version;
> -	int ret, s;
> -
> -	prctl(PR_SET_DUMPABLE, 0);

I would, at least by default, keep this exactly in line with the
paranoid stuff I added to passt, for consistency and ease of auditing:
what will happen if we run *this* tool as root? If we choose to drop to
nobody in that case, removing this will be a problem. And we'll
probably not remember to add it back.

> +	int verbosity = 1;

If we skip --quiet, this can be simplified.

>  
>  	prog.len = (unsigned short)sizeof(filter_pesto) /
>  				   sizeof(filter_pesto[0]);
> @@ -71,15 +105,40 @@ int main(int argc, char **argv)
>  	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog))
>  		die("Failed to apply seccomp filter");
>  
> -	if (argc < 2)
> -		die("Usage: %s CONTROLPATH", argv[0]);

I was suggesting to use PATH instead of CONTROLPATH in the earlier
patch, but this goes away here and we'll probably want to squash this
anyway so it shouldn't matter.

> +	do {
> +		optname = getopt_long(argc, argv, optstring, options, NULL);
> +
> +		switch (optname) {
> +		case -1:
> +		case 0:
> +			break;
> +		case 'h':
> +			usage(argv[0], stdout, EXIT_SUCCESS);
> +			break;
> +		case 'q':
> +			verbosity--;
> +			break;
> +		case 'v':
> +			verbosity++;
> +			break;
> +		case 1:
> +			FPRINTF(stdout, "pesto ");
> +			FPRINTF(stdout, VERSION_BLOB);
> +			exit(EXIT_SUCCESS);
> +		default:
> +			usage(argv[0], stderr, EXIT_FAILURE);
> +		}
> +	} while (optname != -1);
> +
> +	if (argc - optind != 1)
> +		usage(argv[0], stderr, EXIT_FAILURE);
>  
>  	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
>  		die("Failed to create AF_UNIX socket: %s", strerror(errno));
>  
> -	ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[1]);
> +	ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[optind]);
>  	if (ret <= 0 || ret >= (int)sizeof(a.sun_path))
> -		die("Invalid socket path \"%s\"", argv[1]);
> +		die("Invalid socket path \"%s\"", argv[optind]);
>  
>  	ret = connect(s, (struct sockaddr *)&a, sizeof(a));
>  	if (ret < 0) {
> @@ -87,6 +146,8 @@ int main(int argc, char **argv)
>  		    a.sun_path, strerror(errno));
>  	}
>  
> +	debug("Connected to passt/pasta control socket");
> +
>  	ret = read_all_buf(s, &hello, sizeof(hello));
>  	if (ret < 0)
>  		die("Couldn't read server greeting: %s", strerror(errno));
> @@ -96,6 +157,8 @@ int main(int argc, char **argv)
>  
>  	s_version = ntohl(hello.version);
>  
> +	debug("Server protocol version: %"PRIu32, s_version);
> +
>  	if (s_version > PESTO_PROTOCOL_VERSION) {
>  		die("Unknown server protocol version %"PRIu32" > %"PRIu32"\n",
>  		    s_version, PESTO_PROTOCOL_VERSION);
> diff --git a/util.h b/util.h
> index e7993f4d..d7c397f6 100644
> --- a/util.h
> +++ b/util.h
> @@ -21,14 +21,6 @@
>  
>  #include "log.h"
>  
> -#define VERSION_BLOB							       \
> -	VERSION "\n"							       \
> -	"Copyright Red Hat\n"						       \
> -	"GNU General Public License, version 2 or later\n"		       \
> -	"  <https://www.gnu.org/licenses/old-licenses/gpl-2.0.html>\n"	       \
> -	"This is free software: you are free to change and redistribute it.\n" \
> -	"There is NO WARRANTY, to the extent permitted by law.\n\n"
> -
>  #ifndef SECCOMP_RET_KILL_PROCESS
>  #define SECCOMP_RET_KILL_PROCESS	SECCOMP_RET_KILL
>  #endif

-- 
Stefano