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=CyWdAN4b; 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 6AE4B5A0262 for ; Wed, 20 May 2026 02:39:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1779237578; 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=o5sPRHfB7DhpyyiQxsy8jK0Kpt96JhJSvi4UYDPR0ik=; b=CyWdAN4bKM40CBxCdtWq+R7HaIVO4ppWn5QoAfdIYhNda0GtZYbSLtTdtH+F/KV7SOZaSi V+kXVjEcJpQ/zyI3XxkmaStHkNsOJHvqjmH+X/uxJSLXwWmxExgJe8gQIBV18WF/RnhJuf gyr3oj0tPmRg73J22J6J6rg7eJY1W9E= 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-250-UDg_hlGzNM2NIi8yi4dW8w-1; Tue, 19 May 2026 20:39:36 -0400 X-MC-Unique: UDg_hlGzNM2NIi8yi4dW8w-1 X-Mimecast-MFC-AGG-ID: UDg_hlGzNM2NIi8yi4dW8w_1779237576 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-45dbbbf8d57so5904318f8f.3 for ; Tue, 19 May 2026 17:39:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779237575; x=1779842375; 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=o5sPRHfB7DhpyyiQxsy8jK0Kpt96JhJSvi4UYDPR0ik=; b=Me+Jyf0i4QqK2HQDtcVFcfH0zv99Nz/su0diUNvcFSYheGH0x2ZmJUaTk2HUEE6vvg V9b/6ct/TGFItGhpLsGoa7VwhyJNQ0hEGn3oSxZR9sst/9MiCYFMASH6C9zYKPNEQD5a Auj2CJSZG1tnGdAoOCUf9Qg5uQq7mbhhki5B0J+SDdbCqj1vywmQaPzTLuavSVS+pbI3 2AVstCvcrg1QQl5nL9Y16q501mGlB/SaPIsh0iCsx9HDIOFZLJxnfRJtQ9SdM7Pd63DI 85N9F59zKlGH/r5kmoBRTG7qTL+ht40zSpoRBLw9rk7dur5UUvFh8bIIT+5nlMNtf2ZM u7Iw== X-Gm-Message-State: AOJu0YyX+0a/rp5XVw2vuoRE7NV/Oorr34+si3HI4tEiMLj1vgiR9h6T 0JB0QhQjUjZBP1Ztp6+66qLrr6Gjv5GwX5/VwXKrghPwMYU6xTT4/bpNGX7lC4n1CNm+uRRKSGu 1tD1Vt7nMHDhEfsNpDjAdFhnTawrejoTDoJj5K7wN6y/1lqMdZNuPkg== X-Gm-Gg: Acq92OEsaw+uaqF0TVpX2+79hZ7+Auz8QoOg/kbZHEAUUq9zva8lTA4C/jBW+cgtZ6X 2zHQE/WitP1xfJkuPcTfYLeczrq3/mUf2jjsLIsWVMlw/tKqLwzDr+gWmk3aXVeS1enK1qJTfiE aeNwyk3c5XHD89bfeoT4UWtbaldmt/gpIdoEe9qJRjyGVJfAj46xcxn6CaJPS0dsCtorOz2ckaL aK+Sn/LxRUXZ3+jQYwL9PgAdc2vR09qb1sUrkaTreoBBlyWnKgvzofUDTY3QpNMI58/c6fwowHb PUgcl4eAP0Zpg/Mor+J+P6xDIVTRbQWsg9Av1Ebh2Yh6t8ZtvKD0QOAB6msb/AYcTBM4vfcpFyt m1x+2DLcE9c8rF23G8PRN1knnRV4R+QTJ X-Received: by 2002:a05:6000:2902:b0:43f:ea25:20ff with SMTP id ffacd0b85a97d-45e5c594d5dmr33930369f8f.29.1779237575545; Tue, 19 May 2026 17:39:35 -0700 (PDT) X-Received: by 2002:a05:6000:2902:b0:43f:ea25:20ff with SMTP id ffacd0b85a97d-45e5c594d5dmr33930340f8f.29.1779237575098; Tue, 19 May 2026 17:39:35 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45da0a1a22csm48842222f8f.19.2026.05.19.17.39.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 May 2026 17:39:34 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH 3/6] dhcp: Add option type table and value parser Message-ID: <20260520023932.6a7e3537@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 02:39:33 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: InAVUmaxCttXdGWldGVJme1FJy66iRpHpRtPa47uIYM_1779237576 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: KP34J4WFJBGENPQEYVCVMZSXYRORJYSJ X-Message-ID-Hash: KP34J4WFJBGENPQEYVCVMZSXYRORJYSJ 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; > +} 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