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=JrfG0oHF; 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 B93CD5A026D for ; Mon, 15 Jun 2026 08:42:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781505731; 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=s91Z16eFFMiAk03SAGDAD6v9MF+6bHFbKMVIBa+MHxs=; b=JrfG0oHF10vOAzJmfOcs3DlzGp69Q0aeuLHKrk5zbSa9ILnWru2WOfWjg3oSqnKzzuEUFk tVn6cRzH003AnWnRMAS5ewv6qyu3Kd61n465/JqSm7rdGPriqZEvWjhC6cJLFdCyylMZzb XTSLkUknewYR0ZXG5L+udn6l2LOszCI= 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-173-ZjOFPjlWNzKFUjleND9KJw-1; Mon, 15 Jun 2026 02:42:10 -0400 X-MC-Unique: ZjOFPjlWNzKFUjleND9KJw-1 X-Mimecast-MFC-AGG-ID: ZjOFPjlWNzKFUjleND9KJw_1781505729 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-490b61243easo32135725e9.2 for ; Sun, 14 Jun 2026 23:42:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781505729; x=1782110529; 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=s91Z16eFFMiAk03SAGDAD6v9MF+6bHFbKMVIBa+MHxs=; b=suklimXKKoJKDTVYuc7D7NsFNJ+in7MwJJ7MKQwiP0/RZrd/PgO78cjgMFwwL24OLQ 1Q8onJk8Wb4WDw+Brjbgg9O2HZKg241th1oRrvYzEM6yWoHROyLHUq7R0+4lmQxIu0kH 94COvfK/p2Sxjc4EmWm2J+WmcatnD7rij6rfv4JpXTkyA3tZ4hAy+SAqQ0XMZ+o7RG5z ocqTBmi/IP4bHEU0zs+L2b2xmhvzfS1myV1QytC/gxv/QbNPZ6ugtxOckLn7Qr/Sko6K RVmdYvrzuYGOCR3soFzBBTdHiUzzO4juFW0MKqTktiX8OFOw56RD+aZrv9TepeczN7UN CzUQ== X-Forwarded-Encrypted: i=1; AFNElJ98v0jEdkWDttdAF1bcF6DnK0uBdsJmddGm9ilu5HYHojqa3m9dR7onWeSht0jDstt6X5zAhmG3lHw=@passt.top X-Gm-Message-State: AOJu0YwdpUh2q3X3GSGCDGTGnwHwcrzEUXiqq7owv+TDnN8FQaVmM43j vggGLVeKbxxtHwdzkE1l93a9UfEXhtW3ALDnq1foJnZrNpTfZt7puACNF9n41ejm9t5+qMItS67 BiTn7zRDaXzCJCQIMpGqBcZa2QiYJfSPbiKb3rNUqLL3BWMTomMxHiw== X-Gm-Gg: Acq92OEUmFU2anYGtO/ieUYxHr6NAJ2IEMzXNoOa4vLCQ98XDwDLUk3jOU28h7oAvxz zg/7CabGPMsrIPqiDPiXsNThcTc9IdS70R45/PbMWi/S2GzO7pKMGfyIwhDlzHznux5lzIxZnZq /g4KPuz345V5wTDGMA4Hu1jstgVeAh7BTo54MFqP7sAY+Pu+ViHIzWY0cyC+dO5ApbyZqy6kQAd EJyUkmtBE291qHUgMk0/yT8Oekg+rIXyDtIEzA8y3Y5FTH9RmDAVIb2klCfScZ5lR6WD5g3WuFe nrUvI5d7I/NXRbcQH6qJqsNF4Hy5smVKAsIXCg2c2aUH+moYTuVsfUPdQIonhIvd0nwxRQbOUzZ tpKZXdRpoOQMS4oR82L1YhHPJ8AYhkfSN X-Received: by 2002:a05:600c:82c3:b0:490:bb45:79f0 with SMTP id 5b1f17b1804b1-490ec499290mr152532065e9.3.1781505728449; Sun, 14 Jun 2026 23:42:08 -0700 (PDT) X-Received: by 2002:a05:600c:82c3:b0:490:bb45:79f0 with SMTP id 5b1f17b1804b1-490ec499290mr152531665e9.3.1781505727868; Sun, 14 Jun 2026 23:42:07 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49220308f13sm238123705e9.5.2026.06.14.23.42.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 14 Jun 2026 23:42:07 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 1/6] conf: Add --dhcp-opt command-line option Message-ID: <20260615084205.23ca96a2@elisabeth> In-Reply-To: References: <20260601073758.1571317-1-anskuma@redhat.com> <20260601073758.1571317-2-anskuma@redhat.com> <20260612010426.319bc57d@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: Mon, 15 Jun 2026 08:42:06 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: YzYNE5lbfj7GFGVC0Bj5fO-2kzA41v6KbN_xv9ftf7Y_1781505729 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: JXLECK2EBBNKUPMDAGL75T7UNBWIFSDK X-Message-ID-Hash: JXLECK2EBBNKUPMDAGL75T7UNBWIFSDK 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: Anshu Kumari , passt-dev@passt.top, jmaloy@redhat.com, lvivier@redhat.com 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 Sat, 13 Jun 2026 12:50:55 +1000 David Gibson wrote: > On Fri, Jun 12, 2026 at 01:04:28AM +0200, Stefano Brivio wrote: > > Sorry for the late review. > > > > I have a few remarks on top of the one from David (which is the only one > > left that's really critical, I guess): > > > > On Mon, 1 Jun 2026 13:07:51 +0530 > > Anshu Kumari wrote: > > > > > Introduce the --dhcp-opt flag that allows setting arbitrary DHCP > > > options from command-line in the form of [Option CODE,VALUE]. > > > This patch adds the option storage in struct ctx and CLI parsing; > > > the type-aware value parser and DHCP reply injection follow > > > in subsequent patches. > > > > This split makes the patch smaller, but not necessarily easier to > > review, or maybe actually harder: > > > > - as David noted, it would be preferable to have man page changes > > together with functional changes they relate to, but not just for > > correctness: I personally use those during reviews to double check if > > the implementation corresponds to the intention. > > > > That is, in some sense, I use man pages as specification, which is > > particularly fitting when it comes to new command line options. So, > > from my side, while it's not really a blocker, my preference to have > > those man page changes together with the patch implementing the code > > changes is probably a bit stronger than the one expressed by David. > > > > It's not just for review right now, it also helps investigation later > > when somebody finds issues: having a more comprehensive description > > in this patch itself means not having to correlate multiple patches. > > > > Think of bisecting an issue and finding that this patch breaks > > something: git reaches this patch, and now you don't have the man > > page in the checked out tree at all... > > > > - I've been asking myself: is dhcp_add_option() matter for conf.c, > > rather than for dhcp.c? That is, is it merely a matter of > > configuration parsing / handling, or a part of the DHCP > > implementation proper? > > It's somewhat arbitrary, but I think having it in dhcp.c means > slightly less symbols that need to be exported in the headers. Sure, it's _a bit_ arbitrary, but we don't parse the port forwarding configuration from the command line in fwd.c, which makes that somewhat less arbitrary. On the other hand, if it really gets messy with symbols, by all means, let's keep it in dhcp.c. > > I needed to reach 3/6 and actually grasp 3/6 to answer this > > question... which seems to be a good indication that this change > > belongs to the same patch as 3/6. And I had similar fundamental > > questions around this patch that I could only resolve by reading 3/6. > > > > For me, a more reasonable split would have been something like: > > > > - 1/4 dhcp: Refactor fill_one() to operate on a generic buffer > > > > it's a dependency for 3/4 but doesn't depend on others > > > > - 2/4 dhcp: Add option overload > > > > also a dependency of 3/4, it only depends on 1/4 anyway > > > > - 3/4 dhcp: Add --dhcp-opt with option table and value parser > > > > the main feature, with man page for --dhcp-opt, and a clear match > > between what you parse from conf() and how it's used > > > > - 4/4 conf: Add --dhcp-boot command-line option > > > > with its part of man page, as it depends on 3/4 but no other bits > > seem to depend on it > > > > ...I'm not sure how much effort that is at this point, but I think it > > would be nice for the revision history (current review, doesn't matter > > so much, as both David and myself are now familiar with it). > > I agree that would be a better split of the series. For my part I'm > not sure it's worth rearranging at this stage, though. > > [snip] > > > + > > > +/** > > > + * dhcp_add_option() - Add or update a custom DHCP option > > > > It's not clear where it's added, which is rather fundamental (to a > > reply message or to the configuration?) > > Ah, yeah "add option" probably wasn't the best suggestion for the name > of this function. > > > "Set" can replace "Add or update", and "custom" is not really important > > or well defined I think (what makes an option custom? The fact that > > it's not assigned by IANA or the fact that it's specified by the user? > > But then what's not custom...?). > > Fair point. In practice, "custom" here means *directly* specified by > the user, rather than generated by passt. But I agree it's not really > a helpful distinction. > > > diff --git a/passt.h b/passt.h > > > index 1726965..3a0816f 100644 > > > --- a/passt.h > > > +++ b/passt.h > > > @@ -182,6 +182,10 @@ struct ip6_ctx { > > > * @dns_search: DNS search list > > > * @hostname: Guest hostname > > > * @fqdn: Guest FQDN > > > + * @custom_opts: User-specified DHCP options from --dhcp-opt > > > > I think this should be called @dhcp_opts, because @custom_opts in > > the... context of ctx isn't really clear. They're all custom anyway in > > some sense. > > > > > + * @custom_opts.code: DHCP option code > > > + * @custom_opts.str: Original string value from command line > > > > It's the only one, there isn't one that's original and one that isn't > > (right?). > > There is in a later patch, more comments on that there. There's another value (also stored here, I realised later), but that wasn't really my point (which is why I kept this comment). I was rather suggesting that there's no non-original string value from the command line, so "string value from command line" would have been as descriptive. On the other hand when I wrote this comment I hadn't reached 5/6 yet (I thought it was about option overload...), and, with that, this comment makes definitely more sense. I still wanted to point out that it's somewhat redundant, but that's very arbitrary/minor (we have a lot of redundant stuff in comments and code anyway). -- Stefano