From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SfJUKd4Q; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 91CA55A0262 for ; Wed, 20 May 2026 23:23:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1779312229; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NuHzJYu30rpUl6cgxxbuqjW0RVzo3ROXLWJHnf9ow5s=; b=SfJUKd4QsPKeEUTKvy0gyL8Or1E8qjJYuGnVAVuitgjzlTz30komStW6eF+PK6+Vdwe0ZO XlkB37qyT0Uf/EorwjRoYQXKgWkthk+l1zPd7yCUxN9lSRALxKy5+cEfdWVbBYGFuuTUBE sKX2exJ0bSdVR6OSy9bzGvKJuGNjJ7o= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-690-CH2HPWD_Ozmq7jc-_e_PeA-1; Wed, 20 May 2026 17:23:47 -0400 X-MC-Unique: CH2HPWD_Ozmq7jc-_e_PeA-1 X-Mimecast-MFC-AGG-ID: CH2HPWD_Ozmq7jc-_e_PeA_1779312226 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-45e81291d62so3709220f8f.1 for ; Wed, 20 May 2026 14:23:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779312226; x=1779917026; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=NuHzJYu30rpUl6cgxxbuqjW0RVzo3ROXLWJHnf9ow5s=; b=MCcy82DigCzOj2JPoG85RhWg2cYLKcKSOZft2fqjieBpT0HsNu5GlYSiXUYINh/0ta pqT3v15hhsao3WvuDIAI/bKrwqIhLaROkYAaFWgXiYvgd2iS+rJSCUfH5nbYEaird4XV 5k7e3rqjk+SBDTKCddEpPy2iAF+CWKI+e8pBDY+BXIFtgJXzLCWmnUG75puEt9tJkDho NCtgKu14Im4H9rkeP2L8IObjw4f9ovRBW73R5H3AMzxfcIqRXoCLhPul0KErbq4F3mNN tnbC973G4qNJyqmCEAVFmG3LRyH28pEizATAyQEgQGL3+tTj4AVTKVOVR8JizIr+i2rp yjtg== X-Gm-Message-State: AOJu0Yzi2Frm6ueWMHqGoifgpGo7E1mnlUg9VBaGL3KD0cgfvZluhQS9 q0XWL3KtvhAci4kcxvMX/qSqBmvZlO+/WtSptbNHs98VkyMMDnYi25afGvAHlI2B2Fs1TBSQaRf BpoptiOqt5mjxfQSgpxrpim4HfccwyDLnfyp1TOWU0TkjVaNmPHxidA== X-Gm-Gg: Acq92OEho3WRGSIMTVsv+ORVSlj9MChdjoEjjnI4ApN8Y54sTLzdzakVx6cBCjBaHUc NTPG1l7c5oZ4fBCrg22jAMT3naSSct0tw3GEZDp+ePHItdXZF5pMz9UfdMrNf0WLiZu269N3/Wm 9sBWnUT/CV7N7n04Oyf1aWPrkHR49I6A3/806nUKOSOXwEoqertkeWJZytZZc5Kh+q0kOFxbhyY sTGoxfgu4e/ZVKelu/295g8rtOF3EsH8LlOB0I3Tin4jMxmnoDUP1nj6Osn9mWX7anBoWoWhsgm j1V55m7noLClq9ZCfUaY2bgZcwNC0258nOr/lthZPPobWmueOZ2wmk8cGY9cCOtesjIFPzPQD/4 qqZ5JjFDynxdQR4ZUF0z73VK3CAZKEmPhrATBaROr2ntXKBswCA== X-Received: by 2002:a05:600c:3056:b0:490:18cb:e820 with SMTP id 5b1f17b1804b1-490360a17e2mr275225e9.21.1779312225641; Wed, 20 May 2026 14:23:45 -0700 (PDT) X-Received: by 2002:a05:600c:3056:b0:490:18cb:e820 with SMTP id 5b1f17b1804b1-490360a17e2mr274965e9.21.1779312225062; Wed, 20 May 2026 14:23:45 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45da15a666fsm58615315f8f.36.2026.05.20.14.23.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 May 2026 14:23:44 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH 3/6] dhcp: Add option type table and value parser Message-ID: <20260520232339.51b5915e@elisabeth> In-Reply-To: <20260518132002.418296-4-anskuma@redhat.com> References: <20260518132002.418296-1-anskuma@redhat.com> <20260518132002.418296-2-anskuma@redhat.com> <20260518132002.418296-3-anskuma@redhat.com> <20260518132002.418296-4-anskuma@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 20 May 2026 23:23:43 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: MorIAXuFwytDFqfp-Jf3NHIWC5_vYggdP9b79EALNfc_1779312226 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: WWLUXBZ4SMIYTPQII7MQG23TMOMG36GB X-Message-ID-Hash: WWLUXBZ4SMIYTPQII7MQG23TMOMG36GB X-MailFrom: sbrivio@redhat.com 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: passt-dev@passt.top, lvivier@redhat.com, jmaloy@redhat.com, david@gibson.dropbear.id.au 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: On Mon, 18 May 2026 18:49:59 +0530 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. > > 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 > --- > 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; > +} > + > +/** > + * 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); > + > + switch (type) { > + case DHCP_OPT_NONE: { We don't need to add a block here, and... > + die("Unsupported DHCP option: %u," > + " see passt(1) for supported codes", > + code); > + } > + case DHCP_OPT_IPV4: { > + struct in_addr addr; here I think it would be preferable to stick to what we do (almost) everywhere else and just have the variable declarations before the switch, and drop all those curly brackets. I think it's more readable. See also for example that: } } at the end. > + > + if (inet_pton(AF_INET, str, &addr) != 1) > + return -1; > + if (buf_len < sizeof(addr)) > + return -1; > + memcpy(buf, &addr, sizeof(addr)); Not strictly enforced, but we usually add an extra newline before return statements, to make those a bit more separated / visible. > + 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" I don't think the example is particularly fitting or explaining the kind of madness RFC 3442 gifted us with. I would rather pick something like 192.168.2.0/28 as subnet and 192.168.2.1 (RFC 3442 doesn't seem to care about using appropriate IP addresses reserved for documentation), and there our "destination descriptor" would be: 28.192.168.2 hint: 'sipcalc' / 'ipcalc' are pretty useful to visualise this stuff: $ sipcalc 192.168.2.0/28 -[ipv4 : 192.168.2.0/28] - 0 [CIDR] Host address - 192.168.2.0 Host address (decimal) - 3232236032 Host address (hex) - C0A80200 Network address - 192.168.2.0 Network mask - 255.255.255.240 Network mask (bits) - 28 Network mask (hex) - FFFFFFF0 Broadcast address - 192.168.2.15 Cisco wildcard - 0.0.0.15 Addresses in network - 16 Network range - 192.168.2.0 - 192.168.2.15 Usable range - 192.168.2.1 - 192.168.2.14 - $ ipcalc 192.168.2.0/28 Address: 192.168.2.0 11000000.10101000.00000010.0000 0000 Netmask: 255.255.255.240 = 28 11111111.11111111.11111111.1111 0000 Wildcard: 0.0.0.15 00000000.00000000.00000000.0000 1111 => Network: 192.168.2.0/28 11000000.10101000.00000010.0000 0000 HostMin: 192.168.2.1 11000000.10101000.00000010.0000 0001 HostMax: 192.168.2.14 11000000.10101000.00000010.0000 1110 Broadcast: 192.168.2.15 11000000.10101000.00000010.0000 1111 Hosts/Net: 14 Class C, Private Internet > + */ > + 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'; It looks relatively sane until here (as sane as RFC 3442 permits, of course), but then, I think the calculation of the "destination descriptor", below, would deserve its own function. > + mask = strtoul(slash + 1, NULL, 10); > + if (mask > 32) > + return -1; > + > + if (inet_pton(AF_INET, comma + 1, &gw) != 1) > + return -1; This part could happily live here instead. > + sig_octets = (mask + 7) / 8; And this should be ROUND_UP() (see common.h). > + > + 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 > + */ > +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