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=ZAR0P9ib; 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 365205A0262 for ; Fri, 19 Jun 2026 09:55:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781855740; 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: in-reply-to:in-reply-to:references:references; bh=h8J6S0Arprg0cK54SW2dO0IuLxarH+S/KEdu4p8ipuA=; b=ZAR0P9ibasu2/T3S2rkv4e0UjlhSu16NhggIYSpK4so8DmJeIt90SEcXw75Ei2UV7yE7u6 8XIcSKm09916tyrg687lEAKd6Fbiqo0HM1DF1K91dpZ/wkerrhhDqGPBYt2dulNUcWfx4l 058E7iC14n+bhgSJ8vWJjZ96XCo96iU= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-407-jhT4KtshMdepK2lGFU7ZOw-1; Fri, 19 Jun 2026 03:55:33 -0400 X-MC-Unique: jhT4KtshMdepK2lGFU7ZOw-1 X-Mimecast-MFC-AGG-ID: jhT4KtshMdepK2lGFU7ZOw_1781855732 Received: by mail-lj1-f197.google.com with SMTP id 38308e7fff4ca-399770df190so12506751fa.2 for ; Fri, 19 Jun 2026 00:55:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781855732; x=1782460532; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=h8J6S0Arprg0cK54SW2dO0IuLxarH+S/KEdu4p8ipuA=; b=dLi5JcFWkhxkTXMQJn5vx91rB+zs5lstYxbmJ4YFhzZVOwl7DUsoBThVNIR87j1zux 7Wpw39UYpKJQou3afWr04zpYodNtrndAZ6AwgVaZzSS0QJE41WIOwdU+FxdQe9R6+iR+ SGi9LrvmQv+RdQub8aVvdFhBJgP4sTNFdgc30Y633to9ZhlZlEPlq1/d0rRJyhuThdjf mAwuZHKv4R7MNO0V5QUtaIzatifkW0Klvh6xLlVPLQ2OsgijsCsPrx/pTnjflihF2Er+ IfCI8Hnh+RoS5oT9VjXjXLcD4wqhCF3yn4Pla3XkYs+ym/19b8WQXcdurXThB7IJdNmL Lxdg== X-Gm-Message-State: AOJu0YyjYcXOb2w/JGKk9RdYqoHIUrtwEJP6sSzXMnK5sp5jZyz6WvA6 jYE4Sm31eJy5Ggr0aNagMyWUX2epImtuHKv+yo2M5LWd2Zd62emtVY6fum8c8nCPrYzwto129TA p+/tbMSRz9Qa/oeffpjG+GDDkUNtC39upaL7abBgPngjG53b59N/V3rTiwJoZ07uKLplF2RJRGt xFRUfjTnP2UZ1qotJs+1/tHWrAqsnN X-Gm-Gg: AfdE7ck3LrPBNTp3zg14JfEOsmoBtOBU/263Ln/gzj9APrjAAouVHB9t16KnpVlh95t Hzkq1bi19uCGLPMO4lDFge2SUnI/g7E93x0znS0DrMS5av0GFsciXhCweLYFDTrRCWDFaHcG/JH dnmNdQIBflXGu+nzhJFFEeJclUbwQ8HnKsoWDwtyV0lN7nJo8VxZC74ysca4kD9rZnloynuOVqb RMKnvmez6fKgt9XLGzcWVOki9bW X-Received: by 2002:a05:651c:4351:10b0:396:b398:5990 with SMTP id 38308e7fff4ca-3998a21e5bamr5771771fa.10.1781855731709; Fri, 19 Jun 2026 00:55:31 -0700 (PDT) X-Received: by 2002:a05:651c:4351:10b0:396:b398:5990 with SMTP id 38308e7fff4ca-3998a21e5bamr5771721fa.10.1781855731229; Fri, 19 Jun 2026 00:55:31 -0700 (PDT) MIME-Version: 1.0 References: <20260617132243.1499556-1-anskuma@redhat.com> <20260617132243.1499556-3-anskuma@redhat.com> In-Reply-To: From: Anshu Kumari Date: Fri, 19 Jun 2026 13:25:18 +0530 X-Gm-Features: AVVi8CcP0h1LfU3mjnYSLzxWE04fz_7E-l-oARTbBan3KBo0l7W2eHzd8lgXvWM Message-ID: Subject: Re: [PATCH v4 2/4] dhcp: Add option overload To: David Gibson X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: xxIXmnU1Fn0uWc8cn7EvtQndni4SO8-oJb1YL4zkw1I_1781855732 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="000000000000e6bb4d0654969d1b" Message-ID-Hash: IVGQOAYNCMKAPRP3OPJOC3XUFBU6VPJA X-Message-ID-Hash: IVGQOAYNCMKAPRP3OPJOC3XUFBU6VPJA X-MailFrom: anskuma@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, sbrivio@redhat.com, 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: --000000000000e6bb4d0654969d1b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Jun 19, 2026 at 9:24=E2=80=AFAM David Gibson wrote: > On Wed, Jun 17, 2026 at 06:52:36PM +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. > > > > 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. > > > > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > > Signed-off-by: Anshu Kumari > > --- > > 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. > > > > 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= for > > 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). > > > > v2: > > - Added #define DHCP_OVERLOAD_FILE and #define DHCP_OVERLOAD_SNAME > constants > > - Added comment documenting space reservation: /* Reserve 3 bytes for > option 52 */ > > - Fixed DNS search length: sizeof(m->o) only, not combined with > file+sname > > - Removed dhcp_boot references =E2=80=94 reply.file copy now reads fr= om > opts[67] > > - Used DHCP_OVERLOAD_FILE constant in reply.file guard > > --- > > dhcp.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 100 insertions(+), 8 deletions(-) > > > > diff --git a/dhcp.c b/dhcp.c > > index 8e3ffd6..78790d8 100644 > > --- a/dhcp.c > > +++ b/dhcp.c > > @@ -160,14 +160,79 @@ static bool fill_one(uint8_t *buf, size_t size, > int o, int *offset) > > return false; > > } > > > > +/* RFC 2132, Section 9.3 - Option Overload */ > > +enum dhcp_overload { > > + DHCP_OVERLOAD_NONE =3D 0, > > + DHCP_OVERLOAD_FILE =3D 1, > > + DHCP_OVERLOAD_SNAME =3D 2, > > +}; > > + > > /** > > - * fill() - Fill options in message > > - * @m: Message to fill > > + * fill_overflow() - Fill remaining options into sname and file fields > > + * @m: Msg whose sname/file fields may be used > for overflow > > + * @has_bootfile: If true, reserve the file field for the boot file > name > > + * > > + * Try the smaller sname field first: small options go there, leaving > > + * the larger file field available for big options. When @has_bootfile > > + * is set, the file field is reserved for the boot file name per > > + * RFC 2132, Section 9.5 and is not used for option overflow. > > + * > > + * Return: option 52 overload value > > + */ > > +static enum dhcp_overload fill_overflow(struct msg *m, bool > has_bootfile) > > +{ > > + enum dhcp_overload overload =3D DHCP_OVERLOAD_NONE; > > + int sname_off =3D 0, file_off =3D 0; > > + int o; > > + > > + for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > > + if (opts[o].slen =3D=3D -1 || opts[o].sent) > > + continue; > > + fill_one(m->sname, sizeof(m->sname) - 1, o, &sname_off); > > Ah, I think I see now. The earlier debug() messages were removed in > favour of the ones later, because we don't want spurious messages if > the option doesn't fit in the main area but does fit in the overload. > > That's a reasonable change, but needs to be better explained. It also > all belongs in this patch, so the bits that leaked into the previous > patch should be here instead. > > > + } > > + > > + if (!has_bootfile) { > > + for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > > + if (opts[o].slen =3D=3D -1 || opts[o].sent) > > + continue; > > + if (fill_one(m->file, sizeof(m->file) - 1, o, > > + &file_off)) > > + debug("DHCP: skipping option %i" > > + " (overload full)", o); > > + } > > + } else { > > Rather that checking for fill_one() failures only on the last pass, > then having this fallback case, you can have a single unconditional > pass at the end looking for skipped options. Then fill_one()s > signature can be changed to void to match. > > > + for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o++) { > > + if (opts[o].slen =3D=3D -1 || opts[o].sent) > > + continue; > > + debug("DHCP: skipping option %i (overload full)", > o); > > + } > > + } > > + > > + if (sname_off) { > > + m->sname[sname_off] =3D 255; > > + overload |=3D DHCP_OVERLOAD_SNAME; > > + } > > + > > + if (file_off) { > > + m->file[file_off] =3D 255; > > + overload |=3D DHCP_OVERLOAD_FILE; > > + } > > + > > + return overload; > > +} > > + > > +/** > > + * fill() - Fill options in message, with overload into file/sname if > needed > > + * @m: Message to fill > > + * @overload: Set to option 52 value (0 if none, 1/2/3 > per RFC 2132) > > + * @has_bootfile: Reserve 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_bootfile) > > { > > + /* Reserve 3 bytes for option 52 (overload) if needed */ > > + size_t size =3D OPT_MAX - 3; > > int i, o, offset =3D 0; > > > > for (o =3D 0; o < 255; o++) > > @@ -178,17 +243,25 @@ static int fill(struct msg *m) > > * Put it there explicitly, unless requested via option 55. > > */ > > if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) > > - fill_one(m->o, OPT_MAX, 53, &offset); > > + fill_one(m->o, size, 53, &offset); > > > > for (i =3D 0; i < opts[55].clen; i++) { > > o =3D opts[55].c[i]; > > if (opts[o].slen !=3D -1) > > - fill_one(m->o, OPT_MAX, o, &offset); > > + fill_one(m->o, size, o, &offset); > > } > > > > for (o =3D 0; o < 255; o++) { > > if (opts[o].slen !=3D -1 && !opts[o].sent) > > - fill_one(m->o, OPT_MAX, o, &offset); > > + fill_one(m->o, size, o, &offset); > > + } > > + > > + *overload =3D fill_overflow(m, has_bootfile); > > Is there a particular reason to put fill_overflow() in its own > function, rather than just inline here? > There is no particular reason. I just thought it would be better to have a separate function for option overload (may be for better clarity). > > + > > + if (*overload) { > > + m->o[offset++] =3D 52; > > + m->o[offset++] =3D 1; > > + m->o[offset++] =3D *overload; > > } > > > > m->o[offset++] =3D 255; > > @@ -301,6 +374,7 @@ static void opt_set_dns_search(const struct ctx *c, > size_t max_len) > > int dhcp(const struct ctx *c, struct iov_tail *data) > > { > > char macstr[ETH_ADDRSTRLEN]; > > + enum dhcp_overload overload; > > size_t mlen, dlen, opt_len; > > struct in_addr mask, dst; > > struct ethhdr eh_storage; > > @@ -309,9 +383,12 @@ int dhcp(const struct ctx *c, struct iov_tail *dat= a) > > const struct ethhdr *eh; > > const struct iphdr *iph; > > const struct udphdr *uh; > > + uint8_t bootfile[128]; > > struct msg m_storage; > > struct msg const *m; > > + bool has_bootfile; > > struct msg reply; > > + int bootfile_len; > > unsigned int i; > > > > eh =3D IOV_REMOVE_HEADER(data, eh_storage); > > @@ -462,9 +539,24 @@ int dhcp(const struct ctx *c, struct iov_tail *dat= a) > > } > > > > if (!c->no_dhcp_dns_search) > > - opt_set_dns_search(c, sizeof(m->o)); > > + opt_set_dns_search(c, OPT_MAX - 3); > > + > > + /* RFC 2132, Section 9.5: put boot file name in the 'file' header > > + * field. Suppress option 67 from the options area and reserve > > + * the file field from overload. > > + */ > > + has_bootfile =3D opts[67].slen > 0 && > > + (size_t)opts[67].slen < sizeof(reply.file); > > + if (has_bootfile) { > > + memcpy(bootfile, opts[67].s, opts[67].slen); > > + bootfile_len =3D opts[67].slen; > > + opts[67].slen =3D -1; > > + } > > + > > + dlen =3D offsetof(struct msg, o) + fill(&reply, &overload, > has_bootfile); > > > > - dlen =3D offsetof(struct msg, o) + fill(&reply); > > + if (has_bootfile) > > + memcpy(reply.file, bootfile, bootfile_len); > > > > if (m->flags & FLAG_BROADCAST) > > dst =3D in4addr_broadcast; > > -- > > 2.54.0 > > > > -- > David Gibson (he or they) | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you, not the other wa= y > | around. > http://www.ozlabs.org/~dgibson > --000000000000e6bb4d0654969d1b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, Jun 19,= 2026 at 9:24=E2=80=AFAM David Gibson <david@gibson.dropbear.id.au> wrote:
On Wed, Jun 17, 2026 at 06:52:36PM= +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.
>
> Per RFC 2132, Section 9.5, the boot file name is always placed in the<= br> > 'file' header field.=C2=A0 When a boot file is set, the file f= ield is
> reserved from overload and overflow uses only the sname field.
>
> Link: https://bugs.passt.top/show_bug.cgi?id=3D192<= /a>
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
> v4:
>=C2=A0 =C2=A0- Converted overload #defines to enum dhcp_overload.
>=C2=A0 =C2=A0- Fixed missing whitespace in comment before */.
>=C2=A0 =C2=A0- Boot file name always placed in 'file' header fi= eld per RFC 2132,
>=C2=A0 =C2=A0 =C2=A0Section 9.5; file field reserved from overload when= bootfile is
>=C2=A0 =C2=A0 =C2=A0set; option 67 suppressed from options area.
>
> v3:
>=C2=A0 =C2=A0- Added RFC 2132 Section 9.3 reference comment on overload=
>=C2=A0 =C2=A0 =C2=A0constants.
>=C2=A0 =C2=A0- Use ARRAY_SIZE(opts) instead of raw 255 in fill_overflow= ().
>=C2=A0 =C2=A0- Swapped overflow order: try sname (64 bytes) first, then= file
>=C2=A0 =C2=A0 =C2=A0(128 bytes) =E2=80=94 better packing and keeps file= field available for
>=C2=A0 =C2=A0 =C2=A0boot file name.
>=C2=A0 =C2=A0- Removed '&' from &reply.file.
>=C2=A0 =C2=A0- Removed '+1' from memcpy =E2=80=94 reply.file al= ready zeroed.
>=C2=A0 =C2=A0- opt_set_dns_search() max_len: OPT_MAX - 3 instead of
>=C2=A0 =C2=A0 =C2=A0sizeof(m->o).
>
> v2:
>=C2=A0 =C2=A0- Added #define DHCP_OVERLOAD_FILE and #define DHCP_OVERLO= AD_SNAME constants
>=C2=A0 =C2=A0- Added comment documenting space reservation: /* Reserve = 3 bytes for option 52 */
>=C2=A0 =C2=A0- Fixed DNS search length: sizeof(m->o) only, not combi= ned with file+sname
>=C2=A0 =C2=A0- Removed dhcp_boot references =E2=80=94 reply.file copy n= ow reads from opts[67]
>=C2=A0 =C2=A0- Used DHCP_OVERLOAD_FILE constant in reply.file guard
> ---
>=C2=A0 dhcp.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++= +-----
>=C2=A0 1 file changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/dhcp.c b/dhcp.c
> index 8e3ffd6..78790d8 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -160,14 +160,79 @@ static bool fill_one(uint8_t *buf, size_t size, = int o, int *offset)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return false;
>=C2=A0 }
>=C2=A0
> +/* RFC 2132, Section 9.3 - Option Overload */
> +enum dhcp_overload {
> +=C2=A0 =C2=A0 =C2=A0DHCP_OVERLOAD_NONE=C2=A0 =3D 0,
> +=C2=A0 =C2=A0 =C2=A0DHCP_OVERLOAD_FILE=C2=A0 =3D 1,
> +=C2=A0 =C2=A0 =C2=A0DHCP_OVERLOAD_SNAME =3D 2,
> +};
> +
>=C2=A0 /**
> - * fill() - Fill options in message
> - * @m:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Message = to fill
> + * fill_overflow() - Fill remaining options into sname and file field= s
> + * @m:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0Msg whose sname/file fields may be used for overflow > + * @has_bootfile:=C2=A0 =C2=A0 If true, reserve the file field for th= e boot file name
> + *
> + * Try the smaller sname field first: small options go there, leaving=
> + * the larger file field available for big options. When @has_bootfil= e
> + * is set, the file field is reserved for the boot file name per
> + * RFC 2132, Section 9.5 and is not used for option overflow.
> + *
> + * Return: option 52 overload value
> + */
> +static enum dhcp_overload fill_overflow(struct msg *m, bool has_bootf= ile)
> +{
> +=C2=A0 =C2=A0 =C2=A0enum dhcp_overload overload =3D DHCP_OVERLOAD_NON= E;
> +=C2=A0 =C2=A0 =C2=A0int sname_off =3D 0, file_off =3D 0;
> +=C2=A0 =C2=A0 =C2=A0int o;
> +
> +=C2=A0 =C2=A0 =C2=A0for (o =3D 0; (size_t)o < ARRAY_SIZE(opts); o+= +) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (opts[o].slen =3D= =3D -1 || opts[o].sent)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0continue;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fill_one(m->sname,= sizeof(m->sname) - 1, o, &sname_off);

Ah, I think I see now.=C2=A0 The earlier debug() messages were removed in favour of the ones later, because we don't want spurious messages if the option doesn't fit in the main area but does fit in the overload.
That's a reasonable change, but needs to be better explained.=C2=A0 It = also
all belongs in this patch, so the bits that leaked into the previous
patch should be here instead.

> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0if (!has_bootfile) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (o =3D 0; (size_t= )o < ARRAY_SIZE(opts); o++) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (opts[o].slen =3D=3D -1 || opts[o].sent)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0continue;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (fill_one(m->file, sizeof(m->file) - 1, o,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &file_off))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0debug("DHCP: skipping option %i&quo= t;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0" (overload fu= ll)", o);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0} else {

Rather that checking for fill_one() failures only on the last pass,
then having this fallback case, you can have a single unconditional
pass at the end looking for skipped options.=C2=A0 Then fill_one()s
signature can be changed to void to match.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (o =3D 0; (size_t= )o < ARRAY_SIZE(opts); o++) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (opts[o].slen =3D=3D -1 || opts[o].sent)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0continue;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0debug("DHCP: skipping option %i (overload full)", o);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0if (sname_off) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m->sname[sname_off= ] =3D 255;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0overload |=3D DHCP_OV= ERLOAD_SNAME;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0if (file_off) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m->file[file_off] = =3D 255;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0overload |=3D DHCP_OV= ERLOAD_FILE;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0return overload;
> +}
> +
> +/**
> + * fill() - Fill options in message, with overload into file/sname if= needed
> + * @m:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0Message to fill
> + * @overload:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = Set to option 52 value (0 if none, 1/2/3 per RFC 2132)
> + * @has_bootfile:=C2=A0 =C2=A0 Reserve file field for boot file name<= br> >=C2=A0 =C2=A0*
>=C2=A0 =C2=A0* Return: current size of options field
>=C2=A0 =C2=A0*/
> -static int fill(struct msg *m)
> +static int fill(struct msg *m, enum dhcp_overload *overload, bool has= _bootfile)
>=C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0/* Reserve 3 bytes for option 52 (overload) if ne= eded */
> +=C2=A0 =C2=A0 =C2=A0size_t size =3D OPT_MAX - 3;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int i, o, offset =3D 0;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0for (o =3D 0; o < 255; o++)
> @@ -178,17 +243,25 @@ static int fill(struct msg *m)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Put it there explicitly, unless requested= via option 55.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (opts[55].clen > 0 && !memchr(= opts[55].c, 53, opts[55].clen))
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fill_one(m->o, OPT= _MAX, 53, &offset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fill_one(m->o, siz= e, 53, &offset);
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < opts[55].clen; i++) { >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0o =3D opts[55].c= [i];
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (opts[o].slen= !=3D -1)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0fill_one(m->o, OPT_MAX, o, &offset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0fill_one(m->o, size, o, &offset);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0for (o =3D 0; o < 255; o++) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (opts[o].slen= !=3D -1 && !opts[o].sent)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0fill_one(m->o, OPT_MAX, o, &offset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0fill_one(m->o, size, o, &offset);
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0*overload =3D fill_overflow(m, has_bootfile);

Is there a particular reason to put fill_overflow() in its own
function, rather than just inline here?

There is no particular reason. I just thought it would be better to
<= div>have a separate function for option overload (may be for better clarity= ).=C2=A0


> +
> +=C2=A0 =C2=A0 =C2=A0if (*overload) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m->o[offset++] =3D= 52;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m->o[offset++] =3D= 1;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m->o[offset++] =3D= *overload;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0m->o[offset++] =3D 255;
> @@ -301,6 +374,7 @@ static void opt_set_dns_search(const struct ctx *c= , size_t max_len)
>=C2=A0 int dhcp(const struct ctx *c, struct iov_tail *data)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0char macstr[ETH_ADDRSTRLEN];
> +=C2=A0 =C2=A0 =C2=A0enum dhcp_overload overload;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0size_t mlen, dlen, opt_len;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct in_addr mask, dst;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct ethhdr eh_storage;
> @@ -309,9 +383,12 @@ int dhcp(const struct ctx *c, struct iov_tail *da= ta)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct ethhdr *eh;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct iphdr *iph;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct udphdr *uh;
> +=C2=A0 =C2=A0 =C2=A0uint8_t bootfile[128];
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct msg m_storage;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct msg const *m;
> +=C2=A0 =C2=A0 =C2=A0bool has_bootfile;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct msg reply;
> +=C2=A0 =C2=A0 =C2=A0int bootfile_len;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int i;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0eh =3D IOV_REMOVE_HEADER(data, eh_storage);<= br> > @@ -462,9 +539,24 @@ int dhcp(const struct ctx *c, struct iov_tail *da= ta)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!c->no_dhcp_dns_search)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opt_set_dns_search(c,= sizeof(m->o));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opt_set_dns_search(c,= OPT_MAX - 3);
> +
> +=C2=A0 =C2=A0 =C2=A0/* RFC 2132, Section 9.5: put boot file name in t= he 'file' header
> +=C2=A0 =C2=A0 =C2=A0 * field.=C2=A0 Suppress option 67 from the optio= ns area and reserve
> +=C2=A0 =C2=A0 =C2=A0 * the file field from overload.
> +=C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0has_bootfile =3D opts[67].slen > 0 &&<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (size_t)opts[67].slen < sizeof(reply.file);
> +=C2=A0 =C2=A0 =C2=A0if (has_bootfile) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memcpy(bootfile, opts= [67].s, opts[67].slen);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bootfile_len =3D opts= [67].slen;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts[67].slen =3D -1;=
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0dlen =3D offsetof(struct msg, o) + fill(&repl= y, &overload, has_bootfile);
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0dlen =3D offsetof(struct msg, o) + fill(&repl= y);
> +=C2=A0 =C2=A0 =C2=A0if (has_bootfile)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memcpy(reply.file, bo= otfile, bootfile_len);
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (m->flags & FLAG_BROADCAST)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dst =3D in4addr_= broadcast;
> --
> 2.54.0
>

--
David Gibson (he or they)=C2=A0 =C2=A0 =C2=A0 =C2=A0| I'll have my musi= c baroque, and my code
david AT gibson.dropbear.id.au=C2=A0 | minimalist, thank you, not th= e other way
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | around.
http://www.ozlabs.org/~dgibson
--000000000000e6bb4d0654969d1b--