Re: [PATCH 3/6] dhcp: Add option type table and value parser

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 18 May 2026 18:49:59 +0530
Anshu Kumari <anskuma@redhat.com> wrote:

> Add an RFC 2132 type lookup table mapping DHCP option codes to their
> expected value formats, and a dhcp_opt_parse() function that converts
> CLI string values into their binary wire representation.
> 
> Wire dhcp_opt_parse() into the --dhcp-opt handler so that values are
> validated and encoded at configuration time.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
>  conf.c |   9 +++
>  dhcp.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  dhcp.h |  15 ++++
>  3 files changed, 251 insertions(+)
> 
> diff --git a/conf.c b/conf.c
> index 61a393f..3ec10ac 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1485,6 +1485,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			unsigned long code;
>  			const char *comma;
>  			char *end;
> +			int len;
>  
>  			comma = strchr(optarg, ',');
>  			if (!comma)
> @@ -1499,7 +1500,15 @@ void conf(struct ctx *c, int argc, char **argv)
>  				die("Too many --dhcp-opt entries (max %d)",
>  				    MAX_CUSTOM_DHCP_OPTS);
>  
> +			len = dhcp_opt_parse(code, comma + 1,
> +					     c->custom_opts[c->custom_opts_count].val,
> +					     sizeof(c->custom_opts[0].val));
> +			if (len < 0)
> +				die("Invalid value for DHCP option %lu: %s",
> +				    code, comma + 1);
> +
>  			c->custom_opts[c->custom_opts_count].code = code;
> +			c->custom_opts[c->custom_opts_count].len = len;
>  			if (snprintf_check(c->custom_opts[c->custom_opts_count].str,
>  					   sizeof(c->custom_opts[0].str),
>  					   "%s", comma + 1))
> diff --git a/dhcp.c b/dhcp.c
> index 1ff8cba..9220516 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -33,6 +33,233 @@
>  #include "log.h"
>  #include "dhcp.h"
>  
> +/**
> + * struct dhcp_opt_type_entry - Maps option code to RFC 2132 value type
> + * @code:	DHCP option code
> + * @type:	Expected value format
> + */
> +static const struct dhcp_opt_type_entry {
> +	uint8_t code;
> +	enum dhcp_opt_type type;
> +} dhcp_opt_types[] = {
> +	{  1, DHCP_OPT_IPV4 },		/* Subnet Mask */
> +	{  2, DHCP_OPT_UINT32 },	/* Time Offset */
> +	{  3, DHCP_OPT_IPV4_LIST },	/* Router */
> +	{  4, DHCP_OPT_IPV4_LIST },	/* Time Server */
> +	{  5, DHCP_OPT_IPV4_LIST },	/* Name Server */
> +
> +	{  6, DHCP_OPT_IPV4_LIST },	/* Domain Name Server */
> +	{  7, DHCP_OPT_IPV4_LIST },	/* Log Server */
> +	{  8, DHCP_OPT_IPV4_LIST },	/* Cookie Server */
> +	{  9, DHCP_OPT_IPV4_LIST },	/* LPR Server */
> +	{ 10, DHCP_OPT_IPV4_LIST },	/* Impress Server */
> +
> +	{ 11, DHCP_OPT_IPV4_LIST },	/* Resource Location Server */
> +	{ 12, DHCP_OPT_STR },		/* Host Name */
> +	{ 13, DHCP_OPT_UINT16 },	/* Boot File Size */
> +	{ 15, DHCP_OPT_STR },		/* Domain Name */
> +	{ 16, DHCP_OPT_IPV4 },		/* Swap Server */
> +
> +	{ 17, DHCP_OPT_STR },		/* Root Path */
> +	{ 19, DHCP_OPT_UINT8 },	/* IP Forwarding */
> +	{ 23, DHCP_OPT_UINT8 },	/* Default IP TTL */
> +	{ 26, DHCP_OPT_UINT16 },	/* Interface MTU */
> +	{ 28, DHCP_OPT_IPV4 },		/* Broadcast Address */
> +
> +	{ 33, DHCP_OPT_IPV4_LIST },	/* Static Routes (dest+router pairs) */
> +	{ 37, DHCP_OPT_UINT8 },	/* TCP Default TTL */
> +	{ 38, DHCP_OPT_UINT32 },	/* TCP Keepalive Interval */
> +	{ 40, DHCP_OPT_STR },		/* NIS Domain Name */
> +	{ 41, DHCP_OPT_IPV4_LIST },	/* NIS Servers */
> +
> +	{ 42, DHCP_OPT_IPV4_LIST },	/* NTP Servers */
> +	{ 44, DHCP_OPT_IPV4_LIST },	/* NetBIOS Name Server */
> +	{ 50, DHCP_OPT_IPV4 },		/* Requested IP Address */
> +	{ 51, DHCP_OPT_UINT32 },	/* IP Address Lease Time */
> +	{ 53, DHCP_OPT_UINT8 },	/* DHCP Message Type */
> +
> +	{ 54, DHCP_OPT_IPV4 },		/* Server Identifier */
> +	{ 57, DHCP_OPT_UINT16 },	/* Max DHCP Message Size */
> +	{ 58, DHCP_OPT_UINT32 },	/* Renewal (T1) Time */
> +	{ 59, DHCP_OPT_UINT32 },	/* Rebinding (T2) Time */
> +	{ 60, DHCP_OPT_STR },		/* Vendor Class Identifier */
> +
> +	{ 61, DHCP_OPT_STR },		/* Client Identifier */
> +	{ 66, DHCP_OPT_STR },		/* TFTP Server Name */
> +	{ 67, DHCP_OPT_STR },		/* Bootfile Name */
> +	{ 119, DHCP_OPT_STR },		/* Domain Search List (RFC 3397) */
> +	{ 121, DHCP_OPT_ROUTES },	/* Classless Static Routes */
> +
> +	{ 252, DHCP_OPT_STR },		/* WPAD URL */
> +};
> +
> +/**
> + * dhcp_opt_type_lookup() - Look up the value type for a DHCP option code
> + * @code:	DHCP option code
> + *
> + * Return: type from table
> + */
> +static enum dhcp_opt_type dhcp_opt_type_lookup(uint8_t code)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dhcp_opt_types); i++) {
> +		if (dhcp_opt_types[i].code == code)
> +			return dhcp_opt_types[i].type;
> +	}
> +
> +	return DHCP_OPT_NONE;
> +}

