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=UHDGC1e2; 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 0CE2F5A0269 for ; Mon, 27 Jul 2026 08:19:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1785133157; 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=I0rKkWrk4JIh/uQnK6fm55es0ehnbIrwb8xls3+/LbI=; b=UHDGC1e2p+N+ML3iDDtF7rqvUdnmI0QVCkvlzNjslAoMJtTGfjxG3xR9F3z6ebBgCf8qD9 gUc0PJh8YCWC2yG5gFAqVuDVHhut+PPQ1ZvqQJWIPCCuZ8y9M/RrfCje/Jn90OSWwK6vrq Dm5OjkLTPFgJWiXWLZoaVCQ3GQU7Mdg= 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-214-lXp3AQAROxKy-aUU_JVCWw-1; Mon, 27 Jul 2026 02:19:14 -0400 X-MC-Unique: lXp3AQAROxKy-aUU_JVCWw-1 X-Mimecast-MFC-AGG-ID: lXp3AQAROxKy-aUU_JVCWw_1785133153 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-47f9ae25143so1948116f8f.2 for ; Sun, 26 Jul 2026 23:19:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1785133153; x=1785737953; 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=zjEfBCzgEpNFECMBOZ+VPcGwyZrkmlgItYFmBSQ3Edg=; b=lYEMkCkYkjifePYxad2ab6UyVkHS66kVm99gSwItcvUBeYUBSvclWoq3bY/9nDh1og WCiGHbH44ARmJGZ0IVyNl/wQYmiEeAkne/1tfuKTvjg+DSkHRvcpNRosz/K8A60/YNPV mCTSgv24M9ZadBQ9bp7MiPWsUEASK5NEE1FUC/msEXv/MJJnDVNy3sp72TZgDuofjW09 BZ1DRwrVAqQLOkFNIAERMiscWZueruj76FTtvwSoPUtVS9uhYwVYKtKn7abf1A/uX4qm NLYTJlsuu19DY8lRqB0/BSllm3H3+fIpxA3I1tbYLfjn/qs0T3DCtV/X1AzRyP0oZudK ws1w== X-Gm-Message-State: AOJu0YySlmuMeQUK8c74fzUbmOSK2oFTEFW65G6ilgsB0yfSiwn7q0SE lJlanVpMEcEd+ti9U1iFM3A39fI/gVrFR60cSpKGmRb5G8k7RPVG4rSxGSCSW/H2G511Zwcywkz lK/kdzbnUDOA6941mefs8ZPUTaDeSOGhmM5Luqwc5QlXx5tO2/8lMFg== X-Gm-Gg: AR+sD12i4uNtY0S/9i31Cbfbuh7Ioo1w3P+gmjJtoQwmE40O5engndM4+tQ4Ib+DNbW 4rHvtlqHHltaWHBxeT2zPPuzau1azDHQPTDArFNkEtjm/CgUKWrFXZzW+YOhL0grWsgVmPYp3hV uGpt65nLNUjAwiSPLGzUhgTBLoEiZOj3/dG8nkaZ9ndAq9WGJO1/Ft2o9oTGZW6qSTgJWS5f/+K LaDZITjNcNaauylqnLEXs5fhM324SYCeo1KflZoRW+E2yKZ/mPDaZDw3Pa0X2LtAZGTTWwQUcCx ec/r8cc5b2Wkes3R/3snye8qcvwPZ1UTS9p6WIYDpwURd/FUpnAXsexTTNUpV03G72Z4RvHa/b1 POof2qJ/6BANZL6yCjb7cWCvUeo+X X-Received: by 2002:a05:6000:40ce:b0:47f:9311:34cf with SMTP id ffacd0b85a97d-47f9fc94771mr9058059f8f.22.1785133152679; Sun, 26 Jul 2026 23:19:12 -0700 (PDT) X-Received: by 2002:a05:6000:40ce:b0:47f:9311:34cf with SMTP id ffacd0b85a97d-47f9fc94771mr9058009f8f.22.1785133152092; Sun, 26 Jul 2026 23:19:12 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47f96d4656bsm29111193f8f.21.2026.07.26.23.19.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 26 Jul 2026 23:19:11 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v5 3/7] dhcp: Add option overload Message-ID: <20260727081910.1653e621@elisabeth> In-Reply-To: <20260717175648.879152-4-anskuma@redhat.com> References: <20260717175648.879152-1-anskuma@redhat.com> <20260717175648.879152-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: Mon, 27 Jul 2026 08:19:11 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: FOFgzKK9tfVZ9X1Q2_PGdIyr_eu4sHYH0OJaWrG9Hgw_1785133153 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: BUZ75IX53F2WP5G3FOU7AQ4WDTBHPBGR X-Message-ID-Hash: BUZ75IX53F2WP5G3FOU7AQ4WDTBHPBGR 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, david@gibson.dropbear.id.au, 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: On Fri, 17 Jul 2026 23:26:40 +0530 Anshu Kumari wrote: > When the options field is full, overflow remaining DHCP options into > the sname and file fields per RFC 2132 option 52. >=20 > Per RFC 2132, Section 9.5, the boot file name is always placed in the > 'file' header field. When a boot file is set, the file field is > reserved from overload and overflow uses only the sname field. >=20 > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > Signed-off-by: Anshu Kumari > --- > v5: > - enhanced enum dhcp_overload to follow kernel-doc. > - Inline fill_overflow() into fill() > - Use state-based checks instead of slen >=20 > v4: > - Converted overload #defines to enum dhcp_overload. > - Fixed missing whitespace in comment before */. > - Boot file name always placed in 'file' header field per RFC 2132, > Section 9.5; file field reserved from overload when bootfile is > set; option 67 suppressed from options area. >=20 > v3: > - Added RFC 2132 Section 9.3 reference comment on overload > constants. > - Use ARRAY_SIZE(opts) instead of raw 255 in fill_overflow(). > - Swapped overflow order: try sname (64 bytes) first, then file > (128 bytes) =E2=80=94 better packing and keeps file field available f= or > boot file name. > - Removed '&' from &reply.file. > - Removed '+1' from memcpy =E2=80=94 reply.file already zeroed. > - opt_set_dns_search() max_len: OPT_MAX - 3 instead of > sizeof(m->o). >=20 > v2: > - Added #define DHCP_OVERLOAD_FILE and #define DHCP_OVERLOAD_SNAME cons= tants > - Added comment documenting space reservation: /* Reserve 3 bytes for o= ption 52 */ > - Fixed DNS search length: sizeof(m->o) only, not combined with file+sn= ame > - Removed dhcp_boot references =E2=80=94 reply.file copy now reads from= opts[67] > - Used DHCP_OVERLOAD_FILE constant in reply.file guard >=20 > --- > dhcp.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 79 insertions(+), 9 deletions(-) >=20 > diff --git a/dhcp.c b/dhcp.c > index e5d89fc..39f7952 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -176,13 +176,31 @@ static bool fill_one(uint8_t *buf, size_t size, int= o, int *offset) > } > =20 > /** > - * fill() - Fill options in message > - * @m:=09=09Message to fill > +* enum dhcp_overload - DHCP option overload values (RFC 2132, Section 9.= 3) > + * @DHCP_OVERLOAD_NONE:=09=09No overload > + * @DHCP_OVERLOAD_FILE:=09=09file field carries options > + * @DHCP_OVERLOAD_SNAME:=09sname field carries options > + */ > +enum dhcp_overload { > +=09DHCP_OVERLOAD_NONE, > +=09DHCP_OVERLOAD_FILE, > +=09DHCP_OVERLOAD_SNAME, > +}; > + > +/** > + * fill() - Fill options in message, with overload into file/sname if ne= eded > + * @m:=09=09=09Message to fill > + * @overload:=09=09Set to option 52 value (0 if none, 1/2/3 per RFC 2132= ) > + * @has_bootfile:=09Reserve file field for boot file name > * > * Return: current size of options field > */ > -static int fill(struct msg *m) > +static int fill(struct msg *m, enum dhcp_overload *overload, bool has_bo= otfile) > { > +=09int sname_off =3D 0, file_off =3D 0; > +=09*overload =3D DHCP_OVERLOAD_NONE; > +=09/* Reserve 3 bytes for option 52 (overload) if needed */ > +=09size_t size =3D OPT_MAX - 3; Even though C11 admits this, for coding style reasons, we avoid mixing code and declarations. You could simply assign *overload later. > =09int i, o, offset =3D 0; > =20 > =09for (o =3D 0; o < 255; o++) > @@ -199,14 +217,47 @@ static int fill(struct msg *m) > =09for (i =3D 0; i < opts[55].clen; i++) { > =09=09o =3D opts[55].c[i]; > =09=09if (opts[o].state !=3D OPT_UNSET) > -=09=09=09if (fill_one(m->o, OPT_MAX, o, &offset)) > -=09=09=09=09debug("DHCP: skipping option %i", o); > +=09=09=09fill_one(m->o, size, o, &offset); > =09} > =20 > =09for (o =3D 0; o < 255; o++) { > =09=09if (opts[o].state !=3D OPT_UNSET && !opts[o].sent) > -=09=09=09if (fill_one(m->o, OPT_MAX, o, &offset)) > -=09=09=09=09debug("DHCP: skipping option %i", o); > +=09=09=09fill_one(m->o, size, o, &offset); > +=09} > + > +=09/* Overflow unsent options into sname, then file */ > +=09for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > +=09=09if (opts[o].state =3D=3D OPT_UNSET || opts[o].sent) > +=09=09=09continue; > +=09=09fill_one(m->sname, sizeof(m->sname) - 1, o, &sname_off); > +=09} > + > +=09for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > +=09=09if (opts[o].state =3D=3D OPT_UNSET || opts[o].sent) > +=09=09=09continue; > + > +=09=09if (!has_bootfile && > +=09=09=09fill_one(m->file, sizeof(m->file) - 1, o, > +=09=09=09 &file_off)) Nit: this should be indented like this: =09=09if (!has_bootfile && =09=09 fill_one(m->file, sizeof(m->file) - 1, o, &file_off)) so that terms that are logically on the same level are also aligned visually. And: > +=09=09=09=09debug("DHCP: skipping option %i" > +=09=09=09=09 " (overload full)", o); ...the indentation of this one if misleading (it looks like you have two if clauses). On the other hand, if David's suggestion about making fill() return void and print errors later works, this would go away altogether. > +=09} > + > +=09if (sname_off) { > +=09=09m->sname[sname_off] =3D 255; > +=09=09*overload |=3D DHCP_OVERLOAD_SNAME; > +=09} > + > +=09if (file_off) { > +=09=09m->file[file_off] =3D 255; > +=09=09*overload |=3D DHCP_OVERLOAD_FILE; > +=09} > + > + Excess blank line. > +=09if (*overload) { > +=09=09m->o[offset++] =3D 52; > +=09=09m->o[offset++] =3D 1; > +=09=09m->o[offset++] =3D *overload; > =09} > =20 > =09m->o[offset++] =3D 255; > @@ -320,6 +371,7 @@ static void opt_set_dns_search(const struct ctx *c, s= ize_t max_len) > int dhcp(const struct ctx *c, struct iov_tail *data) > { > =09char macstr[ETH_ADDRSTRLEN]; > +=09enum dhcp_overload overload; > =09size_t mlen, dlen, opt_len; > =09struct in_addr mask, dst; > =09struct ethhdr eh_storage; > @@ -328,9 +380,12 @@ int dhcp(const struct ctx *c, struct iov_tail *data) > =09const struct ethhdr *eh; > =09const struct iphdr *iph; > =09const struct udphdr *uh; > +=09uint8_t bootfile[128]; > =09struct msg m_storage; > =09struct msg const *m; > +=09bool has_bootfile; > =09struct msg reply; > +=09int bootfile_len; > =09unsigned int i; > =20 > =09eh =3D IOV_REMOVE_HEADER(data, eh_storage); > @@ -492,9 +547,24 @@ int dhcp(const struct ctx *c, struct iov_tail *data) > =09} > =20 > =09if (!c->no_dhcp_dns_search) > -=09=09opt_set_dns_search(c, sizeof(m->o)); > +=09=09 /* 3 bytes reserved for option 52 (code, length, value) */ Nits: excess whitespace before /*. Even if not needed, our coding style uses curly brackets whenever we have multiple lines (not statements) inside an if clause. > +=09=09opt_set_dns_search(c, OPT_MAX - 3); > + > +=09/* RFC 2132, Section 9.5: put boot file name in the 'file' header > +=09 * field. Suppress option 67 from the options area and reserve > +=09 * the file field from overload. > +=09 */ > +=09has_bootfile =3D opts[67].slen > 0 && > +=09=09 (size_t)opts[67].slen < sizeof(reply.file); This looks reasonable, I'm not sure if it's mandatory (I couldn't find anything in the RFCs that would mandate it). If it's not, we might be wasting precious space in the 'file' field if the filename is short, and keeping it as an option would actually save space (think of a filename that's 8 characters long). On the other hand, deciding whether to send the filename as an option or as 'file' is probably not trivial, so, unless you have a quick solution in mind, I'd just skip that and keep things like this. > +=09if (has_bootfile) { > +=09=09memcpy(bootfile, opts[67].s, opts[67].slen); > +=09=09bootfile_len =3D opts[67].slen; > +=09} > + > +=09dlen =3D offsetof(struct msg, o) + fill(&reply, &overload, has_bootfi= le); > =20 > -=09dlen =3D offsetof(struct msg, o) + fill(&reply); > +=09if (has_bootfile) > +=09=09memcpy(reply.file, bootfile, bootfile_len); > =20 > =09if (m->flags & FLAG_BROADCAST) > =09=09dst =3D in4addr_broadcast; --=20 Stefano