Re: [PATCH v3 20/25] fwd_rule: Move forwarding rule text formatting to common code

On Wed, 25 Mar 2026 15:42:00 +1100 David Gibson wrote:

On Wed, Mar 25, 2026 at 01:56:34AM +0100, Stefano Brivio wrote:

...

On Mon, 23 Mar 2026 18:37:27 +1100 David Gibson wrote:

...

Move the logic for formatting forwarding rules into strings from fwd_rules_print() into fwd_rule.c where it can be shared with pesto. We also make the function explicitly construct a string, rather than directly printing with info(), for greater flexibility.

Signed-off-by: David Gibson --- Makefile | 11 ++++---- fwd.c | 40 +++--------------------------- fwd_rule.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ fwd_rule.h | 11 ++++++++ 4 files changed, 94 insertions(+), 41 deletions(-) create mode 100644 fwd_rule.c

diff --git a/Makefile b/Makefile index bc325482..44c396e7 100644 --- a/Makefile +++ b/Makefile @@ -38,13 +38,14 @@ FLAGS += -DVERSION=\"$(VERSION)\" FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)

PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ctl.c \ - flow.c fwd.c icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c \ - log.c mld.c ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c \ - pif.c repair.c serialise.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.c \ - udp.c udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c + flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c isolation.c \ + lineread.c log.c mld.c ndp.c netlink.c migrate.c packet.c passt.c \ + pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c tcp_buf.c \ + tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c vhost_user.c \ + virtio.c vu_common.c QRAP_SRCS = qrap.c PASST_REPAIR_SRCS = passt-repair.c -PESTO_SRCS = pesto.c inany.c ip.c serialise.c +PESTO_SRCS = pesto.c fwd_rule.c inany.c ip.c serialise.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)

MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1 diff --git a/fwd.c b/fwd.c index a32d0a20..20409c62 100644 --- a/fwd.c +++ b/fwd.c @@ -304,20 +304,6 @@ parse_err: warn("Unable to parse %s", PORT_RANGE_SYSCTL); }

-/** - * fwd_rule_addr() - Return match address for a rule - * @rule: Forwarding rule - * - * Return: matching address for rule, NULL if it matches all addresses - */ -static const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule) -{ - if (rule->flags & FWD_DUAL_STACK_ANY) - return NULL; - - return &rule->addr; -} - /** * fwd_port_map_ephemeral() - Mark ephemeral ports in a bitmap * @map: Bitmap to update @@ -497,28 +483,10 @@ 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 char *percent = *rule->ifname ? "%" : ""; - const char *weak = "", *scan = ""; - char addr[INANY_ADDRSTRLEN]; - - inany_ntop(fwd_rule_addr(rule), addr, sizeof(addr)); - if (rule->flags & FWD_WEAK) - weak = " (best effort)"; - if (rule->flags & FWD_SCAN) - scan = " (auto-scan)"; - - if (rule->first == rule->last) { - info(" %s [%s]%s%s:%hu => %hu %s%s", - ipproto_name(rule->proto), addr, percent, - rule->ifname, rule->first, rule->to, weak, scan); - } else { - info(" %s [%s]%s%s:%hu-%hu => %hu-%hu %s%s", - ipproto_name(rule->proto), addr, percent, - rule->ifname, rule->first, rule->last, - rule->to, rule->last - rule->first + rule->to, - weak, scan); - } + char rulestr[FWD_RULE_STRLEN]; + + info(" %s", fwd_rule_ntop(&fwd->rules[i].rule, + rulestr, sizeof(rulestr))); } }

diff --git a/fwd_rule.c b/fwd_rule.c new file mode 100644 index 00000000..dfbdf683 --- /dev/null +++ b/fwd_rule.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * PASTA - Pack A Subtle Tap Abstraction + * for network namespace/tap device mode + * + * PESTO - Programmable Extensible Socket Translation Orchestrator + * front-end for passt(1) and pasta(1) forwarding configuration + * + * fwd_rule.c - Helpers for working with forwarding rule specifications + * + * Copyright Red Hat + * Author: David Gibson + */ + +#include + +#include "fwd_rule.h" + +/** + * fwd_rule_addr() - Return match address for a rule + * @rule: Forwarding rule + * + * Return: matching address for rule, NULL if it matches all addresses + */ +const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule) +{ + if (rule->flags & FWD_DUAL_STACK_ANY) + return NULL; + + return &rule->addr; +} + +/** + * fwd_rule_ntop() - Format forwarding rule as a string + * @rule: Rule to format + * @dst: Buffer to store output (should have FWD_RULE_STRLEN bytes) + * @size: Size of @dst + */ +const char *fwd_rule_ntop(const struct fwd_rule *rule, char *dst, size_t size) +{ + const char *percent = *rule->ifname ? "%" : ""; + const char *weak = "", *scan = ""; + char addr[INANY_ADDRSTRLEN]; + int len; + + inany_ntop(fwd_rule_addr(rule), addr, sizeof(addr)); + if (rule->flags & FWD_WEAK) + weak = " (best effort)"; + if (rule->flags & FWD_SCAN) + scan = " (auto-scan)"; + + if (rule->first == rule->last) { + len = snprintf(dst, size, + "%s [%s]%s%s:%hu => %hu %s%s", + ipproto_name(rule->proto), addr, percent, + rule->ifname, rule->first, rule->to, weak, scan); + } else { + in_port_t tolast = rule->last - rule->first + rule->to; + len = snprintf(dst, size, + "%s [%s]%s%s:%hu-%hu => %hu-%hu %s%s", + ipproto_name(rule->proto), addr, percent, + rule->ifname, rule->first, rule->last, + rule->to, tolast, weak, scan); + } + + if (len < 0 || (size_t)len >= size) + return NULL; + + return dst; +} diff --git a/fwd_rule.h b/fwd_rule.h index 84ec5cbe..59db0e95 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -42,4 +42,15 @@ struct fwd_rule { uint8_t flags; };

+const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule); + +#define FWD_RULE_STRLEN \ + (IPPROTO_STRLEN - 1 \ + + INANY_ADDRSTRLEN - 1 \ + + IFNAMSIZ - 1 \ + + sizeof(" (best effort)") - 1 \ + + sizeof(" (auto-scan)") - 1 \ + + 15)

I'm not quite sure about the reason for the 15 here, is there some constant we can use perhaps?

It's the spacing and punctuation around the other displayed parameters, roughly the number of non-format characters in "%s [%s]%s%s:%hu-%hu => %hu-%hu %s%s" (plus one for the \0). I don't love it either, but I wasn't sure how to make it less obscure. Is sizeof(" []%:- => - ") an improvement? I'm not sure.

I think it definitely is. I can't see a better explanation than that sizeof(), and it also looks like a good way to double check the number of additional characters. If we want to do overdo with robustness, we could perhaps think of separately defining the format string and using it here for the calculation as well, which makes future (albeit unlikely) updates of the format string safer. But, in that case, dropping characters for each format specifier is something we would have to do manually anyway... or not do at all and rely on the size of the format string as it is as an upper bound, perhaps. -- Stefano