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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 5 May 2026 01:11:17 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

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

^^^ don't make sense for passt, I meant.

> 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