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=JWwPoVqV; 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 CE8A85A0265 for ; Wed, 06 May 2026 14:52:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1778071930; 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=oBsfCaLCOPT2zXFhFkpDFEfE0xMXunrA/P/N1uAlTeI=; b=JWwPoVqVfURUbBL89udNETm5SmOOjwMcBIKA7Oky6zyzzPjaFV6nTygKMo7pDVyE45Eo/2 eHeW/JhMJA5YwEPL2kmmkaaM72bscpni6liYOyc+WDAi5oZdADUYj3+vUskZSME/GAZxQs WMTC+jtvaX9ecoZ64bXhQIOpHPzhQX0= 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-52-f2ouQ_FfPDeS-Pv6TFeH7g-1; Wed, 06 May 2026 08:52:08 -0400 X-MC-Unique: f2ouQ_FfPDeS-Pv6TFeH7g-1 X-Mimecast-MFC-AGG-ID: f2ouQ_FfPDeS-Pv6TFeH7g_1778071927 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-48e526e11ccso6156995e9.3 for ; Wed, 06 May 2026 05:52:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778071927; x=1778676727; 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=oBsfCaLCOPT2zXFhFkpDFEfE0xMXunrA/P/N1uAlTeI=; b=Rz9prnWnBee7kXbpBBpk32dNWrUmBAFD420mf3MzXxs72CIiNnuOa562WD1fDdr75j QMwqkTfAdWUOfphm1cKH8trtC+MX0oPXvZkOgSPdPLHpW2yiaae9qBOwVcD27xdvv8hW 6ZLuPuH6pYEwRNCcaLzEvorIgTHPJX02Vp03YI+jhKKyuw5vcKpIQv8j2/FY+gO0zDSf KlK90K2V6dOBQ3xMb47AfDp5tMSPlKYiB7syL0a81CRbi7nr/fpXcLAkMcyPqqY5IZJV sgtRjIbzLk7yvbIGtNdLoBnyExmJ7v4agkmBFJNUQcXoDzCQJt+DIEsG3cmwVNWYBNzI NLYA== X-Gm-Message-State: AOJu0Yx7sTFND/hMM1P/IlUAEp4d4UlAm6GmZY+wUEhb/VcMmZW4Qj/d Ke+Bo3S8rgIc23aZncdB0xvFpUbwioF0UGSZoVaWO9qEFS1yX2wHfDhAfF7F5ojc8juR3E/gv+6 FL1yJtVVJTZeKUmgP6jzjoMTIot5uAwF66Bm4/YSx53gAyVR/5nTmSg== X-Gm-Gg: AeBDietxlmdQCL21KS/1ts4sBei5t0x6574kLscm6CVzXZcRcGt51P1P8mLsxBWa82e QN9nANn8/WVHgxeSh0Y+MnBczkxkhBr1kluHeFia/kvuL+T3X6UcMw0hOF8HMh8RuGYyQbQbFzb s2dk/yfrTEhdpy6OoE57Grs5iBkr963Nrc6a98kRdC1wxT7oA4jaoKfRkOtgDjTpDrXg0nXGADi IWngrvDGDXSIIdW0ikxZYnRz1HxrCe6fMVPutRKl7ioNVr6abvdlHXFp4ertYaccVqwOllMcbnW o3J8TpPfOUY8JLFtcMx2VI40/vTyD+TcROlwhvqjxQDPowlooZ9Ysfnu6L5YUmHTbe90pz+Syvw ZkQwFvhPo2KC+42Krv+SK8lyNl6Kf+Qqog5HIF+qByD/uz1ERm87T0l1MMcBr X-Received: by 2002:a05:600c:c11c:b0:48a:52f2:a0f1 with SMTP id 5b1f17b1804b1-48e51f46d48mr39666525e9.18.1778071927116; Wed, 06 May 2026 05:52:07 -0700 (PDT) X-Received: by 2002:a05:600c:c11c:b0:48a:52f2:a0f1 with SMTP id 5b1f17b1804b1-48e51f46d48mr39666085e9.18.1778071926589; Wed, 06 May 2026 05:52:06 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-48e538a5486sm48777815e9.6.2026.05.06.05.52.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 May 2026 05:52:06 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v8 19/19] pesto, conf, fwd_rule: Add options and modes to add, delete, clear rules Message-ID: <20260506145203.0ec41778@elisabeth> In-Reply-To: <20260506105601.5bfa57c7@elisabeth> References: <20260505234719.1437340-1-sbrivio@redhat.com> <20260505234719.1437340-20-sbrivio@redhat.com> <20260506102220.66a5c7a5@elisabeth> <20260506105601.5bfa57c7@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, 06 May 2026 14:52:04 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 5gvXtw7ExDc0jzZyC3thVpja-AWS3u3wDOg0NDRnuQw_1778071927 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: YJGB5RB2HBYXVKO63PZQFNR53IZHGNUD X-Message-ID-Hash: YJGB5RB2HBYXVKO63PZQFNR53IZHGNUD 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, Jon Maloy , Laurent Vivier 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, 6 May 2026 10:56:01 +0200 Stefano Brivio wrote: > On Wed, 6 May 2026 18:48:10 +1000 > David Gibson wrote: > > > On Wed, May 06, 2026 at 10:22:20AM +0200, Stefano Brivio wrote: > > > On Wed, 6 May 2026 16:45:27 +1000 > > > David Gibson wrote: > > > > > > > On Wed, May 06, 2026 at 01:47:19AM +0200, Stefano Brivio wrote: > > > > > Instead of just being able to replace the existing forwarding table, > > > > > > > > As of my last version, we already added, rather than replacing. > > > > > > Right, I noticed that, but this isn't the default behaviour we > > > discussed, so I assumed it was accidental, and planned to go back and > > > check the reason why. > > > > > > Given that it wasn't accidental, I'll simply adjust this part of the > > > commit message. > > > > > > > > implement --add and --delete options to maintain the table and add > > > > > or delete specific ports. > > > > > > > > > > The option --clear PIF forces the clearing of a table, instead. > > > > > > > > > > These options can be combined arbitrarily and are handled as > > > > > sequential commands, as now described in pesto(1). > > > > > > > > > > If no option is given before forwarding specifiers for a matching > > > > > table, the command line is interpreted as a replacement of the > > > > > existing rules. > > > > > > > > > > To this end: > > > > > > > > > > - there's no protocol change, as pesto is anyway sending updated > > > > > copies of the table > > > > > > > > > > - the forwarding table functions now include a new fwd_rule_del(), > > > > > which deletes existing rule only if a matching one is found > > > > > > > > > > - a trivial fwd_rule_clear() is factored out from the existing > > > > > conf_handler() implementation, so that it can be directly used > > > > > in pesto > > > > > > > > > > The entry points for parsing of port specifiers now take an additional > > > > > 'del' parameter which is passed down all the way before reaching the > > > > > fwd_rule_add() implementation. If a rule should be deleted, at that > > > > > point, fwd_rule_del() is called instead. > > > > > > > > > > Signed-off-by: Stefano Brivio > > > > > --- > > > > > conf.c | 26 ++++++---------- > > > > > fwd_rule.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++------- > > > > > fwd_rule.h | 4 ++- > > > > > pesto.1 | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > pesto.c | 53 ++++++++++++++++++++++++++++--- > > > > > 5 files changed, 227 insertions(+), 32 deletions(-) > > > > > > > > > > diff --git a/conf.c b/conf.c > > > > > index 3f48793..909c34c 100644 > > > > > --- a/conf.c > > > > > +++ b/conf.c > > > > > @@ -1849,16 +1849,16 @@ void conf(struct ctx *c, int argc, char **argv) > > > > > > > > > > if (name == 't') { > > > > > opt_t = true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]); > > > > > } else if (name == 'u') { > > > > > opt_u = true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]); > > > > > } else if (name == 'T') { > > > > > opt_T = true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]); > > > > > } else if (name == 'U') { > > > > > opt_U = true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]); > > > > > } > > > > > } while (name != -1); > > > > > > > > > > @@ -1910,13 +1910,13 @@ void conf(struct ctx *c, int argc, char **argv) > > > > > > > > > > if (c->mode == MODE_PASTA) { > > > > > if (!opt_t) > > > > > - fwd_rule_parse('t', "auto", c->fwd[PIF_HOST]); > > > > > + fwd_rule_parse('t', false, "auto", c->fwd[PIF_HOST]); > > > > > if (!opt_T) > > > > > - fwd_rule_parse('T', "auto", c->fwd[PIF_SPLICE]); > > > > > + fwd_rule_parse('T', false, "auto", c->fwd[PIF_SPLICE]); > > > > > if (!opt_u) > > > > > - fwd_rule_parse('u', "auto", c->fwd[PIF_HOST]); > > > > > + fwd_rule_parse('u', false, "auto", c->fwd[PIF_HOST]); > > > > > if (!opt_U) > > > > > - fwd_rule_parse('U', "auto", c->fwd[PIF_SPLICE]); > > > > > + fwd_rule_parse('U', false, "auto", c->fwd[PIF_SPLICE]); > > > > > } > > > > > > > > > > conf_sock_listen(c); > > > > > @@ -2135,14 +2135,8 @@ void conf_handler(struct ctx *c, uint32_t events) > > > > > unsigned pif; > > > > > > > > > > /* Clear pending tables */ > > > > > - for (pif = 0; pif < PIF_NUM_TYPES; pif++) { > > > > > - struct fwd_table *fwd = c->fwd_pending[pif]; > > > > > - > > > > > - if (!fwd) > > > > > - continue; > > > > > - fwd->count = 0; > > > > > - fwd->sock_count = 0; > > > > > - } > > > > > + for (pif = 0; pif < PIF_NUM_TYPES; pif++) > > > > > + fwd_rule_clear(c->fwd_pending[pif]); > > > > > > > > > > /* FIXME: this could block indefinitely if the client doesn't > > > > > * write as much as it should > > > > > diff --git a/fwd_rule.c b/fwd_rule.c > > > > > index 03e8e80..eb9a601 100644 > > > > > --- a/fwd_rule.c > > > > > +++ b/fwd_rule.c > > > > > @@ -180,6 +180,66 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule * > > > > > return true; > > > > > } > > > > > > > > > > +/** > > > > > + * fwd_rule_clear() - Clear a forwarding table > > > > > + * @fwd: Table to clear (might be NULL) > > > > > + */ > > > > > +void fwd_rule_clear(struct fwd_table *fwd) > > > > > +{ > > > > > + if (!fwd) > > > > > + return; > > > > > + > > > > > > > > Not essential, but I wonder if it would be wise to verify that there > > > > are no currently open sockets associated with any of the rules. > > > > > > With a loop, I suppose. I can add it as a TODO comment because I guess > > > it would be good to handle that case (open sockets left) for > > > fwd_rule_del() as well, and a part of the implementation can probably > > > be common. > > > > > > > > + fwd->count = 0; > > > > > + fwd->sock_count = 0; > > > > > +} > > > > > + > > > > > +/** > > > > > + * fwd_rule_del() - Partially validate and delete a rule from a forwarding table > > > > > + * @fwd: Table to delete from > > > > > + * @rule: Rule to delete (must match an existing rule) > > > > > + * > > > > > + * Return: 0 on success, negative error code on failure (-ENOENT if not found) > > > > > + * > > > > > + * NOTE: This function can't be used for a forwarding table with valid sockets > > > > > + * stored in fwd->rulesocks. > > > > > > > > The exact meaning of this isn't very clear to me. Does "valid" mean > > > > "open" or something else? > > > > > > It means valid at some point, not necessarily open right now. I'll > > > change it to "open" for clarity. > > > > I'm not sure what "valid at some point" means, either. > > That it was a valid socket file descriptor (an open one) at some point. > > > > > I think what you're getting at is that every entry in fwd->socks[] > > > > must be -1. Or at least every entry with index in [0,sock_count) > > > > > > Yes. > > > > > > > > + */ > > > > > +static int fwd_rule_del(struct fwd_table *fwd, const struct fwd_rule *rule) > > > > > +{ > > > > > + unsigned num, i; > > > > > + > > > > > + for (i = 0; i < fwd->count; i++) { > > > > > + if (fwd_rule_conflicts(rule, &fwd->rules[i])) > > > > > + break; > > > > > + } > > > > > > > > So, this deletes any conflicting rule, not only exact matches. That's > > > > not very clear from the description of @rule. > > > > > > It deletes the first one > > > > Oh, good point. Which actually elevates this to a bug, not just a > > debate about the best semantics, because... > > > > > (but given that fwd_rule_conflicts() is called > > > on insertion, there should be a single one). > > > > ... that's not correct. "conflicts" is not transitive, so (for > > example) in the cases below: > > -t 1000-2000 -t 4000-5000 --delete -t 500-5500 > > -t 127.0.0.1/100 -t 127.0.0.2/100 --delete -t 100 > > The deleted rule conflicts with both the added rules, but they don't > > conflict with each other. > > Right, yes, for partially overlapping rules that's true. But that's not > what Podman needs right now, so I think it can be fixed later. > > > I don't think "delete all conflicting rules" is a great either, since > > it means that: > > -t 1000-1999 -t 2000-2999 --delete -t 1500-2500 > > maps nothing at all, which seems pretty surprising. > > > > > It's good enough for our purposes right now, even though we might want > > > to make that more sophisticated in the future. I'll change the > > > description of @rule. > > > > I really think the current behaviour is too confusing. Only deleting > > exact matches (and giving an error if there's a conflict that's not an > > exact match) I think *is* good enough for now, so that's what I'd > > suggest. > > ...except that it's not implemented by any function and it's not exactly > trivial either, and delaying the implementation further makes this > useless (at least for Podman, which we can approximate to "essentially > useless"), so I'd rather go with something that doesn't take care of > partially overlapping ranges, rather than no feature at all. ...but I ended up implementing exactly that in v10 as a re-spin was needed anyway and I realised that the implementation is absolutely trivial. -- Stefano