From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=KfHK12bS; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id D1F885A0265 for ; Wed, 25 Mar 2026 05:44:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1774413865; bh=NYyn5Yos64wN9SJf/okp5WhqUrDZjJGtOKeFGPdxOog=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=KfHK12bSJ+26T0ktmXzgyfEUIJpfFLHXE7PDzAcfRwyA2+gxibPD+O6jExP2C2v4f d4CQb+prG+GjUdWbHdmRV4OTyahNN2WgphUQctULCHndfQS7rCjhax35gB3k9bAm1S mq4T1ZLmM2JYfNtWXJMcbfM0l91vkuEecaaQMm5qcgZ6e6kQoVAa/2wMKz/eQuaV/V ospHyfzYiY+jF3lO3ciBFJZG876KFJE4njVV+WYzpFfjJ/yrBAeTsNDZeFeNw6UENQ jJmuwntmYpSjkoUZDRqH+fLOpna1p/pz1osYCWZEbBKiKq+WS/naVtRK6pVLRYj/gJ cyXxjfjkAX3UA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4fgZ7Y2R5Bz4wHj; Wed, 25 Mar 2026 15:44:25 +1100 (AEDT) Date: Wed, 25 Mar 2026 15:27:02 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 14/25] pesto: Add command line option parsing and debug messages Message-ID: References: <20260323073732.3158468-1-david@gibson.dropbear.id.au> <20260323073732.3158468-15-david@gibson.dropbear.id.au> <20260325015506.201d9ca0@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="+Wk3tZI8L/gW+Stz" Content-Disposition: inline In-Reply-To: <20260325015506.201d9ca0@elisabeth> Message-ID-Hash: QLKR4B3QSVB6VHYBE6QOHPFDWKORNND7 X-Message-ID-Hash: QLKR4B3QSVB6VHYBE6QOHPFDWKORNND7 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --+Wk3tZI8L/gW+Stz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 25, 2026 at 01:55:07AM +0100, Stefano Brivio wrote: > On Mon, 23 Mar 2026 18:37:21 +1100 > David Gibson wrote: >=20 > > Add basic command line option parsing using getopt_long() to pesto. > > Implement --help, --version, --quiet and --verbose options, along with a >=20 > Two questions: >=20 > - do we really need --quiet? >=20 > - 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? Fair point. I've reworked it that way. >=20 > > debug() macro to print debugging messages when given the verbose option. > >=20 > > Signed-off-by: David Gibson > > --- > > common.h | 8 ++++++ > > conf.c | 45 +++++++++++++++++---------------- > > passt.h | 12 ++++----- > > pesto.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++------ > > util.h | 8 ------ > > 5 files changed, 108 insertions(+), 42 deletions(-) > >=20 > > 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 > > =20 > > +#define VERSION_BLOB \ > > + VERSION "\n" \ > > + "Copyright Red Hat\n" \ > > + "GNU General Public License, version 2 or later\n" \ > > + " \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__) > > =20 > > 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; > > =20 > > + if (c->fd_control_listen >=3D 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); > > } > > =20 > > - c->fd_conf =3D -1; > > - if (*c->conf_path) { > > - c->fd_conf_listen =3D sock_unix(c->conf_path); > > - if (c->fd_conf_listen < 0) { > > + c->fd_control =3D -1; > > + if (*c->control_path) { >=20 > For later: maybe I guess we should squash the renaming of this file > descriptor with the earlier patch adding it. Oops, that was my intention, but clearly I updated the wrong patch. > > + c->fd_control_listen =3D 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 =3D -1; > > + c->fd_control_listen =3D -1; > > } > > } > > =20 > > @@ -1485,13 +1488,13 @@ static void conf_sock_listen(const struct ctx *= c) > > { > > union epoll_ref ref =3D { .type =3D EPOLL_TYPE_CONF_LISTEN }; > > =20 > > - if (c->fd_conf_listen < 0) > > + if (c->fd_control_listen < 0) > > return; > > =20 > > - if (listen(c->fd_conf_listen, 0)) > > + if (listen(c->fd_control_listen, 0)) > > die_perror("Couldn't listen on configuration socket"); > > =20 > > - ref.fd =3D c->fd_conf_listen; > > + ref.fd =3D 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 =3D -1; > > break; > > case 'c': > > - ret =3D snprintf(c->conf_path, sizeof(c->conf_path), "%s", > > - optarg); > > + ret =3D snprintf(c->control_path, sizeof(c->control_path), > > + "%s", optarg); > > if (ret <=3D 0 || ret >=3D (int)sizeof(c->sock_path)) > > die("Invalid configuration path: %s", optarg); > > - c->fd_conf_listen =3D c->fd_conf =3D -1; > > + c->fd_control_listen =3D c->fd_control =3D -1; > > break; > > case 'F': > > errno =3D 0; > > @@ -2294,10 +2297,10 @@ void conf(struct ctx *c, int argc, char **argv) > > FWD_SCAN); > > } > > =20 > > + conf_sock_listen(c); > > + > > if (!c->quiet) > > conf_print(c); > > - > > - conf_sock_listen(c); > > } > > =20 > > /** > > @@ -2321,7 +2324,7 @@ void conf_listen_handler(struct ctx *c, uint32_t = events) > > return; > > } > > =20 > > - fd =3D accept4(c->fd_conf_listen, NULL, NULL, SOCK_NONBLOCK); > > + fd =3D 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"); > > =20 > > /* Another client is already connected: accept and close right away. = */ > > - if (c->fd_conf !=3D -1) { > > + if (c->fd_control !=3D -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; > > } > > =20 > > - c->fd_conf =3D fd; > > + c->fd_control =3D 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; > > =20 > > do { > > - n =3D read(c->fd_conf, discard, sizeof(discard)); > > + n =3D 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) > > =20 > > close: > > debug("Closing configuration socket"); > > - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_conf, NULL); > > - close(c->fd_conf); > > - c->fd_conf =3D -1; > > + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_control, NULL); > > + close(c->fd_control); > > + c->fd_control =3D -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 defa= ult > > * @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]; > > =20 > > @@ -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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -42,6 +43,32 @@ > > exit(EXIT_FAILURE); \ > > } while (0) > > =20 > > +#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[] =3D { > > + {"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 =3D { AF_UNIX, "" }; > > + const char *optstring =3D "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); >=20 > 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. So, I made that change as a temporary debugging hack which I forgot to revert. That said, I don't really see the case for disabling dump in pesto - if you can trace pesto you could just connect directly to the control socket. The possibility of dropping to nobody is the closest to a case I've seen... but I don't really see a reason we'd want to do that. It would be quite awkward for one thing, since nobody probably won't have permission to the control socket. Running this as root wouldn't be the wisest choice, but unlike for passt/pasta I don't think it's scary enough that we should actively try to stop the user from shooting themselves in the foot. That said, it obviously shouldn't be removed in this patch, having just been added in the previous one. I've left it in for now. > > + int verbosity =3D 1; >=20 > If we skip --quiet, this can be simplified. Done. > > =20 > > prog.len =3D (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"); > > =20 > > - if (argc < 2) > > - die("Usage: %s CONTROLPATH", argv[0]); >=20 > 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. Ah, yes, another update in the wrong patch. I've fixed it in the earlier patch anyway, to save you the bother of remembering it goes away when you next look. > > + do { > > + optname =3D 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 !=3D -1); > > + > > + if (argc - optind !=3D 1) > > + usage(argv[0], stderr, EXIT_FAILURE); > > =20 > > if ((s =3D socket(AF_UNIX, SOCK_STREAM, 0)) < 0) > > die("Failed to create AF_UNIX socket: %s", strerror(errno)); > > =20 > > - ret =3D snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[1]); > > + ret =3D snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[optind]); > > if (ret <=3D 0 || ret >=3D (int)sizeof(a.sun_path)) > > - die("Invalid socket path \"%s\"", argv[1]); > > + die("Invalid socket path \"%s\"", argv[optind]); > > =20 > > ret =3D 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)); > > } > > =20 > > + debug("Connected to passt/pasta control socket"); > > + > > ret =3D 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) > > =20 > > s_version =3D ntohl(hello.version); > > =20 > > + 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 @@ > > =20 > > #include "log.h" > > =20 > > -#define VERSION_BLOB \ > > - VERSION "\n" \ > > - "Copyright Red Hat\n" \ > > - "GNU General Public License, version 2 or later\n" \ > > - " \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 >=20 > --=20 > Stefano >=20 --=20 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 --+Wk3tZI8L/gW+Stz Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmnDZBUACgkQzQJF27ox 2GeslA/+L9KOHACzeAsHjEkt7Gxv2kr0rPZQWFDV74UWF1/0EaHIYieGLPyNWxY6 p6JT2VMHwbPjNCLKtDLoL0CXjuWvYvRxYjUZpuu27N0I/37vQtCh8yjwuFlcSheO 2SxCpVrfpTzOb+InuSb4G3q4MVZJMkihkdUu3pCgTQB8thdtKCig6z9uPcXf8DmF 6mL3qCvlyiS/oBbSVsrNAhg6Y2pBdPJFtijjKK2N/kELg74er/W4nJqVlLv5Jsn4 VMK0r8XAGoecQ4ThPnYudfAeaNF4xJm484kgS6oj7QHGYrT/lgnYhMCHGTaTG4JE aXePfRxiXOZ30g4gPnxv291wK5nc/NEWzOVBaRhA1k4CrOiFNWQLEBeHNDLA8oeR gHsu+ZuCLuBqs2+jQ7FKDZQLC4PjDIAgn8ahZywnSA3HZYlWEJS/UnrNptkyeEe6 6ES5WriwjBeMJd4Tq+6AuBQL1zjqB4YGb33qwREnST+3KJngnV8oWgqsBAaqjCGH GqTf+TMXgfGMmh9v4PThEx/16RcrdX8/JiSfPuNM6iZYf9RIvYIQw/xCCuPzIz1V HruYyQpYzzZxJ+53Pbw2fB4N6PIkz9YdoKlmCuJC7oGWKtfZMwp19AFuBKO1vqE2 1S/DakXHcmn9UDJI06l2eaTkdPUQaJhqjm1h9C1/cvrmA10bKIU= =eZQm -----END PGP SIGNATURE----- --+Wk3tZI8L/gW+Stz--