From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=FtMgW7Y9; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 25A195A0269 for ; Wed, 27 May 2026 05:26:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1779852384; bh=hIrFc3L38e87WAfW0sYQOv+dChmdxvhVV6JAj2gazEE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FtMgW7Y9CIaMg0K2URk6L6s62DhvbsYLdnSyzNSwfap1g+R4WtHCKe234Ww+k/pVz 0eHo9oAUaRzBWy4WmlnllUIqlA0aLM4prf0wykqgZ1jZtvx/8P0zoHCW6DQD3/+jNy nkqwiBIexXc3Se+V6K0gVGAhOrE11hAVecWvfMKXckkpXVs8ykKW9FC6wOELy4//0X yy6/NJh6Hq65KtfUpheMwZRxEmo1jkBt1iGWrS3zYSnYqxWftXqwVOmAIHkrtNToOL cS8cr/ktYMARRvBIIGkqFCjqV6xnmSoR27YQ48d3ZGwjsMPDpW0qFF0IsnjoQsKNOJ nO6KfrNE2RA3A== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gQFQS0H4Rz4wDK; Wed, 27 May 2026 13:26:24 +1000 (AEST) Date: Wed, 27 May 2026 13:00:40 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v2 3/6] dhcp: Add option type table and value parser Message-ID: References: <20260526123115.1226166-1-anskuma@redhat.com> <20260526123115.1226166-4-anskuma@redhat.com> <46780f40-afb9-4a63-895c-9564dc0d6abe@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="O1HEjCbG/MqKuq18" Content-Disposition: inline In-Reply-To: <46780f40-afb9-4a63-895c-9564dc0d6abe@redhat.com> Message-ID-Hash: 2DFSAVD35OGH64TLWYLJP54GW56HEI7H X-Message-ID-Hash: 2DFSAVD35OGH64TLWYLJP54GW56HEI7H X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Anshu Kumari , passt-dev@passt.top, sbrivio@redhat.com, jmaloy@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --O1HEjCbG/MqKuq18 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, May 26, 2026 at 06:05:04PM +0200, Laurent Vivier wrote: > On 5/26/26 14:31, Anshu Kumari 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. > >=20 > > Wire dhcp_opt_parse() into the --dhcp-opt handler so that values are > > validated and encoded at configuration time. > >=20 > > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > > Signed-off-by: Anshu Kumari > > --- > > v2: > > - Replaced struct lookup table + dhcp_opt_type_lookup() function wit= h flat dhcp_opt_types[256] array indexed by code. > > - Consolidated DHCP_OPT_UINT8/UINT16/UINT32 into single DHCP_OPT_INT= EGER with dhcp_opt_int_width[256] table. > > - Dropped DHCP_OPT_ROUTES / option 121 entirely. > > - Added kerneldoc for enum dhcp_opt_type values. > > - Removed curly braces from switch cases, declarations before switch. > > - Added newlines before return statements. > > - Changed IP list delimiter from space to comma (--dhcp-opt 6,1.1.1.= 1,8.8.8.8). > > - Defined DHCP_OPT_PARSE_BUF constant for bare 1024. > > - Added len and val[255] fields to struct here (moved from patch=A01= ). > > - Added kerneldoc for @custom_opts.len and @custom_opts.val. > > - Wired dhcp_opt_parse() into case 32 (--dhcp-boot) to populate val/= len. > > --- > > conf.c | 16 ++++++ > > dhcp.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > dhcp.h | 17 +++++++ > > passt.h | 4 ++ > > 4 files changed, 185 insertions(+) > >=20 > > diff --git a/conf.c b/conf.c > > index ae8ee26..3a5f45d 100644 > > --- a/conf.c > > +++ b/conf.c > > @@ -1266,6 +1266,7 @@ void conf(struct ctx *c, int argc, char **argv) > > char *end; > > uid_t uid; > > gid_t gid; > > + int len; >=20 > I think you don't need to introduce len for --dhcp-opt as you already use= ret for --dhcp-boot Also the inline code here in conf.c is becoming fairly bulky, and there's a little duplication between the --dhcp-boot and --dhcp-opt paths. I think it might be worth making an dhcp_add_option() helper allowing more of this logic, including this local to be moved to a function in dhcp.c. That will probably also make life easier for handling repeated options. > > =09 > > if (c->mode =3D=3D MODE_PASTA) > > @@ -1482,7 +1483,14 @@ void conf(struct ctx *c, int argc, char **argv) > > die("Too many DHCP options (max %d)", > > MAX_CUSTOM_DHCP_OPTS); > > + ret =3D dhcp_opt_parse(67, optarg, > > + c->custom_opts[c->custom_opts_count].val, > > + sizeof(c->custom_opts[0].val)); > > + if (ret < 0) > > + die("Invalid boot file value: %s", optarg); > > + > > c->custom_opts[c->custom_opts_count].code =3D 67; > > + c->custom_opts[c->custom_opts_count].len =3D ret; > > if (snprintf_check(c->custom_opts[c->custom_opts_count].str, > > sizeof(c->custom_opts[0].str), > > "%s", optarg)) > > @@ -1503,7 +1511,15 @@ void conf(struct ctx *c, int argc, char **argv) > > die("Too many --dhcp-opt entries (max %d)", > > MAX_CUSTOM_DHCP_OPTS); > > + len =3D dhcp_opt_parse(optcode, comma + 1, > > + c->custom_opts[c->custom_opts_count].val, > > + sizeof(c->custom_opts[0].val)); >=20 > you can use "ret" here too >=20 > > + if (len < 0) > > + die("Invalid value for DHCP option %lu: %s", > > + optcode, comma + 1); > > + > > c->custom_opts[c->custom_opts_count].code =3D optcode; > > + c->custom_opts[c->custom_opts_count].len =3D 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..e1c95ad 100644 > > --- a/dhcp.c > > +++ b/dhcp.c > > @@ -33,6 +33,154 @@ > > #include "log.h" > > #include "dhcp.h" > > +/** > > + * dhcp_opt_types - Maps option code to RFC 2132 value type, indexed b= y code > > + */ > > +static const enum dhcp_opt_type dhcp_opt_types[256] =3D { > > + [1] =3D DHCP_OPT_IPV4, /* Subnet Mask */ > > + [2] =3D DHCP_OPT_INTEGER, /* Time Offset */ > > + [3] =3D DHCP_OPT_IPV4_LIST, /* Router */ > > + [4] =3D DHCP_OPT_IPV4_LIST, /* Time Server */ > > + [5] =3D DHCP_OPT_IPV4_LIST, /* Name Server */ > > + [6] =3D DHCP_OPT_IPV4_LIST, /* Domain Name Server */ > > + [7] =3D DHCP_OPT_IPV4_LIST, /* Log Server */ > > + [8] =3D DHCP_OPT_IPV4_LIST, /* Cookie Server */ > > + [9] =3D DHCP_OPT_IPV4_LIST, /* LPR Server */ > > + [10] =3D DHCP_OPT_IPV4_LIST, /* Impress Server */ > > + [11] =3D DHCP_OPT_IPV4_LIST, /* Resource Location Server */ > > + [12] =3D DHCP_OPT_STR, /* Host Name */ > > + [13] =3D DHCP_OPT_INTEGER, /* Boot File Size */ > > + [15] =3D DHCP_OPT_STR, /* Domain Name */ > > + [16] =3D DHCP_OPT_IPV4, /* Swap Server */ > > + [17] =3D DHCP_OPT_STR, /* Root Path */ > > + [19] =3D DHCP_OPT_INTEGER, /* IP Forwarding */ > > + [23] =3D DHCP_OPT_INTEGER, /* Default IP TTL */ > > + [26] =3D DHCP_OPT_INTEGER, /* Interface MTU */ > > + [28] =3D DHCP_OPT_IPV4, /* Broadcast Address */ > > + [33] =3D DHCP_OPT_IPV4_LIST, /* Static Routes */ > > + [37] =3D DHCP_OPT_INTEGER, /* TCP Default TTL */ > > + [38] =3D DHCP_OPT_INTEGER, /* TCP Keepalive Interval */ > > + [40] =3D DHCP_OPT_STR, /* NIS Domain Name */ > > + [41] =3D DHCP_OPT_IPV4_LIST, /* NIS Servers */ > > + [42] =3D DHCP_OPT_IPV4_LIST, /* NTP Servers */ > > + [44] =3D DHCP_OPT_IPV4_LIST, /* NetBIOS Name Server */ > > + [50] =3D DHCP_OPT_IPV4, /* Requested IP Address */ > > + [51] =3D DHCP_OPT_INTEGER, /* IP Address Lease Time */ > > + [53] =3D DHCP_OPT_INTEGER, /* DHCP Message Type */ > > + [54] =3D DHCP_OPT_IPV4, /* Server Identifier */ > > + [55] =3D DHCP_OPT_STR, /* Parameter Request List */ >=20 > This is a client option, I don't think we need to manage it. >=20 > > + [57] =3D DHCP_OPT_INTEGER, /* Max DHCP Message Size */ > > + [58] =3D DHCP_OPT_INTEGER, /* Renewal (T1) Time */ > > + [59] =3D DHCP_OPT_INTEGER, /* Rebinding (T2) Time */ > > + [60] =3D DHCP_OPT_STR, /* Vendor Class Identifier */ > > + [61] =3D DHCP_OPT_STR, /* Client Identifier */ >=20 > This is also a client option. >=20 > > + [66] =3D DHCP_OPT_STR, /* TFTP Server Name */ > > + [67] =3D DHCP_OPT_STR, /* Bootfile Name */ > > + [119] =3D DHCP_OPT_STR, /* Domain Search List */ >=20 > This is not really a string as this uses the message compression described > in RFC 1035, 4.1.4. (See also 3. Example in rfc3397). I'm not sure we can > encode this manually on the command line. And RFC 3396 defines how to enc= ode > search string for more than 255 characters length It's also already possible to set this via --dhcp-search. For now I think we want to exclude any options we already set ourselves (which includes 119). If we discover someone needs this in future, then we can figure out how the generic and specific ways of specifying it should interact. >=20 > > + [252] =3D DHCP_OPT_STR, /* WPAD URL */ > > +}; > > + > > +/** > > + * dhcp_opt_int_width - Integer width in bytes for INTEGER-typed optio= ns > > + */ > > +static const uint8_t dhcp_opt_int_width[256] =3D { > > + [2] =3D 4, /* Time Offset */ > > + [13] =3D 2, /* Boot File Size */ > > + [19] =3D 1, /* IP Forwarding */ > > + [23] =3D 1, /* Default IP TTL */ > > + [26] =3D 2, /* Interface MTU */ > > + [37] =3D 1, /* TCP Default TTL */ > > + [38] =3D 4, /* TCP Keepalive Interval */ > > + [51] =3D 4, /* IP Address Lease Time */ > > + [53] =3D 1, /* DHCP Message Type */ > > + [57] =3D 2, /* Max DHCP Message Size */ > > + [58] =3D 4, /* Renewal (T1) Time */ > > + [59] =3D 4, /* Rebinding (T2) Time */ > > +}; > > + > > +#define DHCP_OPT_PARSE_BUF 1024 >=20 > Do we need 1024 bytes, all options are bounded to 256 bytes. > > + > > +/** > > + * dhcp_opt_parse() - Parse a DHCP option value > > + * @code: DHCP option code > > + * @str: DHCP Value string from command line > > + * @buf: Output buffer > > + * @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 =3D dhcp_opt_types[code]; > > + char tmp[DHCP_OPT_PARSE_BUF]; > > + char *tok, *saveptr, *end; > > + struct in_addr addr; > > + unsigned long val; > > + unsigned int i; > > + uint8_t width; > > + size_t slen; > > + int len; > > + > > + switch (type) { > > + case DHCP_OPT_NONE: > > + die("Unsupported DHCP option: %u," > > + " see passt(1) for supported codes", code); > > + case DHCP_OPT_IPV4: > > + if (inet_pton(AF_INET, str, &addr) !=3D 1) > > + return -1; > > + > > + if (buf_len < sizeof(addr)) > > + return -1; > > + > > + memcpy(buf, &addr, sizeof(addr)); > > + > > + return sizeof(addr); > > + case DHCP_OPT_IPV4_LIST: > > + len =3D 0; > > + > > + if (snprintf_check(tmp, sizeof(tmp), "%s", str)) > > + return -1; > > + > > + for (tok =3D strtok_r(tmp, ",", &saveptr); tok; > > + tok =3D strtok_r(NULL, ",", &saveptr)) { > > + if (inet_pton(AF_INET, tok, &addr) !=3D 1) > > + return -1; > > + > > + if (len + (int)sizeof(addr) > (int)buf_len) > > + return -1; > > + > > + memcpy(buf + len, &addr, sizeof(addr)); > > + len +=3D sizeof(addr); > > + } > > + return len; > > + case DHCP_OPT_INTEGER: > > + width =3D dhcp_opt_int_width[code]; > > + val =3D strtoul(str, &end, 0); > > + > > + if (*end || buf_len < width) > > + return -1; > > + > > + if (width < 4 && val >=3D (1UL << (width * 8))) > > + return -1; > > + > > + for (i =3D 0; i < width; i++) > > + buf[i] =3D (val >> ((width - 1 - i) * 8)) & 0xff; > > + > > + return width; > > + case DHCP_OPT_STR: > > + slen =3D strlen(str); > > + > > + if (!slen || slen >=3D buf_len) > > + return -1; > > + > > + strncpy((char *)buf, str, buf_len); > > + > > + return slen; > > + } > > + > > + return -1; > > +} > > + > > /** > > * struct opt - DHCP option > > * @sent: Convenience flag, set while filling replies > > diff --git a/dhcp.h b/dhcp.h > > index cd50c99..3da571c 100644 > > --- a/dhcp.h > > +++ b/dhcp.h > > @@ -6,7 +6,24 @@ > > #ifndef DHCP_H > > #define DHCP_H > > +/** > > + * enum dhcp_opt_type - DHCP option value types per RFC 2132 > > + * @DHCP_OPT_NONE: Unsupported or unknown option > > + * @DHCP_OPT_STR: Variable-length string > > + * @DHCP_OPT_IPV4: Single IPv4 address > > + * @DHCP_OPT_IPV4_LIST:Multiple IPv4 addresses, comma-separated > > + * @DHCP_OPT_INTEGER: Unsigned integer (1, 2, or 4 bytes) > > + */ > > +enum dhcp_opt_type { > > + DHCP_OPT_NONE, > > + DHCP_OPT_STR, > > + DHCP_OPT_IPV4, > > + DHCP_OPT_IPV4_LIST, > > + DHCP_OPT_INTEGER, > > +}; > > + > > 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 */ > > diff --git a/passt.h b/passt.h > > index 3a0816f..751fee3 100644 > > --- a/passt.h > > +++ b/passt.h > > @@ -184,6 +184,8 @@ struct ip6_ctx { > > * @fqdn: Guest FQDN > > * @custom_opts: User-specified DHCP options from --dhcp-opt > > * @custom_opts.code: DHCP option code > > + * @custom_opts.len: Length of binary value in @val > > + * @custom_opts.val: Binary-encoded option value > > * @custom_opts.str: Original string value from command line > > * @custom_opts_count: Number of entries in @custom_opts > > * @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled > > @@ -271,6 +273,8 @@ struct ctx { > > struct { > > uint8_t code; > > + uint8_t len; > > + uint8_t val[255]; > > char str[256]; >=20 > Why do we need to keep str[] once the value is encoded into val[]? >=20 > > } custom_opts[MAX_CUSTOM_DHCP_OPTS]; > > int custom_opts_count; >=20 > Thanks, > Laurent >=20 --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --O1HEjCbG/MqKuq18 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoWXlcACgkQzQJF27ox 2Gdz0A/8Co/DDWVVVG7dy+LTVvZhp3PuWXUXCRvSNeptinl+5MNB1ZxdyTG5wOWf +E5aaF9zd6Cb0ciz6iGz3wiXWV3/gzNul6EVVLU3kgezFwA5uusE0+HcKdbY+X97 HzMVe2qtgTum6AvvkqodY/K8iSjuIASJ5cA7G+s+b7A96fImbtU4zPAlYUS5InTz TrCoSGKI5yExpwlzwwIRhosV7NbgqET+dScv0oW0GONkEA+zgZRUOex6voqJZNQ7 d9hKC6zfNhU5nkwJWt85PwlA95qq86NX7rN7ZW8BNd0ea54z7cI8sA+Srh+xzxLF bplcrpE1yOpXUxvnhNhIurQImRGmJv5ezQOpXMZvK5p3zGicUuXY1DZqVLuhiEdO teFFkhUsrKNa2vxapsr9J8lMTTHYE4x0kDac7avEC9d8kWv9uncMvNkdOhbWq/A2 UlELDw747sEjIS7lm2sN9ievw2BuiYChTBoVsbQ5qh/5W38QAw5ELBxmvMfEe36H BQ+JC/2eDOGcGvtbPIVTojo7FS/XMiL5ykSmJW1g85LmeHR/WD8sQY+XnIHK57qt gRY+S2CEB/8U5ZviB1ZSEn1RJpC0FKO+U6qa/n3OsmeoiXJtQOTiPtwRV06IYGow OsxC09f9k5bDvYaCv3627t26kIDKYfTRYE5SwEAbt3py7bF054w= =fNfX -----END PGP SIGNATURE----- --O1HEjCbG/MqKuq18--