Re: [PATCH v5 2/7] dhcp: Add option state management with enum opt_state

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 17 Jul 2026 23:26:39 +0530
Anshu Kumari <anskuma@redhat.com> wrote:

> Introduce enum opt_state to track each DHCP option instead
> of overloading slen = -1 for "not set".
> 
> OPT_UNSET means the option is not configured.
> OPT_DEFAULT means the option was set from host configuration.
> 
> This replaces all slen = -1 / slen != -1 checks with
> state = OPT_UNSET / state != OPT_UNSET, and sets 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>
> ---
> 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 | 57 ++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index bb72b72..e5d89fc 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -33,13 +33,24 @@
>  #include "log.h"
>  #include "dhcp.h"
>  
> +/**
> + * enum opt_state - DHCP option state
> + * @OPT_UNSET:		Option not configured
> + * @OPT_DEFAULT:	Option set from host config

Nit: "config" isn't an actual word, it's a common abbreviation, but
here we don't need it and we could use the original noun.

I would actually say "derived from host configuration", to make it
clear that we're not recycling any kind of DHCP server configuration
find on the host.

> + */
> +enum opt_state {
> +	OPT_UNSET,
> +	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];
> @@ -76,16 +88,19 @@ 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 */
> +		opts[i].state = OPT_UNSET;

This was needed because we were setting opts[i].state to -1. But now
OPT_UNSET is 0, so you could force the assignment in the enum just to
make that clear, and drop this loop.

> +
> +	/* 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, };

Nit: if you write it like this:

	opts[51] = (struct opt) { 0, 4, { 0xff, 0xff, 0xff, 0xff },
						0, { 0 }, OPT_DEFAULT, };

at least the last three fields are aligned with the other options and
slightly more readable, I think.

> +	/* 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, };
>  }

-- 
Stefano