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=RCHEnlIe; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id B960E5A0265 for ; Wed, 06 May 2026 10:56:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1778057766; 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=PwS3b7B9P4IOvh1FxRsjBih5RmHPDXVSqQSTvHFZkK0=; b=RCHEnlIeLcjxSQcjgtlsa9UBwa15o3XGWjq9lGIU+DrZ1JH+MebLZXBpSzg2LESy+pBjiP W1w7bS33tO7V5HiMOXRMV3NvApS3VboHlPTePO7ymui/pq73K3LqnlGzloJBlsb6wyUMjQ hGXdEvWl53rjqwGZhaaU8HP7dnrmuSE= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-159-Rqn5Jim2Mz6Y_KSstsWLbA-1; Wed, 06 May 2026 04:56:05 -0400 X-MC-Unique: Rqn5Jim2Mz6Y_KSstsWLbA-1 X-Mimecast-MFC-AGG-ID: Rqn5Jim2Mz6Y_KSstsWLbA_1778057764 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-4470d6d2a4fso6941529f8f.1 for ; Wed, 06 May 2026 01:56:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778057764; x=1778662564; 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=PwS3b7B9P4IOvh1FxRsjBih5RmHPDXVSqQSTvHFZkK0=; b=FuJlL7Qd1U1oFvUInE9ogEsfbqNHfT2/69wyu1OIF5ezzIH4jLNMcnLzbAMw/x0mwo XtELWrtr/HOwhY0E9fWnM5W0nTCnn1SDwjtNe07WFf2pEYGVoRfFy5k+NfBRL7FZfK+A kk3kY2FSO1DDNtUXJE2ak1YYW6MDGbvhG3cXeqhOCbwoeICOURU0cMHVbwkUvUeVYMF2 SNflA0U4NzcqlGbQUz8J9P/pG6hgcmYIPbYflVh8DND681oB01O5o8055iwga5jXrbWH N8zaDuNP0O+gzficy8B7EpHYNoiL4bL5R5PiBMYcM9crhFm11OwFR8MWHjd7Fq42l26W ZyEQ== X-Gm-Message-State: AOJu0YxSkfRSSTevZbRHvs3beCQ6S7qsDX/X5OZw8XgJtTaavwjU0KBd kSiHuejWdKYaLx3S+BGT+tZx9UC13kBkrunhyaFhOYnOA2OzmdJRskX8E9NwIRCC/PwGhtFQ9a+ hsG0ST1+KBeo2ZWziqbjNuKVIVo+umwLsQiyCjtg6dSBOQX5tKhxKPA== X-Gm-Gg: AeBDieuGBu++ZxNmiJd2JoHpwJh7ivOGxWeXuyM7xqPGX0FV97/a/lRPt49FBn5QwW2 FWKHr32PzlkwvFU8YpMivQmT90SlaeWU22hHSnONdblpw/9V+9vWyeyelP00I3pMf7kFX4jjchT Pzk5PgvnW1lk8P15U6VrZv5wFUPSb463Rgj0rVHG5ELTyJAlD8zIzf3B3uFIbs7YRbKyEQol63C 2bDiA8saLQLUNiHNIuZJcl6k7l0mexx1mY4VE7WcyMJkZO11mP7c3sxxfdCGHqk0Q0niU37HMF3 7OCnmB7+tg/U1nWDJkQiG+X7exIBk1YGVDy8sZ58we0PcqvRDn3i5B3yIzHWXPmEqkg5txXDV2g E9vcOCNhYpl5UjyeOaPTa0XginDFFXz3YgKrDrqsU1wA9sMk0zIvkBYyLiOkz X-Received: by 2002:a05:600c:30d3:b0:485:3b9e:caa7 with SMTP id 5b1f17b1804b1-48e51f49403mr24890155e9.23.1778057763873; Wed, 06 May 2026 01:56:03 -0700 (PDT) X-Received: by 2002:a05:600c:30d3:b0:485:3b9e:caa7 with SMTP id 5b1f17b1804b1-48e51f49403mr24889895e9.23.1778057763244; Wed, 06 May 2026 01:56:03 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-48e538a517bsm34056075e9.4.2026.05.06.01.56.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 May 2026 01:56:02 -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: <20260506105601.5bfa57c7@elisabeth> In-Reply-To: References: <20260505234719.1437340-1-sbrivio@redhat.com> <20260505234719.1437340-20-sbrivio@redhat.com> <20260506102220.66a5c7a5@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 10:56:02 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 9mFnRaNpPnq_01Sd1EUklN0LWBr_mq2Q7s1zn606wqg_1778057764 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 6CLTG4BAP37B54233ZKDP7T4NITRKRQV X-Message-ID-Hash: 6CLTG4BAP37B54233ZKDP7T4NITRKRQV 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 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. -- Stefano