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. > 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. -- 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