* [PATCH] conf: Unify several paths in conf_ports()
@ 2025-02-19 3:54 David Gibson
2025-02-19 5:37 ` Stefano Brivio
0 siblings, 1 reply; 3+ messages in thread
From: David Gibson @ 2025-02-19 3:54 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: 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.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
conf.c | 175 +++++++++++++++++++++++++++++----------------------------
1 file changed, 90 insertions(+), 85 deletions(-)
diff --git a/conf.c b/conf.c
index 18017f51..f862845b 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,14 @@ 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++) {
+ /* Also 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 +325,14 @@ 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 +361,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,14 @@ 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++) {
+ /* Also 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 +325,14 @@ 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 +361,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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] conf: Unify several paths in conf_ports()
2025-02-19 3:54 [PATCH] conf: Unify several paths in conf_ports() David Gibson
@ 2025-02-19 5:37 ` Stefano Brivio
2025-02-20 0:25 ` David Gibson
0 siblings, 1 reply; 3+ messages in thread
From: Stefano Brivio @ 2025-02-19 5:37 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Wed, 19 Feb 2025 14:54:44 +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.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> conf.c | 175 +++++++++++++++++++++++++++++----------------------------
> 1 file changed, 90 insertions(+), 85 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index 18017f51..f862845b 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)
...ouch, bool anything_else? I think this patch is strictly better than
the existing duplication, so I'm fine with it as it is. I don't see any
obvious way to make the prototype smaller (other than things that would
reduce readability by hiding information), so... yeah. I just wanted to
mention that the prototype is a bit heavy.
> +{
> + 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,14 @@ 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++) {
> + /* Also exclude ephemeral ports */
"Also" implies the reader figures out that conf_ports_range_except()
skips port zero while looking at this line, which is not really
something you can expect, I guess.
We could probably just drop this comment, or change it as you did below.
> + 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);
I would add an extra blank line here, otherwise it's quite confusing.
> + conf_ports_range_except(c, optname, optarg, fwd,
> + NULL, NULL,
> + 1, NUM_PORTS - 1, exclude,
> + 1, true);
> return;
> }
>
> @@ -275,37 +325,14 @@ 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);
Same here, a blank line would be nice.
> + conf_ports_range_except(c, optname, optarg, fwd,
> + addr, ifname,
> + 1, NUM_PORTS - 1, exclude,
> + 1, true);
> return;
> }
>
> @@ -334,40 +361,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);
> }
>
> /**
--
Stefano
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] conf: Unify several paths in conf_ports()
2025-02-19 5:37 ` Stefano Brivio
@ 2025-02-20 0:25 ` David Gibson
0 siblings, 0 replies; 3+ messages in thread
From: David Gibson @ 2025-02-20 0:25 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 9511 bytes --]
On Wed, Feb 19, 2025 at 06:37:32AM +0100, Stefano Brivio wrote:
> On Wed, 19 Feb 2025 14:54:44 +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.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > conf.c | 175 +++++++++++++++++++++++++++++----------------------------
> > 1 file changed, 90 insertions(+), 85 deletions(-)
> >
> > diff --git a/conf.c b/conf.c
> > index 18017f51..f862845b 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)
>
> ...ouch, bool anything_else? I think this patch is strictly better than
> the existing duplication, so I'm fine with it as it is. I don't see any
> obvious way to make the prototype smaller (other than things that would
> reduce readability by hiding information), so... yeah. I just wanted to
> mention that the prototype is a bit heavy.
Yes, it certainly is. Like you, I concluded it's still better than
the existing duplication.
> > +{
> > + 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,14 @@ 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++) {
> > + /* Also exclude ephemeral ports */
>
> "Also" implies the reader figures out that conf_ports_range_except()
> skips port zero while looking at this line, which is not really
> something you can expect, I guess.
Well.. it's not conf_ports_range_except() excluding it, but the fact
we past '1', not '0' as the @first parameter.
> We could probably just drop this comment, or change it as you did
> below.
But yeah, I'll update it like below.
>
> > + 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);
>
> I would add an extra blank line here, otherwise it's quite confusing.
Good call, done.
> > + conf_ports_range_except(c, optname, optarg, fwd,
> > + NULL, NULL,
> > + 1, NUM_PORTS - 1, exclude,
> > + 1, true);
> > return;
> > }
> >
> > @@ -275,37 +325,14 @@ 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);
>
> Same here, a blank line would be nice.
Done.
> > + conf_ports_range_except(c, optname, optarg, fwd,
> > + addr, ifname,
> > + 1, NUM_PORTS - 1, exclude,
> > + 1, true);
> > return;
> > }
> >
> > @@ -334,40 +361,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);
> > }
> >
> > /**
>
--
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 --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-02-20 0:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-19 3:54 [PATCH] conf: Unify several paths in conf_ports() David Gibson
2025-02-19 5:37 ` Stefano Brivio
2025-02-20 0:25 ` David Gibson
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).