Not a complete review, but here's something that occurred to me while
browsing this quickly (I plan to finish reviewing at some point, but
meanwhile feel free to post a v2 if you have enough changes).

You're defining 41 options here, and it might look like a waste to just
make an array with 255 elements to store those, so you added an index
(not the natural index of the array). And a lookup function. And one
call to that lookup function (instead of direct addressing).

Now, if you try this:

---
$ CFLAGS="-g" && make && pahole passt | grep -A10 dhcp_opt_type_entry
struct dhcp_opt_type_entry {
	uint8_t                    code;                 /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	enum dhcp_opt_type         type;                 /*     4     4 */

	/* size: 8, cachelines: 1, members: 2 */
	/* sum members: 5, holes: 1, sum holes: 3 */
	/* last cacheline: 8 bytes */
};
---

you'll see that a table with an index takes double the size for each
element, it's 8 bytes instead of 4. With 41 elements, we are at 41 * 8
= 328 bytes instead of 255 * 4 = 1020. Still a win so far.

Then, rebuild with -g -O0, and:

$ nm -td -Sr -P passt | grep dhcp_opt_type_lookup
dhcp_opt_type_lookup t 58041 85

(there are some examples of 'nm' usage in test/memory by the way).

Fine, that function is 85 bytes, we're at 413 bytes.

