Re: [PATCH v3 1/6] conf: Add --dhcp-opt command-line option

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5120 bytes --]

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 <anskuma@redhat.com> 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]