From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v3 20/25] fwd_rule: Move forwarding rule text formatting to common code
Date: Wed, 25 Mar 2026 09:18:07 +0100 (CET) [thread overview]
Message-ID: <20260325091806.3f389776@elisabeth> (raw)
In-Reply-To: <acNnmDxwLg34_wWh@zatzit>
On Wed, 25 Mar 2026 15:42:00 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > ---
> > > 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 <david@gibson.dropbear.id.au>
> > > + */
> > > +
> > > +#include <stdio.h>
> > > +
> > > +#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
next prev parent reply other threads:[~2026-03-25 8:18 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-23 7:37 [PATCH v3 00/25] RFC: Read-only dynamic update implementation David Gibson
2026-03-23 7:37 ` [PATCH v3 01/25] conf: runas can be const David Gibson
2026-03-23 7:37 ` [PATCH v3 02/25] vhost_user: Fix assorted minor cppcheck warnings David Gibson
2026-03-23 7:37 ` [PATCH v3 03/25] serialise: Split functions user for serialisation from util.c David Gibson
2026-03-25 0:54 ` Stefano Brivio
2026-03-25 1:50 ` David Gibson
2026-03-23 7:37 ` [PATCH v3 04/25] serialise: Add helpers for serialising unsigned integers David Gibson
2026-03-23 7:37 ` [PATCH v3 05/25] fwd: Move selecting correct scan bitmap into fwd_sync_one() David Gibson
2026-03-23 7:37 ` [PATCH v3 06/25] fwd: Look up rule index in fwd_sync_one() David Gibson
2026-03-23 7:37 ` [PATCH v3 07/25] fwd: Store forwarding tables indexed by (origin) pif David Gibson
2026-03-25 0:54 ` Stefano Brivio
2026-03-25 4:04 ` David Gibson
2026-03-23 7:37 ` [PATCH v3 08/25] fwd: Allow FWD_DUAL_STACK_ANY flag to be passed directly to fwd_rule_add() David Gibson
2026-03-25 0:54 ` Stefano Brivio
2026-03-25 4:07 ` David Gibson
2026-03-23 7:37 ` [PATCH v3 09/25] fwd, conf: Expose ephemeral ports as bitmap rather than function David Gibson
2026-03-23 7:37 ` [PATCH v3 10/25] conf: Don't bother complaining about overlapping excluded ranges David Gibson
2026-03-23 7:37 ` [PATCH v3 11/25] conf: Move check for mapping port 0 to caller David Gibson
2026-03-23 7:37 ` [PATCH v3 12/25] conf: Move check for disabled interfaces earlier David Gibson
2026-03-23 7:37 ` [PATCH v3 13/25] pesto: Introduce stub configuration interface and tool David Gibson
2026-03-25 0:54 ` Stefano Brivio
2026-03-23 7:37 ` [PATCH v3 14/25] pesto: Add command line option parsing and debug messages David Gibson
2026-03-25 0:55 ` Stefano Brivio
2026-03-25 4:27 ` David Gibson
2026-03-23 7:37 ` [PATCH v3 15/25] pesto: Expose list of pifs to pesto David Gibson
2026-03-25 0:56 ` Stefano Brivio
2026-03-25 4:34 ` David Gibson
2026-03-25 8:18 ` Stefano Brivio
2026-03-25 8:31 ` David Gibson
2026-03-23 7:37 ` [PATCH v3 16/25] ip: Prepare ip.[ch] for sharing with pesto tool David Gibson
2026-03-23 7:37 ` [PATCH v3 17/25] inany: Prepare inany.[ch] " David Gibson
2026-03-23 7:37 ` [PATCH v3 18/25] fwd: Split forwading rule specification from its implementation state David Gibson
2026-03-23 7:37 ` [PATCH v3 19/25] ip: Define a bound for the string returned by ipproto_name() David Gibson
2026-03-23 7:37 ` [PATCH v3 20/25] fwd_rule: Move forwarding rule text formatting to common code David Gibson
2026-03-25 0:56 ` Stefano Brivio
2026-03-25 4:42 ` David Gibson
2026-03-25 8:18 ` Stefano Brivio [this message]
2026-03-25 23:54 ` David Gibson
2026-03-23 7:37 ` [PATCH v3 21/25] pesto: Read current ruleset from passt/pasta and display it David Gibson
2026-03-25 0:56 ` Stefano Brivio
2026-03-25 4:43 ` David Gibson
2026-03-23 7:37 ` [PATCH v3 22/25] conf: Move port parsing functions to own file, ports.c David Gibson
2026-03-23 7:37 ` [PATCH v3 23/25] conf, fwd, ports, util: Move things around for pesto David Gibson
2026-03-23 7:37 ` [PATCH v3 24/25] pesto, conf: Parse, send and receive new rules David Gibson
2026-03-23 7:37 ` [PATCH v3 25/25] conf, fwd: Allow switching to new rules received from pesto David Gibson
2026-03-23 8:38 ` [PATCH v3 00/25] RFC: Read-only dynamic update implementation David Gibson
2026-03-25 0:56 ` Stefano Brivio
2026-03-25 1:00 ` Stefano Brivio
2026-03-25 4:44 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260325091806.3f389776@elisabeth \
--to=sbrivio@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).