Re: [WIP PATCH] Introduce configuration interface and configuration tool (pesto)

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 10 Feb 2026 15:15:36 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Feb 05, 2026 at 12:42:09AM +0100, Stefano Brivio wrote:
> > Draft, very rudimentary. Notably lacking:
> > 
> > - any form of error checking, documentation
> > 
> > - support for anything that's not ports
> > 
> > - delete/insert operations
> > 
> > - proper handling for EPOLLHUP / EPOLLERR
> > 
> > - command responses
> > 
> > - headers, versioning, etc.
> > 
> > Example:
> > 
> >   ./pasta --debug --config-net -c /tmp/pasta.conf -t none
> > 
> >   ./pesto /tmp/pasta.conf add 8889
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Have some time to look at this, while Bass Strait glides past outside.
> 
> For the most part, LGTM, as far as it goes.

Just note that I shared this as early as I could not so much to get a
review (see above as to why it will get half rewritten anyway), rather
so that you could use it to sketch delete operations in the new table,
as we discussed.

I will go through your comments anyway once it's more or less complete,
but many might be irrelevant at that point.

Just a couple of examples of that, and some replies which might make
vaguely sense:

> [...]
>
> > +	if (!getsockopt(c->fd_conf, SOL_SOCKET, SO_PEERCRED, &uc, &len))
> > +		info("Accepted configuration client, PID %i", uc.pid);  
> 
> Is displaying the client's PID actually going to be useful?

I'm not sure, but this is mostly copied and pasted from passt-repair
code for the sake of time.

> By the
> time this happens, we'll have isolated our own pidns, but the client
> will likely be outside our pidns.  The client's pid as translated into
> our pidns will therefore not really be meaningful (probably 0 or -1 or
> something).

Correct, it's usually 0, unless we run in foreground. In that case it
was actually helpful (more like debug() stuff though) with passt-repair.

> > +	ref.fd = c->fd_conf;
> > +
> > +	rc = epoll_add(c->epollfd, EPOLLHUP | EPOLLET, ref);  
> 
> EPOLLHUP is implicitly included.  Surely we need EPOLLIN as well, no?

That was my intention and I forgot, but actually it works pretty well
with just EPOLLHUP because I just get one event, read everything...
probably a bit too "risky" for actual non-prototyping operation though.

> Since we're already assuming we get the config ops in aligned chunks,
> can we avoid the global buffer and read ops one at a time into a local
> buffer?

It depends on how this is implemented on the server side: the current
idea (as we discussed) would be to have a shadow table and replace it
in a single transaction, so in that case we would need just another
copy of the table, but not a permanent list of operations.

So, yes, this will *probably* go away.

> > +	}
> > +
> > +close:
> > +	for (i = 0; i < conf_ops_bytes_read / sizeof(conf_ops[0]); i++) {
> > +		struct fwd_rule *r = &conf_ops[i].r;  
> 
> This is probably already on your plan, but as with migration, I think
> we should explicitly translate from the control stream format, rather
> than embedding struct fwd_rule, so that we decouple the internal rule
> format from the control message format.  Also like migration, I think
> we should make sure the control format is the same, regardless of
> platform specific padding, alignment, and endianness.

Maybe, even though the case is much weaker here. Sure, the client could
run on another machine eventually and we'd need to translate ports...
I'm not sure.

> > +
> > +		debug("%s rule %i, ports %i-%i:%i",
> > +		      conf_ops[i].op == RULE_ADD ? "adding" : "deleting", i,
> > +		      r->first, r->last, r->to);
> > +
> > +		if (conf_ops[i].op == RULE_ADD) {
> > +			fwd_rule_add(&c->tcp.fwd_in, r->flags, &r->addr,
> > +				     /* r->ifname */ NULL,  
> 
> Why ignoring r->ifname?

Because it would have taken me a few minutes minutes to take care of it.

> Also probably in your plan, but we want to update fwd_rule_add() to
> make errors non-fatal as a preliminary to this.

Unless it uses another table (see above) and in that case errors might
need to be fatal.

> > +				     r->first, r->last, r->to);
> > +		} else {
> > +			; /* TODO */
> > +		}
> > +	}
> > +
> > +	fwd_listen_sync(c, &c->tcp.fwd_in, PIF_HOST, IPPROTO_TCP);
> > +
> > +	debug("Closing configuration socket");
> > +	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_conf, NULL);
> > +	close(c->fd_conf);  
> 
> Why always close() after a single batch of ops?

Easier to sketch, no need to keep track of 'count'.

> [...]
>
> > +	prog.len = (unsigned short)sizeof(filter_pesto) /
> > +				   sizeof(filter_pesto[0]);
> > +	prog.filter = filter_pesto;
> > +	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
> > +	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
> > +		fprintf(stderr, "Failed to apply seccomp filter\n");
> > +		_exit(1);  
> 
> The case for seccomp also seems weaker for this client (it's not
> exposed to guest or peer traffic)....

...it might be long lived, we don't know yet. It's exposed to some bits
of data / information from passt.

> > +	}
> > +
> > +	if (argc < 4) {
> > +		fprintf(stderr,
> > +			"Usage: %s PATH add|del SPEC [auto] [weak] \\"
> > +			"[add|del SPEC [auto] [weak]] ...\n",
> > +			argv[0]);  
> 
> IIUC, the code below allows multiple add/del on the same command line,
> which this usage() doesn't reflect.

It does in my opinion, hence the ..., but I don't remember how it's
commonly represented in usage strings.

> > +		_exit(2);  
> 
> ... and especially the case for excluding malloc() (and the subsequent
> complications with exit() and various other things) seems pretty weak
> for this client.

...unless it's long lived.

-- 
Stefano