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=CR+HKFhv; 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 8248A5A0265 for ; Fri, 12 Jun 2026 01:04:34 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781219073; 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=76J/ztaUtr079DxL4+uxNyIwmPH8jOxCjt/oOKxEjFs=; b=CR+HKFhvba5aBn+KFEVP+G8dcSVAlcIPZWr3MadEjEbSVXcp0xShzLJlA3Xu3wO5mEJSKV j/mxkFHJH7XjKIxgbupX+QbSM9F1JlPvo9ds6wqDNyNq4w2bD+JDTlZP34YB/ocSgcCwgQ tOrjqWpxHv9Fib1W/96e1BO2PkuHfiM= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-677-MnmpbrJLO4i43ylG_kYTIw-1; Thu, 11 Jun 2026 19:04:31 -0400 X-MC-Unique: MnmpbrJLO4i43ylG_kYTIw-1 X-Mimecast-MFC-AGG-ID: MnmpbrJLO4i43ylG_kYTIw_1781219070 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-490d3f03883so2523135e9.1 for ; Thu, 11 Jun 2026 16:04:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781219070; x=1781823870; 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=Qfy+wG4JLBlTO0t4YSyinQUfaMgYs3EydC+O1U8555c=; b=XiDOXxSIEkFalze99yjxvyDgirjM8ndzR8LvRpjmXdRU8pUo3L9sNAJNoTNuomZ9Bi FLZBg8GhXwzxn4jCpZBm/6F9d60lzpxEaNHoOT1qiVhZgbNgvNZ+1Vy26rNTjMTJN8iE cPRASoK9KyUWodbxlFaUvv9hBHoL6v58CiQQ5JSDnUk546nnCzaePBdpwls2V0ZFnS/Q kSV4vMzEwTGDkhsic4r8DUvBIQaHm66K4XdiSKnytT3oV2zhWx4c1bgyh9Rp8LDIOy4u yu5AKr13/0Dk7y+ayzSavNJN8Xf/dtDocru+++8WFYtxBn7JdkgaiaUixWwlF/yPp4IE RDCw== X-Gm-Message-State: AOJu0YxSyARb/xOViGr7gdh+10M4eCCmx2843Smw2MOK52qMX7qDhKJo c6G5D1M2qiv89ur6xSWawTVmJJIgwJPpmH9GE0SxsxuOESwkY4H191VAE70ReK/ukjF0YuH1kAW B/m6MvfPhroQ7Nwk6Urd5//lDH8GJYwLR0aUj29rabNEY9mBcSkG5Ig== X-Gm-Gg: Acq92OEAGVU3RWgOhHP6Sqys11vpzZMZFi6xuW21taOPwHiv0UZzPeJ1ZFu6ugLqVAQ oecweR2yrRYLoBdrwRDFSG7Bno2piOLsn+SgFntywT9gU7ZU1Wjs+Br+jCPzzWgYUB4U9G0UIqZ t/gZRGf3U45z2Oqi45IUwflWR38qojtOaPDfCjQoza07lqCTAz9bvsfWX07osR8nn5+7sRWxTma lYFfMDdlh58c7Fa7p8ZBwovRz1ckVFZ/ZhSHLWmbgkly/L6I8uz/D4ci6R7QNIEAQO51TdiiToI 4qTVtR8tPNwyzoBtbZMJtcXukOKkPZud9U4VfwV78wb/PgO7e8rUWWDNJMg3tqnpUu6f5NLBIFI r3JLN44JpP/nGFdLRJX6ovoNX99SYkLKa X-Received: by 2002:a05:600c:4e08:b0:490:abef:dae6 with SMTP id 5b1f17b1804b1-490ec4e753bmr734335e9.19.1781219069980; Thu, 11 Jun 2026 16:04:29 -0700 (PDT) X-Received: by 2002:a05:600c:4e08:b0:490:abef:dae6 with SMTP id 5b1f17b1804b1-490ec4e753bmr733845e9.19.1781219069337; Thu, 11 Jun 2026 16:04:29 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-490ea9520f9sm9284515e9.1.2026.06.11.16.04.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Jun 2026 16:04:28 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v3 1/6] conf: Add --dhcp-opt command-line option Message-ID: <20260612010426.319bc57d@elisabeth> In-Reply-To: <20260601073758.1571317-2-anskuma@redhat.com> References: <20260601073758.1571317-1-anskuma@redhat.com> <20260601073758.1571317-2-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: Fri, 12 Jun 2026 01:04:28 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: ed_lijmycf8VJbwKTSecVEuZnHeyvRpnZO995Ze3tcI_1781219070 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: ZLZE2L3NJIP6TNTMWJ54SZVNWTCUYYKJ X-Message-ID-Hash: ZLZE2L3NJIP6TNTMWJ54SZVNWTCUYYKJ 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, david@gibson.dropbear.id.au, jmaloy@redhat.com, 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: Sorry for the late review. I have a few remarks on top of the one from David (which is the only one left that's really critical, I guess): On Mon, 1 Jun 2026 13:07:51 +0530 Anshu Kumari wrote: > Introduce the --dhcp-opt flag that allows setting arbitrary DHCP > options from command-line in the form of [Option CODE,VALUE]. > This patch adds the option storage in struct ctx and CLI parsing; > the type-aware value parser and DHCP reply injection follow > in subsequent patches. This split makes the patch smaller, but not necessarily easier to review, or maybe actually harder: - as David noted, it would be preferable to have man page changes together with functional changes they relate to, but not just for correctness: I personally use those during reviews to double check if the implementation corresponds to the intention. That is, in some sense, I use man pages as specification, which is particularly fitting when it comes to new command line options. So, from my side, while it's not really a blocker, my preference to have those man page changes together with the patch implementing the code changes is probably a bit stronger than the one expressed by David. It's not just for review right now, it also helps investigation later when somebody finds issues: having a more comprehensive description in this patch itself means not having to correlate multiple patches. Think of bisecting an issue and finding that this patch breaks something: git reaches this patch, and now you don't have the man page in the checked out tree at all... - I've been asking myself: is dhcp_add_option() matter for conf.c, rather than for dhcp.c? That is, is it merely a matter of configuration parsing / handling, or a part of the DHCP implementation proper? I needed to reach 3/6 and actually grasp 3/6 to answer this question... which seems to be a good indication that this change belongs to the same patch as 3/6. And I had similar fundamental questions around this patch that I could only resolve by reading 3/6. For me, a more reasonable split would have been something like: - 1/4 dhcp: Refactor fill_one() to operate on a generic buffer it's a dependency for 3/4 but doesn't depend on others - 2/4 dhcp: Add option overload also a dependency of 3/4, it only depends on 1/4 anyway - 3/4 dhcp: Add --dhcp-opt with option table and value parser the main feature, with man page for --dhcp-opt, and a clear match between what you parse from conf() and how it's used - 4/4 conf: Add --dhcp-boot command-line option with its part of man page, as it depends on 3/4 but no other bits seem to depend on it ...I'm not sure how much effort that is at this point, but I think it would be nice for the revision history (current review, doesn't matter so much, as both David and myself are now familiar with it). Some nits below: > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > Signed-off-by: Anshu Kumari > --- > v3: > - Added dhcp_add_option() helper in dhcp.c for storing options with > duplicate-code detection. > - case 33 now calls dhcp_add_option() instead of inline storage. >=20 > v2: > - Added kerneldoc for @custom_opts, @custom_opts.code, @custom_opts.str= , and @custom_opts_count in struct ctx > - Removed len and val[255] fields from struct (moved to patch 3) > - Removed braces from case 33, moved declarations (optcode, comma, end)= to function scope > - Renamed code =E2=86=92 optcode to follow function-scope convention. > --- > conf.c | 24 +++++++++++++++++++++++- > dhcp.c | 38 ++++++++++++++++++++++++++++++++++++++ > dhcp.h | 1 + > passt.h | 12 ++++++++++++ > 4 files changed, 74 insertions(+), 1 deletion(-) >=20 > diff --git a/conf.c b/conf.c > index 029b9c7..ce78af1 100644 > --- a/conf.c > +++ b/conf.c > @@ -47,6 +47,7 @@ > #include "lineread.h" > #include "isolation.h" > #include "log.h" > +#include "dhcp.h" > #include "vhost_user.h" > #include "epoll_ctl.h" > #include "conf.h" > @@ -616,7 +617,8 @@ static void usage(const char *name, FILE *f, int stat= us) > =09=09" -S, --search LIST=09Space-separated list, search domains\n" > =09=09" a single, empty option disables the DNS search list\n" > =09=09" -H, --hostname NAME =09Hostname to configure client with\n" > -=09=09" --fqdn NAME=09=09FQDN to configure client with\n"); > +=09=09" --fqdn NAME=09=09FQDN to configure client with\n" > +=09=09" --dhcp-opt CODE,VAL=09Set DHCP option by code\n"); What's VAL? It becomes obvious after some thinking but perhaps this would be clearer: =09=09" --dhcp-opt CODE,VAL=09Set DHCP option CODE to VAL\n"); > =09if (strstr(name, "pasta")) > =09=09FPRINTF(f, " default: don't use any search list\n"); > =09else > @@ -844,6 +846,10 @@ static void conf_print(const struct ctx *c) > =09=09=09info(" router: %s", > =09=09=09 inet_ntop(AF_INET, &c->ip4.guest_gw, > =09=09=09=09 buf, sizeof(buf))); > +=09=09=09for (i =3D 0; i < c->custom_opts_count; i++) > +=09=09=09=09info(" option %u: %s", > +=09=09=09=09 c->custom_opts[i].code, > +=09=09=09=09 c->custom_opts[i].str); > =09=09} > =20 > =09=09for (i =3D 0; i < ARRAY_SIZE(c->ip4.dns); i++) { > @@ -1233,6 +1239,7 @@ void conf(struct ctx *c, int argc, char **argv) > =09=09{"migrate-no-linger", no_argument,=09NULL,=09=0930 }, > =09=09{"stats", required_argument,=09=09NULL,=09=0931 }, > =09=09{"conf-path",=09required_argument,=09NULL,=09=09'c' }, > +=09=09{"dhcp-opt", required_argument,=09=09NULL,=09=0933 }, > =09=09{ 0 }, > =09}; > =09const char *optstring =3D "+dqfel:hs:c:F:I:p:P:m:a:n:M:g:i:o:D:S:H:46= 1t:u:T:U:"; > @@ -1248,10 +1255,13 @@ void conf(struct ctx *c, int argc, char **argv) > =09uint8_t prefix_len_from_opt =3D 0; > =09unsigned int ifi4 =3D 0, ifi6 =3D 0; > =09const char *logfile =3D NULL; > +=09unsigned long optcode; > =09char *runas =3D NULL; > =09size_t logsize =3D 0; > +=09const char *comma; > =09long fd_tap_opt; > =09int name, ret; > +=09char *end; > =09uid_t uid; > =09gid_t gid; > =09 > @@ -1465,6 +1475,18 @@ void conf(struct ctx *c, int argc, char **argv) > =09=09=09=09die("Can't display statistics if not running in foreground")= ; > =09=09=09c->stats =3D strtol(optarg, NULL, 0); > =09=09=09break; > +=09=09case 33: > +=09=09=09comma =3D strchr(optarg, ','); > +=09=09=09if (!comma) > +=09=09=09=09die("--dhcp-opt requires CODE,VALUE format"); > + > +=09=09=09optcode =3D strtoul(optarg, &end, 0); > +=09=09=09if (end !=3D comma || optcode < 1 || optcode > 254) > +=09=09=09=09die("DHCP option code must be 1-254: %s", > +=09=09=09=09 optarg); > + > +=09=09=09dhcp_add_option(c, optcode, comma + 1); > +=09=09=09break; > =09=09case 'd': > =09=09=09c->debug =3D 1; > =09=09=09c->quiet =3D 0; > diff --git a/dhcp.c b/dhcp.c > index 1ff8cba..c5fbf37 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -33,6 +33,44 @@ > #include "log.h" > #include "dhcp.h" > =20 > + > +/** > + * dhcp_add_option() - Add or update a custom DHCP option It's not clear where it's added, which is rather fundamental (to a reply message or to the configuration?) "Set" can replace "Add or update", and "custom" is not really important or well defined I think (what makes an option custom? The fact that it's not assigned by IANA or the fact that it's specified by the user? But then what's not custom...?). So, maybe... "Set value for a DHCP option in configuration"? And looking at this, I guess conf_dhcp_option() in conf.c would be a better fit. > + * @c:=09=09Execution context > + * @code:=09DHCP option code > + * @val_str:=09Value string from command line @str would be equally expressive I think, but I'm fine with both (slightly less typing for 'str' though). > + * > + * If @code was already added, the previous value is overwritten. > + * Calls die() on any error. > + * > + * Return: 0 on success > + */ > +int dhcp_add_option(struct ctx *c, uint8_t code, const char *val_str) > +{ > +=09int idx; > + > +=09for (idx =3D 0; idx < c->custom_opts_count; idx++) { > +=09=09if (c->custom_opts[idx].code =3D=3D code) > +=09=09=09break; > +=09} > + > +=09if (idx =3D=3D c->custom_opts_count) { > +=09=09if (c->custom_opts_count >=3D MAX_CUSTOM_DHCP_OPTS) > +=09=09=09die("Too many --dhcp-opt entries (max %d)", > +=09=09=09 MAX_CUSTOM_DHCP_OPTS); What's the rationale of this, though? You now have option overload and a ("working") table with 256 slots, one might actually try to set more than 32 options I think. If not, showing the calculation you made for MAX_CUSTOM_DHCP_OPTS in its #define directive would make that clear. If you're trying to save some memory in struct ctx by storing only 32 entries in the configuration, note that, as it's now moving to the data segment (see Jon's "[PATCH v3] util, passt: Close daemon-lifetime fds on exit to avoid Coverity warning") as long as you have a block of zeroed bytes that's large enough to matter, that's going to take no memory at all. > +=09=09c->custom_opts_count++; > +=09} > + > +=09c->custom_opts[idx].code =3D code; > + > +=09if (snprintf_check(c->custom_opts[idx].str, > +=09=09=09 sizeof(c->custom_opts[0].str), > +=09=09=09 "%s", val_str)) > +=09=09die("DHCP option value too long: %s", val_str); > + > +=09return 0; > +} > + > /** > * struct opt - DHCP option > * @sent:=09Convenience flag, set while filling replies > diff --git a/dhcp.h b/dhcp.h > index cd50c99..9c8f1e3 100644 > --- a/dhcp.h > +++ b/dhcp.h > @@ -8,5 +8,6 @@ > =20 > int dhcp(const struct ctx *c, struct iov_tail *data); > void dhcp_init(void); > +int dhcp_add_option(struct ctx *c, uint8_t code, const char *val_str); > =20 > #endif /* DHCP_H */ > diff --git a/passt.h b/passt.h > index 1726965..3a0816f 100644 > --- a/passt.h > +++ b/passt.h > @@ -182,6 +182,10 @@ struct ip6_ctx { > * @dns_search:=09=09DNS search list > * @hostname:=09=09Guest hostname > * @fqdn:=09=09Guest FQDN > + * @custom_opts:=09User-specified DHCP options from --dhcp-opt I think this should be called @dhcp_opts, because @custom_opts in the... context of ctx isn't really clear. They're all custom anyway in some sense. > + * @custom_opts.code:=09DHCP option code > + * @custom_opts.str:=09Original string value from command line It's the only one, there isn't one that's original and one that isn't (right?). > + * @custom_opts_count:=09Number of entries in @custom_opts And if you allow for 256 items here, you would skip this and the whole complexity. > * @ifi6:=09=09Template interface for IPv6, -1: none, 0: IPv6 disabled > * @ip6:=09=09IPv6 configuration > * @pasta_ifn:=09=09Name of namespace interface for pasta > @@ -263,6 +267,14 @@ struct ctx { > =09char hostname[PASST_MAXDNAME]; > =09char fqdn[PASST_MAXDNAME]; > =20 > +#define MAX_CUSTOM_DHCP_OPTS=0932 > + > +=09struct { > +=09=09uint8_t code; > +=09=09char str[256]; This should be 255 characters long, otherwise it might suggest to others that it's NULL-terminated, which is a rather dangerous assumption. > +=09} custom_opts[MAX_CUSTOM_DHCP_OPTS]; > +=09int custom_opts_count; > + > =09int ifi6; > =09struct ip6_ctx ip6; > =20 --=20 Stefano