From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=gJ6Ub8vn; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 87DDE5A061A for ; Wed, 25 Mar 2026 09:18:13 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1774426692; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WeJwgFpQWUdl9TkgXJZCK+m5ruJlk1EwWuxd41/8OP0=; b=gJ6Ub8vnq7ZDz0HgsymJjfvWa4PPwmoINCKKIIXdlAISqvhbRPjF1KMwRyEsnxOtfOs1qs vVbVNLr9Fgir9Fbn6JFpXcPkM40haOpIqLNduVl4CYpBD6v7RSh0Niuqmf/dW70d9tFeu4 eMGbYroCElhPtqOJ6LE0/AW+btKnhoA= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-484-q0cSEa9VOXKjc2szfzGsSw-1; Wed, 25 Mar 2026 04:18:11 -0400 X-MC-Unique: q0cSEa9VOXKjc2szfzGsSw-1 X-Mimecast-MFC-AGG-ID: q0cSEa9VOXKjc2szfzGsSw_1774426690 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-4836abfc742so35162825e9.0 for ; Wed, 25 Mar 2026 01:18:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774426690; x=1775031490; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WeJwgFpQWUdl9TkgXJZCK+m5ruJlk1EwWuxd41/8OP0=; b=VmHc/UZt+qGqa9BJOW7TuHaNOKjMerdynmbF/OKZrUa1/I8mFbxr7V2TPoqa5qrMjh 857K/85Ei/b1POM8b//fmS5BtfOUAhTjbZQlZIEbPoDp4+8G/Te5vXqkI1HIU2uEyKO/ U5bhPw1tbsjabjF3vTFQ/o9WmNr6Zo4yKWzENwgP0ED9FvprlqBvELMVeXWOZQDx8Ggh PPgLuVg+q/pBMQLXdYvNuwMZaW7tQAtRPOg5MNyUGYAhsUZsNOZwFZhF8Gly9c2iQbBf W3MipRnhMURUCk9YvZuNhYrJ7JdZfStdJrYb7o6IsYtMSQ5iao2wCr6LKciLzQk9MPlC 2xAA== X-Gm-Message-State: AOJu0YzDA6XyNTxNaMceO6Dq/9Ct9E3554lw+2Jge+XKWmiBpa8Fho3m PoO4lRPzlUMg3hbPdYM3ZmEHFMumFXJ0pecJAR2keEih+2gdO/m+u0bRhNBrkxxhNLU8UxyKLTf 1F4XaaftC7ZCHz0hdjz0OeaHpirGcllylaMEXnKLJaq1+pNXtbliP7w== X-Gm-Gg: ATEYQzz+c0CH//ju2ysH+O7ISiVlegbrKn7jN7PJNviXv8tQoUl+30Hha0G3sPJOOsI IDSxc/CH+ha5PhYhOlyzbQXd8gbNkqmoQYnCww4xkRtuETAjxOlK0SSVKSGlUkPSNfTdgWQx8z9 zgpXeu/0FR+CVtygeNogxyXp+ZFyl6VUVYZWAmZzg/4w1nXcCQdHI8zzfODJel+ijfi1s2wDStq RweJBfc5IX/8FBeZWeW5RJLxc0LZ9GS1VAsJi+Gv0snkFsSr2aAW48AhYN5JIlePeOz0TzzHh38 v5VPzpJM/GD4iVHbiK06i/rrmG+y7ulUNDL40IWIxw8GGoJIUWOMhkMVaWGvDqkUD2rdn5rZTOf LvlOmwgNDgVj49BEOrAR9A1UiFf00zM5UQNPBdWJEWDQXcYwFbA== X-Received: by 2002:a05:600c:5298:b0:483:6d4a:7e6d with SMTP id 5b1f17b1804b1-4871606c6a9mr39234285e9.30.1774426689757; Wed, 25 Mar 2026 01:18:09 -0700 (PDT) X-Received: by 2002:a05:600c:5298:b0:483:6d4a:7e6d with SMTP id 5b1f17b1804b1-4871606c6a9mr39233395e9.30.1774426688846; Wed, 25 Mar 2026 01:18:08 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-487172a72b6sm22391985e9.3.2026.03.25.01.18.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Mar 2026 01:18:08 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 20/25] fwd_rule: Move forwarding rule text formatting to common code Message-ID: <20260325091806.3f389776@elisabeth> In-Reply-To: References: <20260323073732.3158468-1-david@gibson.dropbear.id.au> <20260323073732.3158468-21-david@gibson.dropbear.id.au> <20260325015633.655d7386@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 25 Mar 2026 09:18:07 +0100 (CET) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: DwgHNoeNjozOvXYj3YWc82_dQWi_wJqlORoiTmUDrz8_1774426690 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: QKIFVZ5SGGTIJ5DEFJQF2GJBGX6C2PD7 X-Message-ID-Hash: QKIFVZ5SGGTIJ5DEFJQF2GJBGX6C2PD7 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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