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

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

[David Gibson]

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);
+	}
+
+	for (i = first; i <= last; i++) {
+		if (bitmap_isset(exclude, i))
+			continue;
+
+		if (bitmap_isset(fwd->map, i)) {
+			warn(
+"Altering mapping of already mapped port number: %s", optarg);
+		}
+
+		bitmap_set(fwd->map, i);
+		fwd->delta[i] = to - first;
+
+		if (optname == 't')
+			ret = tcp_sock_init(c, addr, ifname, i);
+		else if (optname == 'u')
+			ret = udp_sock_init(c, 0, addr, ifname, i);
+		else
+			/* No way to check in advance for -T and -U */
+			ret = 0;
+
+		if (ret == -ENFILE || ret == -EMFILE) {
+			die("Can't open enough sockets for port specifier: %s",
+			    optarg);
+		}
+
+		if (!ret) {
+			bound_one = true;
+		} else if (!weak) {
+			die("Failed to bind port %u (%s) for option '-%c %s'",
+			    i, strerror_(-ret), optname, optarg);
+		}
+	}
+
+	if (!bound_one)
+		die("Failed to bind any port for '-%c %s'", optname, optarg);
+}
+
 /**
  * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets
  * @c:		Execution context
@@ -135,10 +204,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 {
 	union inany_addr addr_buf = inany_any6, *addr = &addr_buf;
 	char buf[BUFSIZ], *spec, *ifname = NULL, *p;
-	bool exclude_only = true, bound_one = false;
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
+	bool exclude_only = true;
 	unsigned i;
-	int ret;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -173,32 +241,15 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 
 		fwd->mode = FWD_ALL;
 
-		/* Skip port 0.  It has special meaning for many socket APIs, so
-		 * trying to bind it is not really safe.
-		 */
-		for (i = 1; i < NUM_PORTS; i++) {
+		/* Exclude ephemeral ports */
+		for (i = 0; i < NUM_PORTS; i++)
 			if (fwd_port_is_ephemeral(i))
-				continue;
-
-			bitmap_set(fwd->map, i);
-			if (optname == 't') {
-				ret = tcp_sock_init(c, NULL, NULL, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			} else if (optname == 'u') {
-				ret = udp_sock_init(c, 0, NULL, NULL, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			}
-		}
-
-		if (!bound_one)
-			goto bind_all_fail;
+				bitmap_set(exclude, i);
 
+		conf_ports_range_except(c, optname, optarg, fwd,
+					NULL, NULL,
+					1, NUM_PORTS - 1, exclude,
+					1, true);
 		return;
 	}
 
@@ -275,37 +326,15 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 	} while ((p = next_chunk(p, ',')));
 
 	if (exclude_only) {
-		/* Skip port 0.  It has special meaning for many socket APIs, so
-		 * trying to bind it is not really safe.
-		 */
-		for (i = 1; i < NUM_PORTS; i++) {
-			if (fwd_port_is_ephemeral(i) ||
-			    bitmap_isset(exclude, i))
-				continue;
-
-			bitmap_set(fwd->map, i);
-
-			if (optname == 't') {
-				ret = tcp_sock_init(c, addr, ifname, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			} else if (optname == 'u') {
-				ret = udp_sock_init(c, 0, addr, ifname, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			} else {
-				/* No way to check in advance for -T and -U */
-				bound_one = true;
-			}
-		}
-
-		if (!bound_one)
-			goto bind_all_fail;
+		/* Exclude ephemeral ports */
+		for (i = 0; i < NUM_PORTS; i++)
+			if (fwd_port_is_ephemeral(i))
+				bitmap_set(exclude, i);
 
+		conf_ports_range_except(c, optname, optarg, fwd,
+					addr, ifname,
+					1, NUM_PORTS - 1, exclude,
+					1, true);
 		return;
 	}
 
@@ -334,40 +363,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		if ((*p != '\0')  && (*p != ',')) /* Garbage after the ranges */
 			goto bad;
 
-		for (i = orig_range.first; i <= orig_range.last; i++) {
-			if (bitmap_isset(fwd->map, i))
-				warn(
-"Altering mapping of already mapped port number: %s", optarg);
-
-			if (bitmap_isset(exclude, i))
-				continue;
-
-			bitmap_set(fwd->map, i);
-
-			fwd->delta[i] = mapped_range.first - orig_range.first;
-
-			ret = 0;
-			if (optname == 't')
-				ret = tcp_sock_init(c, addr, ifname, i);
-			else if (optname == 'u')
-				ret = udp_sock_init(c, 0, addr, ifname, i);
-			if (ret)
-				goto bind_fail;
-		}
+		conf_ports_range_except(c, optname, optarg, fwd,
+					addr, ifname,
+					orig_range.first, orig_range.last,
+					exclude,
+					mapped_range.first, false);
 	} while ((p = next_chunk(p, ',')));
 
 	return;
-enfile:
-	die("Can't open enough sockets for port specifier: %s", optarg);
 bad:
 	die("Invalid port specifier %s", optarg);
 mode_conflict:
 	die("Port forwarding mode '%s' conflicts with previous mode", optarg);
-bind_fail:
-	die("Failed to bind port %u (%s) for option '-%c %s', exiting",
-	    i, strerror_(-ret), optname, optarg);
-bind_all_fail:
-	die("Failed to bind any port for '-%c %s', exiting", optname, optarg);
 }
 
 /**
-- 
@@ -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);
+	}
+
+	for (i = first; i <= last; i++) {
+		if (bitmap_isset(exclude, i))
+			continue;
+
+		if (bitmap_isset(fwd->map, i)) {
+			warn(
+"Altering mapping of already mapped port number: %s", optarg);
+		}
+
+		bitmap_set(fwd->map, i);
+		fwd->delta[i] = to - first;
+
+		if (optname == 't')
+			ret = tcp_sock_init(c, addr, ifname, i);
+		else if (optname == 'u')
+			ret = udp_sock_init(c, 0, addr, ifname, i);
+		else
+			/* No way to check in advance for -T and -U */
+			ret = 0;
+
+		if (ret == -ENFILE || ret == -EMFILE) {
+			die("Can't open enough sockets for port specifier: %s",
+			    optarg);
+		}
+
+		if (!ret) {
+			bound_one = true;
+		} else if (!weak) {
+			die("Failed to bind port %u (%s) for option '-%c %s'",
+			    i, strerror_(-ret), optname, optarg);
+		}
+	}
+
+	if (!bound_one)
+		die("Failed to bind any port for '-%c %s'", optname, optarg);
+}
+
 /**
  * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets
  * @c:		Execution context
@@ -135,10 +204,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 {
 	union inany_addr addr_buf = inany_any6, *addr = &addr_buf;
 	char buf[BUFSIZ], *spec, *ifname = NULL, *p;
-	bool exclude_only = true, bound_one = false;
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
+	bool exclude_only = true;
 	unsigned i;
-	int ret;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -173,32 +241,15 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 
 		fwd->mode = FWD_ALL;
 
-		/* Skip port 0.  It has special meaning for many socket APIs, so
-		 * trying to bind it is not really safe.
-		 */
-		for (i = 1; i < NUM_PORTS; i++) {
+		/* Exclude ephemeral ports */
+		for (i = 0; i < NUM_PORTS; i++)
 			if (fwd_port_is_ephemeral(i))
-				continue;
-
-			bitmap_set(fwd->map, i);
-			if (optname == 't') {
-				ret = tcp_sock_init(c, NULL, NULL, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			} else if (optname == 'u') {
-				ret = udp_sock_init(c, 0, NULL, NULL, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			}
-		}
-
-		if (!bound_one)
-			goto bind_all_fail;
+				bitmap_set(exclude, i);
 
+		conf_ports_range_except(c, optname, optarg, fwd,
+					NULL, NULL,
+					1, NUM_PORTS - 1, exclude,
+					1, true);
 		return;
 	}
 
@@ -275,37 +326,15 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 	} while ((p = next_chunk(p, ',')));
 
 	if (exclude_only) {
-		/* Skip port 0.  It has special meaning for many socket APIs, so
-		 * trying to bind it is not really safe.
-		 */
-		for (i = 1; i < NUM_PORTS; i++) {
-			if (fwd_port_is_ephemeral(i) ||
-			    bitmap_isset(exclude, i))
-				continue;
-
-			bitmap_set(fwd->map, i);
-
-			if (optname == 't') {
-				ret = tcp_sock_init(c, addr, ifname, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			} else if (optname == 'u') {
-				ret = udp_sock_init(c, 0, addr, ifname, i);
-				if (ret == -ENFILE || ret == -EMFILE)
-					goto enfile;
-				if (!ret)
-					bound_one = true;
-			} else {
-				/* No way to check in advance for -T and -U */
-				bound_one = true;
-			}
-		}
-
-		if (!bound_one)
-			goto bind_all_fail;
+		/* Exclude ephemeral ports */
+		for (i = 0; i < NUM_PORTS; i++)
+			if (fwd_port_is_ephemeral(i))
+				bitmap_set(exclude, i);
 
+		conf_ports_range_except(c, optname, optarg, fwd,
+					addr, ifname,
+					1, NUM_PORTS - 1, exclude,
+					1, true);
 		return;
 	}
 
@@ -334,40 +363,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		if ((*p != '\0')  && (*p != ',')) /* Garbage after the ranges */
 			goto bad;
 
-		for (i = orig_range.first; i <= orig_range.last; i++) {
-			if (bitmap_isset(fwd->map, i))
-				warn(
-"Altering mapping of already mapped port number: %s", optarg);
-
-			if (bitmap_isset(exclude, i))
-				continue;
-
-			bitmap_set(fwd->map, i);
-
-			fwd->delta[i] = mapped_range.first - orig_range.first;
-
-			ret = 0;
-			if (optname == 't')
-				ret = tcp_sock_init(c, addr, ifname, i);
-			else if (optname == 'u')
-				ret = udp_sock_init(c, 0, addr, ifname, i);
-			if (ret)
-				goto bind_fail;
-		}
+		conf_ports_range_except(c, optname, optarg, fwd,
+					addr, ifname,
+					orig_range.first, orig_range.last,
+					exclude,
+					mapped_range.first, false);
 	} while ((p = next_chunk(p, ',')));
 
 	return;
