Re: [PATCH v2] conf: Unify several paths in conf_ports()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4464 bytes --]

On Sat, Mar 15, 2025 at 12:50:28AM +0100, Stefano Brivio wrote:
> On Wed, 12 Mar 2025 14:43:59 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In conf_ports() we have three different paths which actually do the setup
> > of an individual forwarded port: one for the "all" case, one for the
> > exclusions only case and one for the range of ports with possible
> > exclusions case.
> > 
> > We can unify those cases using a new helper which handles a single range
> > of ports, with a bitmap of exclusions.  Although this is slightly longer
> > (largely due to the new helpers function comment), it reduces duplicated
> > logic.  It will also make future improvements to the tracking of port
> > forwards easier.
> > 
> > The new conf_ports_range_except() function has a pretty prodigious
> > parameter list, but I still think it's an overall improvement in conceptual
> > complexity.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c | 173 ++++++++++++++++++++++++++++++---------------------------
> >  1 file changed, 90 insertions(+), 83 deletions(-)
> > 
> > v2:
> >  * Commit message updated slightly, but otherwise unmodified.
> > 
> > 
> > diff --git a/conf.c b/conf.c
> > index 065e7201..4e0099ba 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -123,6 +123,75 @@ static int parse_port_range(const char *s, char **endptr,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * conf_ports_range_except() - Set up forwarding for a range of ports minus a
> > + *                             bitmap of exclusions
> > + * @c:		Execution context
> > + * @optname:	Short option name, t, T, u, or U
> > + * @optarg:	Option argument (port specification)
> > + * @fwd:	Pointer to @fwd_ports to be updated
> > + * @addr:	Listening address
> > + * @ifname:	Listening interface
> > + * @first:	First port to forward
> > + * @last:	Last port to forward
> > + * @exclude:	Bitmap of ports to exclude
> > + * @to:		Port to translate @first to when forwarding
> > + * @weak:	Ignore errors, as long as at least one port is mapped
> > + */
> > +static void conf_ports_range_except(const struct ctx *c, char optname,
> > +				    const char *optarg, struct fwd_ports *fwd,
> > +				    const union inany_addr *addr,
> > +				    const char *ifname,
> > +				    uint16_t first, uint16_t last,
> > +				    const uint8_t *exclude, uint16_t to,
> > +				    bool weak)
> > +{
> > +	bool bound_one = false;
> > +	unsigned i;
> > +	int ret;
> > +
> > +	if (first == 0) {
> > +		die("Can't forward port 0 for option '-%c %s'",
> > +		    optname, optarg);
> > +	}
> 
> This introduces two subtle functional changes that are a bit unexpected
> given the commit message. Before:
> 
> $ ./pasta -t 0
> $
> 
> $ ./pasta -t 0-1025
> Failed to bind port 1 (Permission denied) for option '-t 0-1025', exiting
> 
> After:
> 
> $ ./pasta -t 0
> Can't forward port 0 for option '-t 0'
> 
> $ ./pasta -t 0-1025
> Can't forward port 0 for option '-t 0-1025'

I'd consider both those improvements, since we *aren't* able to
forward port 0.

> 
> ...anyway, I doubt anybody would use -t 0 on purpose (to get a port
> automatically assigned), and while it probably works for TCP (check
> bound ports after starting pasta, use the assigned one), it wouldn't
> necessarily work as expected for UDP if the application relies on our
> flow tracking.
> 
> For TCP, actually, -t 0 might be useful, see e.g. random_free_port() in
> Podman tests (/test/system/helpers.network.bash). We should print the
> port number that was bound, though, and document the feature.

I agree that could be a useful thing to do, but I don't think -t 0
would be a good syntax for it: if it's useful to get pasta to assign
one port it's probably useful to get it to assign multiple ports, and
repeating -t 0 doesn't really make sense for that.

> More than that: that could actually be the only race-free possibility
> of picking and forwarding a port where the number doesn't matter.
> 
> In any case, given that it works by mistake now and it's undocumented,
> let me go ahead and apply this for the moment. We can add the
> "functionality" back later if it makes sense.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]