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

On Thu, 1 Oct 2026 18:45:56 +0530 Anshu Kumari 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 --- 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