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

[Stefano Brivio <sbrivio@redhat.com>]

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