Re: [PATCH v6 15/18] pesto: Parse and add new rules from command line

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 4 May 2026 18:44:29 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 5/3/26 23:55, Stefano Brivio wrote:
> > From: David Gibson <david@gibson.dropbear.id.au>
> > 
> > This adds parsing of options using fwd_rule_parse(), validates them and
> > adds them to the existing rules. It doesn't yet send those rules back to
> > passt or pasta.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > Message-ID: <20260322141843.4095972-3-sbrivio@redhat.com>
> > [dwg: Based on an early draft by Stefano]
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >   Makefile   |   1 +
> >   fwd_rule.c |   2 +-
> >   fwd_rule.h |   1 +
> >   pesto.c    | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   4 files changed, 111 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 057e4eb..125ec01 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -227,6 +227,7 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck
> >   passt.cppcheck: BASE_CPPFLAGS += -UPESTO
> >   passt.cppcheck: CPPCHECK_FLAGS += \
> >   	--suppress=unusedFunction:fwd_rule.c \
> > +	--suppress=staticFunction:fwd_rule.c \
> >   	--suppress=unusedFunction:serialise.c
> >   passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
> >   
> > diff --git a/fwd_rule.c b/fwd_rule.c
> > index da9d893..3c1eaa4 100644
> > --- a/fwd_rule.c
> > +++ b/fwd_rule.c
> > @@ -187,7 +187,7 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *
> >    *
> >    * Return: 0 on success, negative error code on failure
> >    */
> > -static int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
> > +int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
> >   {
> >   	/* Flags which can be set from the caller */
> >   	const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY;
> > diff --git a/fwd_rule.h b/fwd_rule.h
> > index 330d49e..f43b37d 100644
> > --- a/fwd_rule.h
> > +++ b/fwd_rule.h
> > @@ -103,6 +103,7 @@ const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size);
> >   void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd);
> >   int fwd_rule_read(int fd, struct fwd_rule *rule);
> >   int fwd_rule_write(int fd, const struct fwd_rule *rule);
> > +int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new);
> >   
> >   /**
> >    * fwd_rules_dump() - Dump forwarding rules
> > diff --git a/pesto.c b/pesto.c
> > index 4bf9bd8..95aecad 100644
> > --- a/pesto.c
> > +++ b/pesto.c
> > @@ -55,6 +55,43 @@ static void usage(const char *name, FILE *f, int status)
> >   	FPRINTF(f, "Usage: %s [OPTION]... PATH\n", name);
> >   	FPRINTF(f,
> >   		"\n"
> > +		"  -t, --tcp-ports SPEC	TCP inbound port forwarding\n"
> > +		"    can be specified multiple times\n"
> > +		"    SPEC can be:\n"
> > +		"      'none': don't forward any ports\n"
> > +		"      [ADDR[%%IFACE]/]PORTS: forward specific ports\n"
> > +		"        PORTS is either 'all' (forward all unbound, non-ephemeral\n"
> > +		"        ports), or a comma-separated list of ports, optionally\n"
> > +		"        ranged with '-' and optional target ports after ':'.\n"
> > +		"        Ranges can be reduced by excluding ports or ranges\n"
> > +		"        prefixed by '~'.\n"
> > +		"        The 'auto' keyword may be given to only forward\n"
> > +		"        ports which are bound in the target namespace\n"
> > +		"        Examples:\n"
> > +		"        -t all         Forward all ports\n"
> > +		"        -t 127.0.0.1/all Forward all ports from local address\n"
> > +		"                         127.0.0.1\n"
> > +		"        -t 22		Forward local port 22 to 22\n"
> > +		"        -t 22:23	Forward local port 22 to 23\n"
> > +		"        -t 22,25	Forward ports 22, 25 to ports 22, 25\n"
> > +		"        -t 22-80  	Forward ports 22 to 80\n"
> > +		"        -t 22-80:32-90	Forward ports 22 to 80 to\n"
> > +		"			corresponding port numbers plus 10\n"
> > +		"        -t 192.0.2.1/5	Bind port 5 of 192.0.2.1\n"
> > +		"        -t 5-25,~10-20	Forward ports 5 to 9, and 21 to 25\n"
> > +		"        -t ~25		Forward all ports except for 25\n"
> > +		"        -t auto	Forward all ports bound in namespace\n"
> > +		"        -t 192.0.2.2/auto Forward ports from 192.0.2.2 if\n"
> > +		"                          they are bound in the namespace\n"
> > +		"        -t 8000-8010,auto Forward ports 8000-8010 if they\n"
> > +		"                          are bound in the namespace\n"
> > +		"  -u, --udp-ports SPEC	UDP inbound port forwarding\n"
> > +		"    SPEC is as described for TCP above\n"
> > +		"  -T, --tcp-ns SPEC	TCP outbound port forwarding\n"
> > +		"    SPEC is as described above\n"
> > +		"  -U, --udp-ns SPEC	UDP outbound port forwarding\n"
> > +		"    SPEC is as described above\n"  
> 
> I think description from conf.c is clearer:
> 
>                  "  -T, --tcp-ns SPEC    TCP port forwarding to init namespace\n"
>                  "  -U, --udp-ns SPEC    UDP port forwarding to init namespace\n"

Changed in v7.

> Is it possible to define a common usage description between passt/pasta/pesto?
> A "#define COMMON_OPTS" ?

I gave it a quick try, but note that there are options that are shared
between passt and pesto, as well as between pasta and pesto, but not
between passt and pasta, because the "namespace" options don't make
sense for pasta.

Due to that, a COMMON_OPTS macro (or several of them) makes things
pretty hard to follow because it makes it even harder to spot which
parts are for which tool.

> > +		"  -s, --show		Show configuration before and after\n"  
> 
> Update pesto.1

Done in v7, and I updated it throughout the whole series (other options
were already added before this point but not documented).

> >   		"  -d, --debug		Print debugging messages\n"
> >   		"  -h, --help		Display this help message and exit\n"
> >   		"  --version		Show version and exit\n");
> > @@ -204,6 +241,8 @@ static void show_conf(const struct configuration *conf)
> >   		fwd_rules_dump(printf, pc->fwd.rules, pc->fwd.count,
> >   			       "    ", "\n");
> >   	}
> > +	/* Flush stdout, so this doesn't get misordered with later debug()s */
> > +	(void)fflush(stdout);
> >   }
> >   
> >   /**
> > @@ -215,7 +254,7 @@ static void show_conf(const struct configuration *conf)
> >    *
> >    * #syscalls:pesto socket s390x:socketcall i686:socketcall
> >    * #syscalls:pesto connect shutdown close
> > - * #syscalls:pesto exit_group fstat read write
> > + * #syscalls:pesto exit_group fstat read write openat
> >    */
> >   int main(int argc, char **argv)
> >   {
> > @@ -223,11 +262,18 @@ int main(int argc, char **argv)
> >   		{"debug",	no_argument,		NULL,		'd' },
> >   		{"help",	no_argument,		NULL,		'h' },
> >   		{"version",	no_argument,		NULL,		1 },
> > +		{"tcp-ports",	required_argument,	NULL,		't' },
> > +		{"udp-ports",	required_argument,	NULL,		'u' },
> > +		{"tcp-ns",	required_argument,	NULL,		'T' },
> > +		{"udp-ns",	required_argument,	NULL,		'U' },
> > +		{"show",	no_argument,		NULL,		's' },
> >   		{ 0 },
> >   	};
> > +	struct pif_configuration *inbound, *outbound;
> >   	struct sockaddr_un a = { AF_UNIX, "" };
> > +	const char *optstring = "dht:u:T:U:s";
> >   	struct configuration conf = { 0 };
> > -	const char *optstring = "dh";
> > +	bool update = false, show = false;
> >   	struct pesto_hello hello;
> >   	struct sock_fprog prog;
> >   	int optname, ret, s;
> > @@ -248,6 +294,8 @@ int main(int argc, char **argv)
> >   	if (setvbuf(stdout, stdout_buf, _IOFBF, sizeof(stdout_buf)))
> >   		die_perror("Failed to set stdout buffer");
> >   
> > +	fwd_probe_ephemeral();
> > +
> >   	do {
> >   		optname = getopt_long(argc, argv, optstring, options, NULL);
> >   
> > @@ -255,6 +303,16 @@ int main(int argc, char **argv)
> >   		case -1:
> >   		case 0:
> >   			break;
> > +		case 't':
> > +		case 'u':
> > +		case 'T':
> > +		case 'U':
> > +			/* Parse these options after we've read state from passt/pasta */
> > +			update = true;
> > +			break;
> > +		case 's':
> > +			show = true;
> > +			break;
> >   		case 'h':
> >   			usage(argv[0], stdout, EXIT_SUCCESS);
> >   			break;
> > @@ -287,6 +345,8 @@ int main(int argc, char **argv)
> >   		die_perror("Failed to connect to %s", a.sun_path);
> >   	}
> >   
> > +	debug("Connected to passt/pasta control socket");
> > +
> >   	ret = read_all_buf(s, &hello, sizeof(hello));
> >   	if (ret < 0)
> >   		die_perror("Couldn't read server greeting");
> > @@ -324,11 +384,54 @@ int main(int argc, char **argv)
> >   	while (read_pif_conf(s, &conf))
> >   		;
> >   
> > -	printf("passt/pasta configuration (%s)\n", a.sun_path);
> > -	show_conf(&conf);
> > +	if (!update) {
> > +		printf("passt/pasta configuration (%s)\n", a.sun_path);
> > +		show_conf(&conf);
> > +		goto noupdate;
> > +	}
> > +
> > +	if (show) {
> > +		printf("Previous configuration (%s)\n", a.sun_path);
> > +		show_conf(&conf);
> > +	}
> > +
> > +	inbound = pif_conf_by_name(&conf, "HOST");
> > +	outbound = pif_conf_by_name(&conf, "SPLICE");
> > +
> > +	optind = 0;
> > +	do {
> > +		optname = getopt_long(argc, argv, optstring, options, NULL);
> >   
> > +		switch (optname) {
> > +		case 't':
> > +		case 'u':
> > +			if (!inbound) {
> > +				die("Can't use -%c, no inbound interface",
> > +				    optname);
> > +			}
> > +			fwd_rule_parse(optname, optarg, &inbound->fwd);
> > +			break;
> > +		case 'T':
> > +		case 'U':
> > +			if (!outbound) {
> > +				die("Can't use -%c, no outbound interface",
> > +				    optname);
> > +			}
> > +			fwd_rule_parse(optname, optarg, &outbound->fwd);
> > +			break;
> > +		default:
> > +			continue;
> > +		}
> > +	} while (optname != -1);
> > +
> > +	if (show) {
> > +		printf("Updated configuration (%s)\n", a.sun_path);
> > +		show_conf(&conf);
> > +	}
> > +
> > +noupdate:
> >   	if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0)
> >   		die_perror("Error shutting down control socket");
> > -
> > +			  
> 
> Unrelated change.

Dropped in v7.

> >   	exit(0);
> >   }  

-- 
Stefano