-enfile:
-	die("Can't open enough sockets for port specifier: %s", optarg);
 bad:
 	die("Invalid port specifier %s", optarg);
 mode_conflict:
 	die("Port forwarding mode '%s' conflicts with previous mode", optarg);
-bind_fail:
-	die("Failed to bind port %u (%s) for option '-%c %s', exiting",
-	    i, strerror_(-ret), optname, optarg);
-bind_all_fail:
-	die("Failed to bind any port for '-%c %s', exiting", optname, optarg);
 }
 
 /**
-- 
2.48.1

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

[Stefano Brivio]

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'

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

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.

-- 
Stefano

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

[David Gibson]

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

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

[Paul Holzinger]

On 15/03/2025 00:50, 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'
>
> ...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.
Why would this not work for UDP? bind() wise you can still bind 0 fine 
and get a free port assigned?
>
> 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.
>
> More than that: that could actually be the only race-free possibility
> of picking and forwarding a port where the number doesn't matter.

Yes it could be useful for podman but then it should also work with udp. 
I am less worried about the tests, this issue is in podman proper as you 
can do "-p 80", then podman assigns a free host port. Except that this 
is super broken in podman because we do this once when we create the 
container so this is totally racy and non conflict free[1]. The thing of 
course is for podman we have to deal with like 4 other port forwarder 
implementations that we would need to support. As such I don't see us 
ever finding time to properly fix it unless it magically gets a ton of 
priority. So if pasta does not support for it I have no problems with 
that, however maybe one day we like to reconsider.

[1] 
https://github.com/containers/podman/issues/10205#issuecomment-1010055023

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

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

[Stefano Brivio]

On Mon, 17 Mar 2025 12:50:36 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> On 15/03/2025 00:50, 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'
> >
> > ...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.  
>
> Why would this not work for UDP? bind() wise you can still bind 0 fine 
> and get a free port assigned?

The bind() part itself would work, but with the current implementation
we wouldn't be able to track flows corresponding to this specific port
forwarding, so I expect that the "return" (outbound) traffic won't
work.

It's a matter of implementation (or lack thereof), we could get it to
work with a getsockname() after bind().

Before this change, it happened to work *by mistake* for TCP, not for
UDP. With this change, it doesn't work for TCP. We can add it back with
a proper syntax (-t ...any?), as David mentioned.

> > 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.
> >
> > More than that: that could actually be the only race-free possibility
> > of picking and forwarding a port where the number doesn't matter.  
> 
> Yes it could be useful for podman but then it should also work with udp.

We can get it to work if needed. We would need, I guess:

- that getsockname() for UDP, whatever is missing for the UDP case

- a new configuration sub-option

- documentation

> I am less worried about the tests, this issue is in podman proper as you 
> can do "-p 80", then podman assigns a free host port. Except that this 
> is super broken in podman because we do this once when we create the 
> container so this is totally racy and non conflict free[1].

Ouch, I wasn't aware of that. For pasta it should be relatively easy to
do that in a race-free way, because the kernel guarantees that.

> The thing of 
> course is for podman we have to deal with like 4 other port forwarder 
> implementations that we would need to support. As such I don't see us 
> ever finding time to properly fix it unless it magically gets a ton of 
> priority. So if pasta does not support for it I have no problems with 
> that, however maybe one day we like to reconsider.
> 
> [1] 
> https://github.com/containers/podman/issues/10205#issuecomment-1010055023

I would wait for David's feedback on this, but to me it looks like a
small-ish thing we can add without much thinking and planning.

I'm not sure you can close that issue if we implement it in pasta as
long as forwarding is done like it's done now for custom networks, but
the issue would look less serious I guess.

I don't know about the Podman side of it, but probably that would look
trivial to you (-t any:80 maybe? or -t :80 ?).

-- 
Stefano

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

[Paul Holzinger]

On 18/03/2025 17:54, Stefano Brivio wrote:
> On Mon, 17 Mar 2025 12:50:36 +0100
> Paul Holzinger <pholzing@redhat.com> wrote:
>
>> On 15/03/2025 00:50, 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'
>>>
>>> ...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.
>> Why would this not work for UDP? bind() wise you can still bind 0 fine
>> and get a free port assigned?
> The bind() part itself would work, but with the current implementation
> we wouldn't be able to track flows corresponding to this specific port
> forwarding, so I expect that the "return" (outbound) traffic won't
> work.
>
> It's a matter of implementation (or lack thereof), we could get it to
> work with a getsockname() after bind().
>
> Before this change, it happened to work *by mistake* for TCP, not for
> UDP. With this change, it doesn't work for TCP. We can add it back with
> a proper syntax (-t ...any?), as David mentioned.
>
>>> 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.
>>>
>>> More than that: that could actually be the only race-free possibility
>>> of picking and forwarding a port where the number doesn't matter.
>> Yes it could be useful for podman but then it should also work with udp.
> We can get it to work if needed. We would need, I guess:
>
> - that getsockname() for UDP, whatever is missing for the UDP case
>
> - a new configuration sub-option
>
> - documentation
>
>> I am less worried about the tests, this issue is in podman proper as you
>> can do "-p 80", then podman assigns a free host port. Except that this
>> is super broken in podman because we do this once when we create the
>> container so this is totally racy and non conflict free[1].
> Ouch, I wasn't aware of that. For pasta it should be relatively easy to
> do that in a race-free way, because the kernel guarantees that.
>
>> The thing of
>> course is for podman we have to deal with like 4 other port forwarder
>> implementations that we would need to support. As such I don't see us
>> ever finding time to properly fix it unless it magically gets a ton of
>> priority. So if pasta does not support for it I have no problems with
>> that, however maybe one day we like to reconsider.
>>
>> [1]
>> https://github.com/containers/podman/issues/10205#issuecomment-1010055023
> I would wait for David's feedback on this, but to me it looks like a
> small-ish thing we can add without much thinking and planning.
>
> I'm not sure you can close that issue if we implement it in pasta as
> long as forwarding is done like it's done now for custom networks, but
> the issue would look less serious I guess.
>
> I don't know about the Podman side of it, but probably that would look
> trivial to you (-t any:80 maybe? or -t :80 ?).

I guess something like -t 0:80 could also work. That would allow me to 
store unassigned ports as 0 and the the convert to pasta cli code would 
not need any special handling for this case.

Overall it will be more complicated in Podman which is why I never 
bothered to take this on. The issue is that commands like podman inspect 
or podman port need to know the actual ports that were assigned. So it 
would need some form of interface from pasta that prints out which host 
port it uses for each namespace port. Then podman must need to gain 
support to store two different set of ports, dynamic and static 
(currently) so we can keep track of the ports pasta returned to us.

All stuff we can implement but until someone pushes hard for it I don't 
think it will ever make it. I think there are better network things we 
can spend our time on.

-- 
Paul Holzinger

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

[Stefano Brivio]

On Tue, 18 Mar 2025 18:15:27 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> On 18/03/2025 17:54, Stefano Brivio wrote:
> > On Mon, 17 Mar 2025 12:50:36 +0100
> > Paul Holzinger <pholzing@redhat.com> wrote:
> >  
> >> On 15/03/2025 00:50, 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'
> >>>
> >>> ...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.  
> >> Why would this not work for UDP? bind() wise you can still bind 0 fine
> >> and get a free port assigned?  
> > The bind() part itself would work, but with the current implementation
> > we wouldn't be able to track flows corresponding to this specific port
> > forwarding, so I expect that the "return" (outbound) traffic won't
> > work.
> >
> > It's a matter of implementation (or lack thereof), we could get it to
> > work with a getsockname() after bind().
> >
> > Before this change, it happened to work *by mistake* for TCP, not for
> > UDP. With this change, it doesn't work for TCP. We can add it back with
> > a proper syntax (-t ...any?), as David mentioned.
> >  
> >>> 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.
> >>>
> >>> More than that: that could actually be the only race-free possibility
> >>> of picking and forwarding a port where the number doesn't matter.  
> >> Yes it could be useful for podman but then it should also work with udp.  
> > We can get it to work if needed. We would need, I guess:
> >
> > - that getsockname() for UDP, whatever is missing for the UDP case
> >
> > - a new configuration sub-option
> >
> > - documentation
> >  
> >> I am less worried about the tests, this issue is in podman proper as you
> >> can do "-p 80", then podman assigns a free host port. Except that this
> >> is super broken in podman because we do this once when we create the
> >> container so this is totally racy and non conflict free[1].  
> > Ouch, I wasn't aware of that. For pasta it should be relatively easy to
> > do that in a race-free way, because the kernel guarantees that.
> >  
> >> The thing of
> >> course is for podman we have to deal with like 4 other port forwarder
> >> implementations that we would need to support. As such I don't see us
> >> ever finding time to properly fix it unless it magically gets a ton of
> >> priority. So if pasta does not support for it I have no problems with
> >> that, however maybe one day we like to reconsider.
> >>
> >> [1]
> >> https://github.com/containers/podman/issues/10205#issuecomment-1010055023  
> > I would wait for David's feedback on this, but to me it looks like a
> > small-ish thing we can add without much thinking and planning.
> >
> > I'm not sure you can close that issue if we implement it in pasta as
> > long as forwarding is done like it's done now for custom networks, but
> > the issue would look less serious I guess.
> >
> > I don't know about the Podman side of it, but probably that would look
> > trivial to you (-t any:80 maybe? or -t :80 ?).  
> 
> I guess something like -t 0:80 could also work. That would allow me to 
> store unassigned ports as 0 and the the convert to pasta cli code would 
> not need any special handling for this case.
> 
> Overall it will be more complicated in Podman which is why I never 
> bothered to take this on. The issue is that commands like podman inspect 
> or podman port need to know the actual ports that were assigned.

Ah, right, I forgot about that.

> So it 
> would need some form of interface from pasta that prints out which host 
> port it uses for each namespace port. Then podman must need to gain 
> support to store two different set of ports, dynamic and static 
> (currently) so we can keep track of the ports pasta returned to us.

That doesn't really sound trivial, I was underestimating this.

> All stuff we can implement but until someone pushes hard for it I don't 
> think it will ever make it. I think there are better network things we 
> can spend our time on.

...so we should rather defer this I guess. Probably it will make sense
to revisit this together with the new forwarding configuration interface
once we have it implemented.

-- 
Stefano