Re: [PATCH v7 3/7] dhcp: Add --dhcp-opt with option table and value parser

[Stefano Brivio <sbrivio@redhat.com>]

On Thu,  1 Oct 2026 18:45:56 +0530
Anshu Kumari <anskuma@redhat.com> wrote:

> Add a --dhcp-opt CODE,VALUE flag that sets any DHCP option by
> numeric code with type-aware parsing per RFC 2132.
> 
> A type lookup table maps option codes to RFC 2132 value types
> (IPv4, IPv4 list, integer, string).  dhcp_opt_parse() converts
> CLI strings to binary wire format; parsed options are stored in
> opts[] and injected into DHCP replies.  Options set via --dhcp-opt
> (OPT_USER) take priority over host-derived defaults (OPT_DEFAULT).
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
> v7:
>   - Removed client-only options 50 (Requested IP) and 57 (Max Message Size)
>     from dhcp_opt_types[]
>   - Switched integer parsing from strtoul()/strtol() to parse_unsigned()
>     for UINT8/UINT16/UINT32 types
>   - Fixed alignment: use htons()/htonl() + memcpy() instead of direct
>     pointer casts for UINT16/UINT32 encoding.
>   - Used explicit OPT_DEFAULT check instead of != OPT_USER where the condition
>     body does not set conf.
>   - Removed unnecessary OPT_USER guard on option 121
>   - Checked inet_ntop() return value in dhcp_opt_to_str()
> 
> v6:
>   - dropped option 53.
>   - Used parse_unsigned(), parse_literal(), parse_ipv4()
>     from parse.c instead of manual strtoul/inet_pton.
>   - Moved option-parsing variables into case 34 block
>     scope.

This change comes from David's suggestion in:

  https://archives.passt.top/passt-dev/al2c8QVZUuQB69HV@zatzit/

...but back then it was three variables. Now it's one:

> [...]
>
> @@ -1589,6 +1596,24 @@ void conf(struct ctx *c, int argc, char **argv)
>  		case 32:
>  			c->chroot_fallback = true;
>  			break;
> +		case 34: {
> +			unsigned long optcode;

...which I think could happily be declared at the top of the function
along with max_mtu.

In general, I think it would be good to avoid mixing up scoping logic
in case switches (we almost always avoid extra blocks, except for a few
cases here which I missed during review), because if we do the code
becomes a bit more surprising (where do you look for variable
declarations?).

Other than that, I see the point of keeping variable scope limited. But
here it's just one variable, so I think it could really be declared at
the beginning of the function without much thinking.

> [...]
>
> +/**
> + * dhcp_opt_to_str() - Render a binary DHCP option value to a printable string
> + * @code:	DHCP option code
> + * @buf:	Output string buffer
> + * @buf_len:	Size of output buffer
> + *
> + * Return: pointer to @buf if option is set, NULL otherwise
> + */
> +const char *dhcp_opt_to_str(uint8_t code, char *buf, size_t buf_len)
> +{
> +	enum dhcp_opt_type type;
> +	unsigned int i;
> +	int off = 0;
> +
> +	if (opts[code].conf == OPT_UNSET)
> +		return NULL;
> +
> +	assert(code < ARRAY_SIZE(dhcp_opt_types));
> +
> +	type = dhcp_opt_types[code];
> +
> +	switch (type) {
> +	case DHCP_OPT_IPV4:
> +	case DHCP_OPT_IPV4_LIST:
> +		for (i = 0; i + sizeof(struct in_addr) <= (unsigned int)opts[code].slen;

Given that opts[code].slen doesn't change in this loop, perhaps a
temporary variable holding it would make this more readable.

> +		     i += sizeof(struct in_addr)) {
> +			if (off) {
> +				if (off + 1 >= (int)buf_len)
> +					return NULL;
> +				buf[off++] = ',';
> +			}
> +			if (!inet_ntop(AF_INET, opts[code].s + i,
> +				       buf + off, buf_len - off))
> +				return NULL;
> +			off += strlen(buf + off);
> +		}
> +		return buf;
> +	case DHCP_OPT_UINT8:
> +	case DHCP_OPT_UINT16:
> +	case DHCP_OPT_UINT32: {

Same here ('uval'?).

> +		uint32_t val = 0;
> +
> +		if (opts[code].slen == 1) {
> +			val = opts[code].s[0];
> +		} else if (opts[code].slen == 2) {
> +			uint16_t v16;
> +			memcpy(&v16, opts[code].s, sizeof(v16));
> +			val = ntohs(v16);
> +		} else if (opts[code].slen == 4) {
> +			memcpy(&val, opts[code].s, sizeof(val));
> +			val = ntohl(val);
> +		}
> +
> +		if (snprintf(buf, buf_len, "%u", val) >= (int)buf_len)
> +			return NULL;
> +		return buf;
> +	}
> +	case DHCP_OPT_INT32: {
> +		int32_t val;

And here. This one is especially surprising because there's another
'val' just above, and if you miss that curly bracket then you would
think that the usual style / scoping applies, but it doesn't.

> +		uint32_t v32;
> +
> +		assert(opts[code].slen == 4);
> +		memcpy(&v32, opts[code].s, sizeof(v32));
> +		val = (int32_t)ntohl(v32);
> +
> +		if (snprintf(buf, buf_len, "%d", val) >= (int)buf_len)
> +			return NULL;
> +		return buf;
> +	}
> +	case DHCP_OPT_STR:
> +		(void)snprintf(buf, buf_len, "%.*s",
> +			       opts[code].slen, opts[code].s);
> +		return buf;
> +	default:
> +		assert(0);
> +	}
> +}
> +
>
> [...]

-- 
Stefano