Re: [PATCH 06/18] fwd: Better split forwarding rule specification from associated sockets

Stefano Brivio
2026-04-07T23:16:34Z

On Tue, 7 Apr 2026 13:16:18 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> 6dad076df037 ("fwd: Split forwarding rule specification from its
> implementation state") created struct fwd_rule_state with a forwarding rule
> plus the table of sockets used for its implementation. It turns out this
> is quite awkward for sharing rule parsing code between passt and the
> upcoming configuration client.

Indeed, I hated it, in that short moment I had to fiddle with it. Thanks
for coming up with a cleaner solution.

>
> Instead keep the index of listening sockets in a parallel array in
> struct fwd_table.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> fwd.c | 73 ++++++++++++++++++++++++++++++-----------------------------
> fwd.h | 16 ++++---------
> 2 files changed, 41 insertions(+), 48 deletions(-)
>
> diff --git a/fwd.c b/fwd.c
> index e09b42fe..7e0edc38 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -363,7 +363,7 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags,
> /* Flags which can be set from the caller */
> const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY;
> unsigned num = (unsigned)last - first + 1;
> - struct fwd_rule_state *new;
> + struct fwd_rule *new;
> unsigned i, port;
>
> assert(!(flags & ~allowed_flags));
> @@ -379,7 +379,7 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags,
> /* Check for any conflicting entries */
> for (i = 0; i < fwd->count; i++) {
> char newstr[INANY_ADDRSTRLEN], rulestr[INANY_ADDRSTRLEN];
> - const struct fwd_rule *rule = &fwd->rules[i].rule;
> + const struct fwd_rule *rule = &fwd->rules[i];
>
> if (proto != rule->proto)
> /* Non-conflicting protocols */
> @@ -399,38 +399,38 @@ void fwd_rule_add(struct fwd_table *fwd, uint8_t proto, uint8_t flags,
> rule->first, rule->last);
> }
>
> - new = &fwd->rules[fwd->count++];
> - new->rule.proto = proto;
> - new->rule.flags = flags;
> + new = &fwd->rules[fwd->count];
> + new->proto = proto;
> + new->flags = flags;
>
> if (addr) {
> - new->rule.addr = *addr;
> + new->addr = *addr;
> } else {
> - new->rule.addr = inany_any6;
> - new->rule.flags |= FWD_DUAL_STACK_ANY;
> + new->addr = inany_any6;
> + new->flags |= FWD_DUAL_STACK_ANY;
> }
>
> - memset(new->rule.ifname, 0, sizeof(new->rule.ifname));
> + memset(new->ifname, 0, sizeof(new->ifname));
> if (ifname) {
> int ret;
>
> - ret = snprintf(new->rule.ifname, sizeof(new->rule.ifname),
> + ret = snprintf(new->ifname, sizeof(new->ifname),
> "%s", ifname);
> - if (ret <= 0 || (size_t)ret >= sizeof(new->rule.ifname))
> + if (ret <= 0 || (size_t)ret >= sizeof(new->ifname))
> die("Invalid interface name: %s", ifname);
> }
>
> assert(first <= last);
> - new->rule.first = first;
> - new->rule.last = last;
> + new->first = first;
> + new->last = last;
> + new->to = to;
>
> - new->rule.to = to;
> + fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count];
> + for (port = new->first; port <= new->last; port++)
> + fwd->rulesocks[fwd->count][port - new->first] = -1;
>
> - new->socks = &fwd->socks[fwd->sock_count];
> + fwd->count++;
> fwd->sock_count += num;
> -
> - for (port = new->rule.first; port <= new->rule.last; port++)
> - new->socks[port - new->rule.first] = -1;
> }
>
> /**
> @@ -466,7 +466,7 @@ const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd,
>
> if (hint >= 0) {
> char ostr[INANY_ADDRSTRLEN], rstr[INANY_ADDRSTRLEN];
> - const struct fwd_rule *rule = &fwd->rules[hint].rule;
> + const struct fwd_rule *rule = &fwd->rules[hint];
>
> assert((unsigned)hint < fwd->count);
> if (fwd_rule_match(rule, ini, proto))
> @@ -480,8 +480,8 @@ const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd,
> }
>
> for (i = 0; i < fwd->count; i++) {
> - if (fwd_rule_match(&fwd->rules[i].rule, ini, proto))
> - return &fwd->rules[i].rule;
> + if (fwd_rule_match(&fwd->rules[i], ini, proto))
> + return &fwd->rules[i];
> }
>
> return NULL;
> @@ -496,7 +496,7 @@ void fwd_rules_print(const struct fwd_table *fwd)
> unsigned i;
>
> for (i = 0; i < fwd->count; i++) {
> - const struct fwd_rule *rule = &fwd->rules[i].rule;
> + const struct fwd_rule *rule = &fwd->rules[i];
> const char *percent = *rule->ifname ? "%" : "";
> const char *weak = "", *scan = "";
> char addr[INANY_ADDRSTRLEN];
> @@ -533,9 +533,9 @@ void fwd_rules_print(const struct fwd_table *fwd)
> static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx,
> const uint8_t *tcp, const uint8_t *udp)
> {
> - const struct fwd_rule_state *rs = &c->fwd[pif]->rules[idx];
> - const struct fwd_rule *rule = &rs->rule;
> + const struct fwd_rule *rule = &c->fwd[pif]->rules[idx];
> const union inany_addr *addr = fwd_rule_addr(rule);
> + int *socks = c->fwd[pif]->rulesocks[idx];
> const char *ifname = rule->ifname;
> const uint8_t *map = NULL;
> bool bound_one = false;
> @@ -555,7 +555,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx,
> }
>
> for (port = rule->first; port <= rule->last; port++) {
> - int fd = rs->socks[port - rule->first];
> + int fd = socks[port - rule->first];
>
> if (map && !bitmap_isset(map, port)) {
> /* We don't want to listen on this port */
> @@ -563,7 +563,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx,
> /* We already are, so stop */
> epoll_del(c->epollfd, fd);
> close(fd);
> - rs->socks[port - rule->first] = -1;
> + socks[port - rule->first] = -1;
> }
> continue;
> }
> @@ -595,7 +595,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx,
> continue;
> }
>
> - rs->socks[port - rule->first] = fd;
> + socks[port - rule->first] = fd;
> bound_one = true;
> }
>
> @@ -685,11 +685,12 @@ void fwd_listen_close(const struct fwd_table *fwd)
> unsigned i;
>
> for (i = 0; i < fwd->count; i++) {
> - const struct fwd_rule_state *rs = &fwd->rules[i];
> + const struct fwd_rule *rule = &fwd->rules[i];
> + int *socks = fwd->rulesocks[i];
> unsigned port;
>
> - for (port = rs->rule.first; port <= rs->rule.last; port++) {
> - int *fdp = &rs->socks[port - rs->rule.first];
> + for (port = rule->first; port <= rule->last; port++) {
> + int *fdp = &socks[port - rule->first];
> if (*fdp >= 0) {
> close(*fdp);
> *fdp = -1;
> @@ -769,8 +770,8 @@ static bool has_scan_rules(const struct fwd_table *fwd, uint8_t proto)
> unsigned i;
>
> for (i = 0; i < fwd->count; i++) {
> - if (fwd->rules[i].rule.proto == proto &&
> - fwd->rules[i].rule.flags & FWD_SCAN)
> + if (fwd->rules[i].proto == proto &&
> + fwd->rules[i].flags & FWD_SCAN)
> return true;
> }
> return false;
> @@ -838,14 +839,14 @@ static void current_listen_map(uint8_t *map, const struct fwd_table *fwd,
> memset(map, 0, PORT_BITMAP_SIZE);
>
> for (i = 0; i < fwd->count; i++) {
> - const struct fwd_rule_state *rs = &fwd->rules[i];
> + const struct fwd_rule *rule = &fwd->rules[i];
> unsigned port;
>
> - if (rs->rule.proto != proto)
> + if (rule->proto != proto)
> continue;
>
> - for (port = rs->rule.first; port <= rs->rule.last; port++) {
> - if (rs->socks[port - rs->rule.first] >= 0)
> + for (port = rule->first; port <= rule->last; port++) {
> + if (fwd->rulesocks[i][port - rule->first] >= 0)
> bitmap_set(map, port);
> }
> }
> diff --git a/fwd.h b/fwd.h
> index 33600cbf..83ee9b2e 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -26,16 +26,6 @@ struct flowside;
> void fwd_probe_ephemeral(void);
> void fwd_port_map_ephemeral(uint8_t *map);
>
> -/**
> - * struct fwd_rule_state - Forwarding rule and associated state
> - * @rule: Rule specification
> - * @socks: Array of listening sockets for this entry
> - */
> -struct fwd_rule_state {
> - struct fwd_rule rule;
> - int *socks;
> -};
> -
> #define FWD_RULE_BITS 8
> #define MAX_FWD_RULES MAX_FROM_BITS(FWD_RULE_BITS)
> #define FWD_NO_HINT (-1)
> @@ -61,15 +51,17 @@ struct fwd_listen_ref {
> #define MAX_LISTEN_SOCKS (NUM_PORTS * 5)
>
> /**
> - * struct fwd_table - Table of forwarding rules (per initiating pif)
> + * struct fwd_table - Forwarding state (per initiating pif)
> * @count: Number of forwarding rules
> * @rules: Array of forwarding rules
> + * @rulesocks: Pointers to socket arrays per-rule

I don't see this as particularly descriptive (which sockets? What's
the array size?). I'm thinking of something like:

@socks_ref: Per-rule pointers to associated @socks, @sock_count of them

> * @sock_count: Number of entries used in @socks
> * @socks: Listening sockets for forwarding
> */
> struct fwd_table {
> unsigned count;
> - struct fwd_rule_state rules[MAX_FWD_RULES];
> + struct fwd_rule rules[MAX_FWD_RULES];
> + int *rulesocks[MAX_FWD_RULES];
> unsigned sock_count;
> int socks[MAX_LISTEN_SOCKS];
> };

--
Stefano