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=jI9YBMky; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id E0FC65A0269 for ; Thu, 08 Oct 2026 00:06:22 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1791410781; 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=Nr45qbWHVoQHyygdgpFlm2GvXW4IEDQBqjrH3BIZQkE=; b=jI9YBMkyZNO/3FO1xezr/JMijzq1skNCdqX67/Nwr0qWcEWZAElhOSbWMn/gHgyWC3SZU1 VaQcizopYJ9+jIDbIi4aWMrrC2pL/PY3gJ4xCpa0uWuVjnTtVH7aqsWWSTg3Aqx/SZi4BW fdXQscpTvROXcvjMqGG/gb8LnPliy3w= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-561-uhgnZSpAOQe5nQrWgPpXqg-1; Wed, 07 Oct 2026 18:06:20 -0400 X-MC-Unique: uhgnZSpAOQe5nQrWgPpXqg-1 X-Mimecast-MFC-AGG-ID: uhgnZSpAOQe5nQrWgPpXqg_1791410779 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-48afcecb035so21094f8f.0 for ; Wed, 07 Oct 2026 15:06:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1791410779; x=1792015579; h=date:content-transfer-encoding:content-type: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:content-type; bh=Nr45qbWHVoQHyygdgpFlm2GvXW4IEDQBqjrH3BIZQkE=; b=jkV6LZCCE9K7sN1X/YnDorq5RIjxXnhh7n7GfhN4tBGoHnGDmOJh5yVd8M47CorSwN 4WSu7fZcJfxlFae7dGCqfCn/ljfeYk8UvtiUoRsm93NG/MQNz9O/J53hsOTF5KwTlnFI 8CzhHh1zzNr735KY0y+51u1KQ0zZCx7Ew0noJ11VI1XJWczD+pmVDFFV1LCvXWFXdKq8 S6Oio43bYO+XhiM8r5BDFSiQVMOQzKHJ10llrqtJ5Pqdn271Gfslt1cAIIQ14jkLAVuE gwo/cqzTFsSzwoI9YZMn5DiU8h1hKM2tOdnnW2mxKphfXrDEy5dwcmwqqm5uCdOSrAZQ eqTw== X-Gm-Message-State: AFq9FYKwOaenaxfG0uo0UErPOtTO8nkX2aNQYqVhzdGKxuj6gR9Oz/Gz +344eu8o0PU2Bdukc0jN2Lr76kta/9nBI6bduHz99WtDjo3Z0/DX3DZidaGFamJm8t1vdr5/tQb apq53fQhU77tOH1P7jWARZ7bJWt6DS0J7ZFeYj+ri+3oNTRB5YK9vM6X+Imt2bmzhKZtqAuLigL EM+13TlQxS8Cwf6tqld/W4sQfQVcWNJDINe6CO X-Gm-Gg: AYBFou3mZOaw/CLFAGR8CKJxpmtNsrBfbKrGdMeb1+d44tWskQc8h/WGcZQrdYUcRq7 ejbdGT7skb+TycarpBKHCRWC+jUj+kmzbbQu3t+xbB8hUEEXkMtsw4O4ZD+Xr+99vy8QjY5wE/g beZY5o4VzMfkRuMSf1Of7KtOUeflTJza1u6JsKIu4uP77HKx22TkfFO1EV2USsKWL0KgAW1F4m0 2xHzka7pLolqxky+r6MQi6eBldY3XPiaPhNKVMg/GWp6nG05ABVr7XHzKVBoBkJFqJz/fU1kspG QYqHFy1MI2RzOgGd108M+mXI3WeRVOHVTxZZ3j/PVLONeWhb/VMwZ2Gh/UADCPKxV3qEaqCfA3n Ku7/N05Uwag== X-Received: by 2002:a05:6000:40ce:b0:48c:58b9:f17c with SMTP id ffacd0b85a97d-48c7f101d2dmr975401f8f.12.1791410779261; Wed, 07 Oct 2026 15:06:19 -0700 (PDT) X-Received: by 2002:a05:6000:40ce:b0:48c:58b9:f17c with SMTP id ffacd0b85a97d-48c7f101d2dmr975350f8f.12.1791410778690; Wed, 07 Oct 2026 15:06:18 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-48c71d2d563sm7343203f8f.41.2026.10.07.15.06.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Oct 2026 15:06:18 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v7 3/7] dhcp: Add --dhcp-opt with option table and value parser Message-ID: <20261008000617.0f8f22a9@elisabeth> In-Reply-To: <20261001131602.653553-4-anskuma@redhat.com> References: <20261001131602.653553-1-anskuma@redhat.com> <20261001131602.653553-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: Thu, 08 Oct 2026 00:06:17 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: qtF3U6UDdQuBMVUo8_xXktHJ4nq7KkXaoo4myUWKHu0_1791410779 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: MC2AU2VFHYVG4KZHMKG2BVVSIFHZUIKM X-Message-ID-Hash: MC2AU2VFHYVG4KZHMKG2BVVSIFHZUIKM 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 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 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