Re: [PATCH v6 2/6] dhcp: Add option state management with enum opt_state

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 24 Aug 2026 19:14:30 +0530
Anshu Kumari <anskuma@redhat.com> wrote:

> Overloading slen = -1 to mean "not set" conflates the option
> length with its lifecycle state, and can't express additional
> states such as "set by the user via command-line" which will
> be needed for --dhcp-opt support.
> 
> Introduce enum opt_state with OPT_UNSET (option not
> configured) and OPT_DEFAULT (derived from host
> configuration).  Replace all slen = -1 / slen != -1 checks
> with state comparisons, and set state = OPT_DEFAULT for
> options initialised in dhcp_init() and at reply time in
> dhcp().
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
> v6:
>   - Rewrote commit message to explain why slen = -1 overloading
>     is problematic and how it motivates the enum.
>   - Dropped explicit OPT_UNSET initialization loop inside dhcp_init().
>   - Realigned opts[51] initializer for consistency.
> 
> v5:
>   - New patch: introduce enum opt_state { OPT_UNSET, OPT_DEFAULT } to replace slen = -1 for tracking option state
>   - Replace all slen = -1 / slen != -1 checks with state = OPT_UNSET / state != OPT_UNSET
>   - Set OPT_DEFAULT for options initialised in dhcp_init() and at reply time
> ---
>  dhcp.c | 58 ++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index bb72b72..321968a 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -33,13 +33,24 @@
>  #include "log.h"
>  #include "dhcp.h"
>  
> +/**
> + * enum opt_state - DHCP option state

Nit: now that I see checks in later patches on .state (to check what
was configured) _and_ on .sent (to check the actual state), I wonder if
this shouldn't be called .conf instead, as it represents the
configuration state, rather than what one might more naturally called
state (sent, not sent, etc.).

That is, I would rename this to enum opt_conf, call it 'conf' in struct
opt, and change this comment to "DHCP option configuration". The names
of the possible values are fine as they are, I think.

> + * @OPT_UNSET:		Option not configured
> + * @OPT_DEFAULT:	Option derived from host configuration
> + */
> +enum opt_state {
> +	OPT_UNSET = 0,
> +	OPT_DEFAULT,
> +};
> +
>  /**
>   * struct opt - DHCP option
>   * @sent:	Convenience flag, set while filling replies
> - * @slen:	Length of option defined for server, -1 if not going to be sent
> + * @slen:	Length of option defined for server
>   * @s:		Option payload from server
>   * @clen:	Length of option received from client, -1 if not received
>   * @c:		Option payload from client
> + * @state:	Option state (unset or default)
>   */
>  struct opt {
>  	int sent;
> @@ -47,6 +58,7 @@ struct opt {
>  	uint8_t s[255];
>  	int clen;
>  	uint8_t c[255];
> +	enum opt_state state;
>  };
>  
>  static struct opt opts[256];
> @@ -73,19 +85,17 @@ static struct opt opts[256];
>   */
>  void dhcp_init(void)
>  {
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(opts); i++)
> -		opts[i].slen = -1;
> -
> -	opts[1]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Mask */
> -	opts[3]  = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Router */
> -	opts[51] = (struct opt) { 0, 4, {  0xff,
> -					   0xff,
> -					   0xff,
> -					   0xff }, 0, { 0 }, };	/* Lease time */
> -	opts[53] = (struct opt) { 0, 1, {     0 }, 0, { 0 }, };	/* Type */
> -	opts[54] = (struct opt) { 0, 4, {     0 }, 0, { 0 }, };	/* Server ID */
> +	/* Mask */
> +	opts[1]  = (struct opt) { 0, 4, {  0 }, 0, { 0 }, OPT_DEFAULT, };
> +	/* Router */
> +	opts[3]  = (struct opt) { 0, 4, {  0 }, 0, { 0 }, OPT_DEFAULT, };
> +	/* Lease time */
> +	opts[51] = (struct opt) { 0, 4, { 0xff, 0xff, 0xff, 0xff },
> +						       0, { 0 }, OPT_DEFAULT };
> +	/* Type */
> +	opts[53] = (struct opt) { 0, 1, {  0 }, 0, { 0 }, OPT_DEFAULT, };
> +	/* Server ID */
> +	opts[54] = (struct opt) { 0, 4, {  0 }, 0, { 0 }, OPT_DEFAULT, };
>  }
>  
>  /**
> @@ -183,13 +193,13 @@ static int fill(struct msg *m)
>  
>  	for (i = 0; i < opts[55].clen; i++) {
>  		o = opts[55].c[i];
> -		if (opts[o].slen != -1)
> +		if (opts[o].state != OPT_UNSET)

Pre-existing, but it would be convenient to fix this here: whenever we
have multiple _lines_ (regardless of statements) in the body of a
conditional clause, we use curly brackets, even if not needed, to
decrease the risk of mistakes later.

This is the same as the "netdev" Linux kernel coding style.

>  			if (fill_one(m->o, OPT_MAX, o, &offset))
>  				debug("DHCP: skipping option %i", o);
>  	}
>  
>  	for (o = 0; o < 255; o++) {
> -		if (opts[o].slen != -1 && !opts[o].sent)
> +		if (opts[o].state != OPT_UNSET && !opts[o].sent)

Same here.

>  			if (fill_one(m->o, OPT_MAX, o, &offset))
>  				debug("DHCP: skipping option %i", o);
>  	}
> @@ -243,6 +253,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
>  	int i;
>  
>  	opts[119].slen = 0;
> +	opts[119].state = OPT_DEFAULT;
>  
>  	for (i = 0; i < 255; i++)
>  		max_len -= opts[i].slen;
> @@ -291,7 +302,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
>  	}
>  
>  	if (!opts[119].slen)
> -		opts[119].slen = -1;
> +		opts[119].state = OPT_UNSET;
>  }
>  
>  /**
> @@ -389,7 +400,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  		iov_drop_header(data, *olen);
>  	}
>  
> -	opts[80].slen = -1;
> +	opts[80].state = OPT_UNSET;
>  	if (opts[53].clen > 0 && opts[53].c[0] == DHCPDISCOVER) {
>  		if (opts[80].clen == -1) {
>  			info("DHCP: offer to discover");
> @@ -398,6 +409,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  			info("DHCP: ack to discover (Rapid Commit)");
>  			opts[53].s[0] = DHCPACK;
>  			opts[80].slen = 0;
> +			opts[80].state = OPT_DEFAULT;
>  		}
>  	} else if (opts[53].clen <= 0 || opts[53].c[0] == DHCPREQUEST) {
>  		info("%s: ack to request", /* DHCP needs a valid message type */
> @@ -421,6 +433,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  	    != (c->ip4.guest_gw.s_addr & mask.s_addr)) {
>  		/* a.b.c.d/32:0.0.0.0, 0:a.b.c.d */
>  		opts[121].slen = 14;
> +		opts[121].state = OPT_DEFAULT;
>  		opts[121].s[0] = 32;
>  		memcpy(opts[121].s + 1,
>  		       &c->ip4.guest_gw, sizeof(c->ip4.guest_gw));
> @@ -430,6 +443,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  
>  	if (c->mtu) {
>  		opts[26].slen = 2;
> +		opts[26].state = OPT_DEFAULT;
>  		opts[26].s[0] = c->mtu / 256;
>  		opts[26].s[1] = c->mtu % 256;
>  	}
> @@ -441,12 +455,15 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  		((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i];
>  		opts[6].slen += sizeof(uint32_t);
>  	}
> -	if (!opts[6].slen)
> -		opts[6].slen = -1;
> +	if (opts[6].slen)
> +		opts[6].state = OPT_DEFAULT;
> +	else
> +		opts[6].state = OPT_UNSET;

As Laurent mentioned for the loop setting everything to OPT_UNSET: no
need for this. You already set OPT_UNSET to 0 to indicate the initial
mode.

>  
>  	opt_len = strlen(c->hostname);
>  	if (opt_len > 0) {
>  		opts[12].slen = opt_len;
> +		opts[12].state = OPT_DEFAULT;
>  		memcpy(opts[12].s, &c->hostname, opt_len);
>  	}
>  
> @@ -463,6 +480,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  			encode_domain_name((char *)opts[81].s + 3, c->fqdn);
>  
>  			opts[81].slen = opt_len;
> +			opts[81].state = OPT_DEFAULT;
>  		} else {
>  			debug("DHCP: client FQDN option doesn't fit, skipping");
>  		}

-- 
Stefano