From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=PkAcaqlF; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id A7BC35A0262 for ; Sat, 13 Jun 2026 04:51:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1781319097; bh=1DILPrClWjM0IMb+8eMgt7yMwCCcT89p6OV4mwcxI4k=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PkAcaqlFdlHho85+/XEbSX7pmrH3YFv8aTZDFxsvD7Go/aU1eXzm36/UOOobvgsa8 glTR9aAR+YbqkqyjC+LG68wuBMzLpDtHcCW3q84DSZM/DxRyVCe4f9NXTEoH/Z/Ou4 L1ktXCAcMR6XSYMnVwIdHhbNoCM2ZZyBeMGRPeIMhrB92BZoSYFU4XSOsB+ddG3WLN 2ZGxnsuFMSZmS6MrZb3iHuW99jlQLd6ybQZ3jsxguyYlFwi7mRBipGku/Xdus68Xn2 0bZNpss4TAJg9P4FOySn7gn6Qokwv88dwCDeLyTZUKKX4yQmWdzSQR8C5ZhjdccJK2 du0a3gR+qMYpQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gcgrT5xqpz58mF; Sat, 13 Jun 2026 12:51:37 +1000 (AEST) Date: Sat, 13 Jun 2026 12:50:55 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 1/6] conf: Add --dhcp-opt command-line option Message-ID: References: <20260601073758.1571317-1-anskuma@redhat.com> <20260601073758.1571317-2-anskuma@redhat.com> <20260612010426.319bc57d@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="FPd7jOg74XtWjfjy" Content-Disposition: inline In-Reply-To: <20260612010426.319bc57d@elisabeth> Message-ID-Hash: SUYMQDKCYC2Y3AEYJC42XHG2E5IUTUDU X-Message-ID-Hash: SUYMQDKCYC2Y3AEYJC42XHG2E5IUTUDU X-MailFrom: dgibson@gandalf.ozlabs.org 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: --FPd7jOg74XtWjfjy Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jun 12, 2026 at 01:04:28AM +0200, Stefano Brivio wrote: > Sorry for the late review. >=20 > I have a few remarks on top of the one from David (which is the only one > left that's really critical, I guess): >=20 > On Mon, 1 Jun 2026 13:07:51 +0530 > Anshu Kumari wrote: >=20 > > 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. >=20 > This split makes the patch smaller, but not necessarily easier to > review, or maybe actually harder: >=20 > - 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. >=20 > 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. >=20 > 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. >=20 > 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... >=20 > - 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. > 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. >=20 > For me, a more reasonable split would have been something like: >=20 > - 1/4 dhcp: Refactor fill_one() to operate on a generic buffer >=20 > it's a dependency for 3/4 but doesn't depend on others >=20 > - 2/4 dhcp: Add option overload >=20 > also a dependency of 3/4, it only depends on 1/4 anyway >=20 > - 3/4 dhcp: Add --dhcp-opt with option table and value parser >=20 > the main feature, with man page for --dhcp-opt, and a clear match > between what you parse from conf() and how it's used >=20 > - 4/4 conf: Add --dhcp-boot command-line option >=20 > with its part of man page, as it depends on 3/4 but no other bits > seem to depend on it >=20 > ...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 >=20 > 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 >=20 > 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. >=20 > > + * @custom_opts.code: DHCP option code > > + * @custom_opts.str: Original string value from command line >=20 > 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. --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --FPd7jOg74XtWjfjy Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmosxXYACgkQzQJF27ox 2GfhfhAAnt6yOJfkNolP6PriWJJHXibfmgpVZRWRhAwleuAVvERLKu8xmr3sOnwS lb52cx26BFvwJJ8W9z3fijaNAnd54HanHRC3lXB4vRiuiarb8qXhW6fcpBpaUq8F R+UsGS3z3Ul4Jl6/9g52sTbjd6n4M7+bH2CuNrHg/HjSXmLSMVGVy9XoR8PQaDAA EsT2neDUoLexKEZ7YDMFmRwXDyOeoFH+Rj55YWU54c96AeIX193rgoEG287dqhdY gqBf5FDcuJNQ65QYU+hadzqRnJjHfzbPfJATsu5QU1DdxXAz2+DP3RGRlgUzVv4W ZGacd4oWbRvHYRn562MBcVjHbKlktpKDrmair38H8wr5hjv3ggneoyOgh4iKOODd aSToxvTvIOI67jpSCpwxoAcvRPUAP8Y3jxFaJauf7eFfsIQxXgrDSiKr44lXOd1G 2Fv5zSr16JClR9sG8x3+IoRaWJLWjerMyqdwfNK3jVAL2PO+NIO/yjquK5FpVTI2 bQwx7lCO64ROWhu+g3wEb4sDPuqWLIezxffRRo4Iu8cDSv+iakdYht7fzloIu0Ps 0NisltTS4FKcXiGp8cR8bU30uDRZII5NGQvx38GJ8FC/6YtYOru4IkFDRaxVDM/a 7cYDUCVQiIBGLLB8xNenYsXTvykd/ioFMN97eZn/MRHze9rSF7Y= =eaTG -----END PGP SIGNATURE----- --FPd7jOg74XtWjfjy--