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=QYXRFGGH; 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 282405A0265 for ; Wed, 01 Jul 2026 02:08:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1782864488; 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=bvcjMPpn+dtTxe5G56BVuChuDSJ4eEbL28FzAwKlzds=; b=QYXRFGGHlr7UhMvS1CqHd9oqYZuLgz2TncBHYV8r9OMe65/sFrHzkoCCJUj+t0HM5URsG1 OeGfEuYC+15pPE7KPmEMdE9zRqYMV2fkdMQgwWMb7jYZ8+Kp0/LcZ3/KLORezZqKT0WbX0 9y4UL0N1y8AyoVDs49lNgnlzAikcBg8= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-70-DqDefUxeM_STtgmyN9KKhA-1; Tue, 30 Jun 2026 20:08:05 -0400 X-MC-Unique: DqDefUxeM_STtgmyN9KKhA-1 X-Mimecast-MFC-AGG-ID: DqDefUxeM_STtgmyN9KKhA_1782864484 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-4926e6e3d78so258065e9.3 for ; Tue, 30 Jun 2026 17:08:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782864484; x=1783469284; 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=bvcjMPpn+dtTxe5G56BVuChuDSJ4eEbL28FzAwKlzds=; b=l0/LlTk01O+LAGZYqm2TpfNAQgort5DlzR5dLdX8ZsC0vJe2HnAkpB8xfcNEFNqJAC odPHY53S0n2nVPaVqknjcTr1afIFi8bkaLcgsbxbvjN7WyEE/nG+YtNuEXIwmzHtutHz lOOrZENO5UZ4Xlte5Zrd+NKFd2/AUEnqknMagKnskohjSSL3EHeuUXLlzgcX8d44oW0o Te6EuRaM7wFFfKUAp1mXytBVfsOYcNXhYuB68Bh53INzTZkNX6wzn7UTP3Uxlfat6cSW tRHxqzmwEPGiAYVYX+za1kpN3/Lhc+hIcjmDoiq3e1Mj9k2uhNePBICqGSud0GW/k/Gp TadQ== X-Gm-Message-State: AOJu0Ywm7NNnOUffiQF3RJQkNoGGNa4ZDeF3USW9lqL7LilCSV+3ysUh HHmzfd9JyQeCU5JfQKJdUS5OyN2HxfndrZvNHtF3ZgpXzz7miQeRjLy8OcV64LdHrGGNfnCr/Fm WiWZLzanlf4H8ZVnYPhRwi7LALPYJpDJ6CN897QNE1pTxTrt8Fa/ur1K/O2rxGw== X-Gm-Gg: AfdE7cnHQRI9Qd27N1JzVcxwO3TBvd5ylLLT5eEBUP9npEncfEl2baFD8CCzgGtl/Ut 6bqWZhFrwlzkxqqv6sGk1IgNnKAHtfksZNdoUJRa5lVVqvnF/mdKX5z7BoUQOYLUdz+6OgArdul UHzpxmc5k2p67AzpFfszOnSNXccyp+vC2zEw6VdC8FJ2WhW/5SV0hHwwLgRQcFOl78MUYwF57gT kqyUVZjlzCskcsy76Qzmd+f10L0mI4PcMs/N3HlIh9NYjhtbvNP9W4XpKCa6BgSUOanU7LXeJkW +iUda5SD2BLoyKrtb0drsAR6Sym4a85rMJAtOfTWhNrQzy6Aqick8Z84hklLyib8p5xFGLYP1vU rjFzz0BmxrRLPt+z/MM7qYg== X-Received: by 2002:a05:600c:4687:b0:493:be0e:377c with SMTP id 5b1f17b1804b1-493be0e37ccmr28122255e9.24.1782864484164; Tue, 30 Jun 2026 17:08:04 -0700 (PDT) X-Received: by 2002:a05:600c:4687:b0:493:be0e:377c with SMTP id 5b1f17b1804b1-493be0e37ccmr28121735e9.24.1782864483607; Tue, 30 Jun 2026 17:08:03 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-493bef23feasm16558545e9.2.2026.06.30.17.08.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jun 2026 17:08:02 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 12/12] fwd_rule: Rewrite forward rule parsing using parse.c helpers Message-ID: <20260701020801.4d26f7e8@elisabeth> In-Reply-To: <20260626071003.3472194-13-david@gibson.dropbear.id.au> References: <20260626071003.3472194-1-david@gibson.dropbear.id.au> <20260626071003.3472194-13-david@gibson.dropbear.id.au> 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, 01 Jul 2026 02:08:02 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: qMNJXey00jtll0iveXW0xcx5MffqlVfAXzc44-tOu1k_1782864484 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: VXKCIYH5HNC2CL5OVNIYWEQEW6UAJAZ7 X-Message-ID-Hash: VXKCIYH5HNC2CL5OVNIYWEQEW6UAJAZ7 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 Fri, 26 Jun 2026 17:10:03 +1000 David Gibson wrote: > Forwarding rules for the -[tTuU] options are parsed in fwd_rule_parse() > and fwd_rule_parse_ports(). Currently those use a mix of loosely > recursive descent parsing helpers, consuming input from left to right, > and ad-hoc C parsing - searching for delimeters with strchr() or the like Nit if you respin: delimiters. > and breaking down on that basis. > > As well as being a slightly awkward mix, the current approach will not work > for adding target addresses to the rules: we can't easily split the > listening from target parts first, because the ':' delimiter could appear > multiple times for multiple port ranges, or in IPv6 addresses. However, > without splitting the target part first, we can't first split off the > listening address and interface because the '/' delimiter could appear in > the target portion, but not the listening portion, e.g.: > -t 12345:192.0.1.1/12345 > > To address this, rewrite the entirety of the parsing so we consume the > input left to right in a more-or-less recursive descent manner. This means > we no longer rely on certain delimiters having a single meaning over the > whole input, just the next part of it. > > Because of the semantics of port specs, we need to make several passes > over the list of comma separated chunks. Previously, some of the logic was > duplicated between the two passes, making it fragile. Now, we introduce > a parse_port_chunk() helper which we re-use in each pass to make it clearer > we parse exactly the same way on each pass. > > Signed-off-by: David Gibson > --- > fwd_rule.c | 262 ++++++++++++++++++++++++++++++----------------------- > parse.c | 29 ++++++ > parse.h | 2 + > 3 files changed, 181 insertions(+), 112 deletions(-) > > diff --git a/fwd_rule.c b/fwd_rule.c > index b14df340..8ae21b99 100644 > --- a/fwd_rule.c > +++ b/fwd_rule.c > @@ -439,17 +439,96 @@ fail: > fwd_rule_fmt(&rule, rulestr, sizeof(rulestr))); > } > > -/* > - * for_each_chunk - Step through delimited chunks of a string > - * @p_: Pointer to start of each chunk (updated) > - * @ep_: Pointer to end of each chunk (updated) > - * @s_: String to step through > - * @sep_: String of all allowed delimiters > +/** > + * enum fwd_port_chunk_kind - Kind of port specifier piece Nit: I think we should use kerneldoc-style comments also for enum (see enum udp_iov_idx, udp.c for an example). > + */ > +enum fwd_port_chunk_kind { > + CHUNK_ALL, /* "all" */ > + CHUNK_AUTO, /* "auto" */ > + CHUNK_EXCLUDE, /* "~1111-2222" */ > + CHUNK_INCLUDE, /* "1111-2222" */ > +}; > + > +/** > + * parse_port_chunk() - Parse one chunk of a port specifier > + * @cursor: Parsing point (see parse.c) > + * @kindp: Updated with kind of chunk we parsed > + * @lrange: Updated with listening port range (for INCLUDE & EXCLUDE) > + * @trange: Updated with target port range (for EXCLUDE) > + */ > +static bool parse_port_chunk(const char **cursor, enum fwd_port_chunk_kind *kindp, > + struct port_range *lrange, struct port_range *trange) Nit: this exceeds 80 columns. I'm wondering whether this doesn't rather belong in parse.c: having it here makes it hard to spot what it returns (the "Theory of Operation" comment from parse.c), and anyway it's pure parsing, nothing else. > +{ > + struct port_range lr = { 0 }, tr = { 0 }; > + enum fwd_port_chunk_kind kind; > + const char *p = *cursor; > + > + if (parse_literal(&p, "all")) { > + kind = CHUNK_ALL; > + } else if (parse_literal(&p, "auto")) { > + kind = CHUNK_AUTO; > + } else if (parse_literal(&p, "~")) { > + kind = CHUNK_EXCLUDE; > + if (!parse_port_range(&p, &lr)) > + return false; > + } else if (parse_port_range(&p, &lr)) { > + kind = CHUNK_INCLUDE; > + > + if (parse_literal(&p, ":")) { > + if (!parse_port_range(&p, &tr)) > + return false; > + } else { > + tr = lr; > + } > + } else { > + return false; > + } > + > + *kindp = kind; > + *lrange = lr; > + if (trange) > + *trange = tr; > + *cursor = p; I wonder if it's worth it being rigid about avoiding side effects on failure, in these functions. In general I agree it's desirable, but in practice we don't need it here and it's quite a few extra lines with slightly confusing variable names. > + return true; > +} > + > +/** > + * parse_laddrif() - Parse listening address+ifname This makes it look like + is a separator, maybe "address%ifname"? The name isn't really descriptive I think. Maybe we could take for granted that it's about the listening part, and call it parse_addrifname()? Or parse_addr_ifname()? > + * @cursor: Parsing cursor (see parse.c) > + * @addr: Updated with parsed inany address (NULL for *) > + * @abuf: Buffer to store address > + * @ifname: Updated with parsed interface name ("" if none) > */ > -#define for_each_chunk(p_, ep_, s_, sep_) \ > - for ((p_) = (s_); \ > - (ep_) = (p_) + strcspn((p_), (sep_)), *(p_); \ > - (p_) = *(ep_) ? (ep_) + 1 : (ep_)) > +static bool parse_laddrif(const char **cursor, > + const union inany_addr **addr, > + union inany_addr *abuf, > + char *ifname) > +{ > + union inany_addr atmp = inany_any6; > + char iftmp[IFNAMSIZ] = {0}; > + const char *p; > + > + if (p = *cursor, The advantage of using the comma operator isn't really clear to me here. Can't we just do this assignment unconditionally as initialisation? By the way, while we don't adhere to it, I think MISRA C contains many guidelines that are actually useful (maybe more than half of them?), and MISRA C:2012 forbids this kind of usage because of readability issues: https://www.mathworks.com/help/bugfinder/ref/misrac2012rule12.3.html > + parse_inany(&p, &atmp) && > + parse_ifspec(&p, iftmp) && > + parse_literal(&p, "/")) { > + /* Specific listening address */ > + *addr = abuf; > + } else if (p = *cursor, > + parse_literal(&p, "*"), > + parse_ifspec(&p, iftmp) && > + parse_literal(&p, "/")) { > + /* Missing or "*" address */ > + *addr = NULL; > + } else { > + return false; > + } > + > + *abuf = atmp; > + memcpy(ifname, iftmp, IFNAMSIZ); > + *cursor = p; > + return true; > +} > > /** > * fwd_rule_parse_ports() - Parse port range(s) specifier > @@ -466,87 +545,70 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, > const char *spec) > { > uint8_t exclude[PORT_BITMAP_SIZE] = { 0 }; > + enum fwd_port_chunk_kind kind; > + struct port_range lrange; > bool exclude_only = true; > - const char *p, *ep; > uint8_t flags = 0; > + const char *p; > unsigned i; > > - /* Parse excluded ranges and "auto" in the first pass */ > - for_each_chunk(p, ep, spec, ",") { > - struct port_range xrange; > - > - /* Include range, parse later */ > - if (parse_literal(&p, "all") || isdigit(*p)) > - continue; > - > - if (parse_literal(&p, "auto")) { > - if (p != ep) /* Garbage after the keyword */ > - goto bad; > + /* Consider excluded ranges and "auto" in the first pass */ > + p = spec; > + do { > + if (!parse_port_chunk(&p, &kind, &lrange, NULL)) > + goto bad; > > + switch (kind) { > + case CHUNK_AUTO: > if (!(fwd->caps & FWD_CAP_SCAN)) { > die( > "'auto' port forwarding is only allowed for pasta"); > } > - > flags |= FWD_SCAN; > - continue; > - } > - > - /* Should be an exclude range */ > - if (!parse_literal(&p, "~")) > - goto bad; > - > - if (!parse_port_range(&p, &xrange)) > - goto bad; > - if (p != ep) /* Garbage after the range */ > - goto bad; > - > - for (i = xrange.first; i <= xrange.last; i++) > - bitmap_set(exclude, i); > - } > - > - /* Now process base ranges, skipping exclusions */ > - for_each_chunk(p, ep, spec, ",") { > - struct port_range orig_range, mapped_range; > - > - /* Handle "all" like exclude only */ > - if (parse_literal(&p, "all")) { > - if (p != ep) /* Garbage after the keyword */ > - goto bad; > + break; > > - continue; > + case CHUNK_EXCLUDE: > + for (i = lrange.first; i <= lrange.last; i++) > + bitmap_set(exclude, i); > + break; > + default: > + ; /* Handled later */ > } > + } while (parse_literal(&p, ",")); > > - if (!isdigit(*p)) > - /* Already parsed */ > - continue; > + /* Consider included ranges in next pass */ > + p = spec; > + do { > + struct port_range trange; > > - if (!parse_port_range(&p, &orig_range)) > + if (!parse_port_chunk(&p, &kind, &lrange, &trange)) > goto bad; > > - exclude_only = false; > + switch (kind) { > + case CHUNK_AUTO: /* already handled */ > + case CHUNK_EXCLUDE: /* already handled */ > + case CHUNK_ALL: /* handled later */ > + continue; > > - if (parse_literal(&p, ":")) { > - /* There's a range to map to as well */ > - if (!parse_port_range(&p, &mapped_range)) > - goto bad; > - if ((mapped_range.last - mapped_range.first) != > - (orig_range.last - orig_range.first)) > + case CHUNK_INCLUDE: > + exclude_only = false; > + if (trange.last - trange.first != > + lrange.last - lrange.first) > goto bad; > - } else { > - mapped_range = orig_range; > - } > > - if (p != ep) /* Garbage after the ranges */ > + fwd_rule_range_except(fwd, del, proto, addr, ifname, > + lrange.first, lrange.last, > + exclude, trange.first, flags); > + break; > + default: > goto bad; > + } > + } while (parse_literal(&p, ",")); > > - fwd_rule_range_except(fwd, del, proto, addr, ifname, > - orig_range.first, orig_range.last, > - exclude, > - mapped_range.first, flags); > - } > + if (!parse_eoi(p)) > + goto bad; /* trailing garbage */ > > - /* Finally handle "all" and exclude only specs */ > + /* Finally handle "all" and exclude only cases */ > if (exclude_only) { > fwd_port_map_ephemeral(exclude); > > @@ -569,10 +631,11 @@ bad: > void fwd_rule_parse(char optname, bool del, const char *optarg, > struct fwd_table *fwd) > { > - char buf[BUFSIZ], *spec, *ifname = NULL; > - union inany_addr addr_buf = inany_any6; > - const union inany_addr *addr = &addr_buf; > + const union inany_addr *addr; > + union inany_addr addr_buf; > + char ifname[IFNAMSIZ]; > uint8_t proto; > + const char *p; > > if (optname == 't' || optname == 'T') > proto = IPPROTO_TCP; > @@ -581,7 +644,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, > else > assert(0); > > - if (!strcmp(optarg, "none")) { > + if (p = optarg, Same as above: do we really need this? It's local anyway. > + parse_literal(&p, "none") && parse_eoi(p)) { > unsigned i; > > for (i = 0; i < fwd->count; i++) { > @@ -593,45 +657,19 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, > return; > } > > - strncpy(buf, optarg, sizeof(buf) - 1); > - > - if ((spec = strchr(buf, '/'))) { > - const char *p = buf; > - > - *spec = 0; > - spec++; > - > - if (optname != 't' && optname != 'u') > + if (p = optarg, > + parse_laddrif(&p, &addr, &addr_buf, ifname)) { > + if (optname == 'T' || optname == 'U') > die("Listening address not allowed for -%c %s", > optname, optarg); > - > - if ((ifname = strchr(buf, '%'))) { > - *ifname = 0; > - ifname++; > - > - /* spec is already advanced one past the '/', > - * so the length of the given ifname is: > - * (spec - ifname - 1) > - */ > - if (spec - ifname - 1 >= IFNAMSIZ) { > - die("Interface name '%s' is too long (max %u)", > - ifname, IFNAMSIZ - 1); > - } > - } > - > - if (ifname == buf + 1 || /* Interface without address */ > - !strcmp(buf, "*")) /* Explicit wildcard address */ > - addr = NULL; > - else if (!parse_inany(&p, &addr_buf) && parse_eoi(p)) > - die("Bad forwarding address '%s'", buf); > } else { > - spec = buf; > - > + /* No address or ifname */ > addr = NULL; > + ifname[0] = '\0'; Actually, here, having side effects from parse_laddrif() failing would be rather convenient and intuitive. > } > > if (optname == 'T' || optname == 'U') { > - assert(!addr && !ifname); > + assert(!addr && !*ifname); > > if (!(fwd->caps & FWD_CAP_IFNAME)) { > warn( > @@ -640,18 +678,18 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, > > if (fwd->caps & FWD_CAP_IPV4) { > fwd_rule_parse_ports(fwd, del, proto, > - &inany_loopback4, NULL, > - spec); > + &inany_loopback4, NULL, p); > } > if (fwd->caps & FWD_CAP_IPV6) { > fwd_rule_parse_ports(fwd, del, proto, > - &inany_loopback6, NULL, > - spec); > + &inany_loopback6, NULL, p); > } > return; > } > > - ifname = "lo"; > + static_assert(sizeof("lo") <= sizeof(ifname), > + "ifname buffer too small"); > + strcpy(ifname, "lo"); > } > > /* No need for dual stack if we only have one IP version */ > @@ -660,13 +698,13 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, > else if (!addr && !(fwd->caps & FWD_CAP_IPV6)) > addr = &inany_any4; > > - if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) { > + if (*ifname && !(fwd->caps & FWD_CAP_IFNAME)) { > die( > "Device binding for '-%c %s' unsupported (requires kernel 5.7+)", > optname, optarg); > } > > - fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec); > + fwd_rule_parse_ports(fwd, del, proto, addr, *ifname ? ifname : NULL, p); > } > > /** > diff --git a/parse.c b/parse.c > index 3e0dbd45..d83ea9f9 100644 > --- a/parse.c > +++ b/parse.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > > #include "common.h" > #include "parse.h" > @@ -212,3 +213,31 @@ bool parse_inany_(const char **cursor, union inany_addr *addr, > > return false; > } > + > +/** > + * parse_ifspec() - Parse a interface name specifier (starting with %) > + * @ifname: On success updated with parsed name (must have IFNAMSIZ space) > + * > + * This will accept a missing specifier (empty string), setting ifname to "" > + */ > +bool parse_ifspec(const char **cursor, char *ifname) > +{ > + const char *p = *cursor; > + size_t len; > + > + if (!parse_literal(&p, "%")) { > + /* No interface specifier */ > + ifname[0] = '\0'; > + return true; > + } > + > + /* ifnames can have anything that's not '/' or whitespace */ dev_valid_name(), net/core/dev.c in the Linux kernel, also forbids '.' and '..', to avoid breaking sysfs. We don't really _need_ to check for that as we don't create interfaces, but strictly speaking the comment isn't accurate, and we might want to filter for consistency anyway. > + len = strcspn(p, "/ \f\n\r\t\v"); > + if (!len || len >= IFNAMSIZ) > + return false; > + > + memcpy(ifname, p, len); > + ifname[len] = '\0'; > + *cursor = p + len; > + return true; > +} > diff --git a/parse.h b/parse.h > index 08b038cf..1079f5bf 100644 > --- a/parse.h > +++ b/parse.h > @@ -32,4 +32,6 @@ bool parse_inany_(const char **cursor, union inany_addr *addr, > > #define parse_inany(cursor, addr) parse_inany_((cursor), (addr), NULL) > > +bool parse_ifspec(const char **cursor, char *ifname); > + > #endif /* _PARSE_H */ Other than those comments, it all looks good to me, and also like an obvious, much needed improvement. I tested it quite a bit and I didn't manage to break things. -- Stefano