But you can probably expect (have a look with objdump -Ddslrx passt for
example) that having to call that function takes a few more
instructions compared to just a reference to an array, I guess you
might have 10 or 20 bytes more in total.

The easiest way (and most meaningful way) to check would be to compare
the binary sizes of the two versions (I didn't do it).

For passt, code or data doesn't really matter, it's all allocated
statically, so you would see it in the size.

But note that a static memory area (maybe at least a page size? I don't
remember) that's not initialised (completely zeroed), which would be
the case for all the options you don't define, doesn't take space.

The kernel would only allocate memory for us only as we write it. Again,
checking with nm:

$ nm -td -Sr --size-sort -P passt | head -2
tcp_migrate_snd_queue b 43143680 67108864
tcp_migrate_rcv_queue b 110252544 67108864

$ ls -lh passt
-rwxr-xr-x 1 sbrivio sbrivio 897K May 20 02:17 passt

...that's definitely less than those 128 MB we allocate statically for
the tcp_migrate_* buffers.

That being said, I expect that the version with a separate index field
and lookup function and calls to the lookup function actually takes
more memory than the "dumb" one.

And readability / keeping lines of code to a minimum is also relevant
here. For example, you could have:

> +/**
> + * dhcp_opt_parse() - Parse a DHCP option value
> + * @code:	DHCP option code (determines value type via lookup table)
> + * @str:	Value string from command line
> + * @buf:	Output buffer for binary value
> + * @buf_len:	Size of output buffer
> + *
> + * Return: number of bytes written to @buf, or -1 on error
> + */
> +int dhcp_opt_parse(uint8_t code, const char *str, uint8_t *buf, size_t buf_len)
> +{
> +	enum dhcp_opt_type type = dhcp_opt_type_lookup(code);
> +

	enum dhcp_opt_type type = dhcp_opt_types[code];

instead, and it's immediately clear where those definitions come from,
without having to read the lookup function first.


> +	switch (type) {
> +	case DHCP_OPT_NONE: {
> +		die("Unsupported DHCP option: %u,"
> +		    " see passt(1) for supported codes",
> +		    code);
> +	}
> +	case DHCP_OPT_IPV4: {
> +		struct in_addr addr;
> +
> +		if (inet_pton(AF_INET, str, &addr) != 1)
> +			return -1;
> +		if (buf_len < sizeof(addr))
> +			return -1;
> +		memcpy(buf, &addr, sizeof(addr));
> +		return sizeof(addr);
> +	}
> +	case DHCP_OPT_IPV4_LIST: {
> +		char *tok, *saveptr;
> +		char tmp[1024];
> +		int len = 0;
> +
> +		if (snprintf_check(tmp, sizeof(tmp), "%s", str))
> +			return -1;
> +
> +		for (tok = strtok_r(tmp, " ", &saveptr); tok;
> +		     tok = strtok_r(NULL, " ", &saveptr)) {
> +			struct in_addr addr;
> +
> +			if (inet_pton(AF_INET, tok, &addr) != 1)
> +				return -1;
> +			if (len + (int)sizeof(addr) > (int)buf_len)
> +				return -1;
> +			memcpy(buf + len, &addr, sizeof(addr));
> +			len += sizeof(addr);
> +		}
> +		return len;
> +	}
> +	case DHCP_OPT_UINT8: {
> +		unsigned long val;
> +		char *end;
> +
> +		val = strtoul(str, &end, 0);
> +		if (*end || val > 255 || buf_len < 1)
> +			return -1;
> +		buf[0] = val;
> +		return 1;
> +	}
> +	case DHCP_OPT_UINT16: {
> +		unsigned long val;
> +		char *end;
> +
> +		val = strtoul(str, &end, 0);
> +		if (*end || val > 65535 || buf_len < 2)
> +			return -1;
> +		buf[0] = (val >> 8) & 0xff;
> +		buf[1] = val & 0xff;
> +		return 2;
> +	}
> +	case DHCP_OPT_UINT32: {
> +		unsigned long val;
> +		char *end;
> +
> +		val = strtoul(str, &end, 0);
> +		if (*end || buf_len < 4)
> +			return -1;
> +		buf[0] = (val >> 24) & 0xff;
> +		buf[1] = (val >> 16) & 0xff;
> +		buf[2] = (val >>  8) & 0xff;
> +		buf[3] = val & 0xff;
> +		return 4;
> +	}
> +	case DHCP_OPT_ROUTES: {
> +		/* RFC 3442: "CIDR/mask,gateway" entries, space-separated
> +		 * Encodes as: mask-width + significant-octets + router
> +		 * e.g. "192.168.1.0/24,10.0.0.1 0.0.0.0/0,10.0.0.1"
> +		 */
> +		char *tok, *saveptr;
> +		char tmp[1024];
> +		int len = 0;
> +
> +		if (snprintf_check(tmp, sizeof(tmp), "%s", str))
> +			return -1;
> +
> +		for (tok = strtok_r(tmp, " ", &saveptr); tok;
> +		     tok = strtok_r(NULL, " ", &saveptr)) {
> +			struct in_addr dest, gw;
> +			char *slash, *comma;
> +			unsigned long mask;
> +			int sig_octets;
> +
> +			slash = strchr(tok, '/');
> +			if (!slash)
> +				return -1;
> +			*slash = '\0';
> +
> +			if (inet_pton(AF_INET, tok, &dest) != 1)
> +				return -1;
> +
> +			comma = strchr(slash + 1, ',');
> +			if (!comma)
> +				return -1;
> +			*comma = '\0';
> +
> +			mask = strtoul(slash + 1, NULL, 10);
> +			if (mask > 32)
> +				return -1;
> +
> +			if (inet_pton(AF_INET, comma + 1, &gw) != 1)
> +				return -1;
> +
> +			sig_octets = (mask + 7) / 8;
> +
> +			if (len + 1 + sig_octets + 4 > (int)buf_len)
> +				return -1;
> +
> +			buf[len++] = mask;
> +			memcpy(buf + len, &dest, sig_octets);
> +			len += sig_octets;
> +			memcpy(buf + len, &gw, 4);
> +			len += 4;
> +		}
> +		return len;
> +	}
> +	case DHCP_OPT_STR: {
> +		size_t len = strlen(str);
> +
> +		if (!len || len >= buf_len)
> +			return -1;
> +		strncpy((char *)buf, str, buf_len);
> +		return len;
> +	}
> +	}
> +
> +	return -1;
> +}
> +
>  /**
>   * struct opt - DHCP option
>   * @sent:	Convenience flag, set while filling replies
> diff --git a/dhcp.h b/dhcp.h
> index cd50c99..01b2290 100644
> --- a/dhcp.h
> +++ b/dhcp.h
> @@ -6,7 +6,22 @@
>  #ifndef DHCP_H
>  #define DHCP_H
>  
> +/**
> + * enum dhcp_opt_type - DHCP option value types per RFC 2132

Nit: this should be documented as well. We haven't been very consistent
with that, but (at least) enum tcp_iov_parts in tcp_internal.h is
properly documented in the kerneldoc style.

> + */
> +enum dhcp_opt_type {
> +	DHCP_OPT_NONE,
> +	DHCP_OPT_STR,
> +	DHCP_OPT_IPV4,
> +	DHCP_OPT_IPV4_LIST,
> +	DHCP_OPT_UINT8,
> +	DHCP_OPT_UINT16,
> +	DHCP_OPT_UINT32,
> +	DHCP_OPT_ROUTES,
> +};
> +
>  int dhcp(const struct ctx *c, struct iov_tail *data);
>  void dhcp_init(void);
> +int dhcp_opt_parse(uint8_t code, const char *str, uint8_t *buf, size_t buf_len);
>  
>  #endif /* DHCP_H */

-- 
Stefano