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=WnSuzCmh; 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 774155A0269 for ; Thu, 08 Oct 2026 00:06:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1791410791; 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=1TiryDP/dEAQBWDBUT89TD2SUFPPN9Kg+zu73TzRXMs=; b=WnSuzCmh5QoC9AnbD02B7WZCqkWgAr2xh209GeMQ2LjqtbOp4SMMUQbxbHgP839aEzQyhw DwSNAQIJwMcr0Gn1tFt+i8a4XVPymd/IFc5XPvXxHVGTCdPRGu76TN0hS43ES7PLlx3Pgv JYyS5UcWQ+3kbDu717tOFkqhNFSILVc= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-96-zRUtD1nHP_2MpYP9KCbjFQ-1; Wed, 7 Oct 2026 22:06:30 +0000 X-MC-Unique: zRUtD1nHP_2MpYP9KCbjFQ-1 X-Mimecast-MFC-AGG-ID: zRUtD1nHP_2MpYP9KCbjFQ_1791410789 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-4887e5afd67so3633477f8f.3 for ; Wed, 07 Oct 2026 15:06:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1791410789; x=1792015589; 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=Qujs+Rf25OZ6NdiXDucdA44ova7hv9TKXVDHlxxsaL8=; b=GYdJSAY9OzP7xPSvLJT1TtKjIPXvqq6JWDAQMJfWj0kAV4SDpWe7L+WArq7NLnostg LTomROhSi/abJhEYUYtWHfY/T0Q20WkIzkLunY8EONttaFE05dYwGLoX9PpGRyzA+1B4 jU7Jxqgql2uEHXFWoNx7T7svU/ucIOFeuFpurgY/DxG43g6rGdUFCMJAc8UIYWZ+ZsA+ q5sWuvj3+530tOAXVw0w6G9fOgv1+TOylfY1In/qCLpSkfRKyzk+1tWe+0XwfFvPZkMo SW3J4pBV6lvpk+nN60B27zgYqcygVy65tvwR6Gr4loebib98FAjAkXoybjdz4gaVZhSp Hw9w== X-Gm-Message-State: AFq9FYKzve9caAE6QqWxvsVEbYAuYGWWNswQPJrz4LcjNEXiU3sClklJ KGx33A9584sevF28EKebOTSINqaxk4g94M8L0jC1NELfu0QPbBZ9/QQpg2eqCE2XW0eGeSnPGoH 4eCV04pZusD1vfDZKGPxnKor7b+wIIz41MVXE4l8B0e/o0dq83WPWRVfblmTeZp/uJ0pta0yv81 xpqov15VSIcQzdGJuOdDJNtVHzKeOftH+GFk7j X-Gm-Gg: AYBFou2Tgzqua1iRnl6KB/sNnougBBuwpg8M/92zA/IWyYQ+WFehVfJ8Zkz4huF4CzP CWWE+wHxWSQMUpYT9LhQVnQrXyi6FGFyh02OK2L4zYjbHsv1QtLH0oH1TD+zrhOIvkXo+Yb3pX5 yeQYakn/EM6BRDX2GzGugNp9wVv9U9fljbbW/BC+bJrlWnZZI6QatVrfZr8pfiDfvxNXylYlTCT 1L2s59u++tow8OCzBkothWgcyDIKM59xUUg2H1eFEicYSb11AD+DQsZ9lSUFtjjNSipHdxvFvid QGr+3TmVtZ8D7r1lPn4KVJZep4im0a5O7sQ/a/nVjpyIFhemHaHM8CR1aom6H1cDDEu+HZJC3q/ 2pBRv7IwaiQ== X-Received: by 2002:adf:fe50:0:b0:487:27f9:834 with SMTP id ffacd0b85a97d-48c7288b300mr5088689f8f.41.1791410788681; Wed, 07 Oct 2026 15:06:28 -0700 (PDT) X-Received: by 2002:adf:fe50:0:b0:487:27f9:834 with SMTP id ffacd0b85a97d-48c7288b300mr5088642f8f.41.1791410787989; Wed, 07 Oct 2026 15:06:27 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-48c71d3b502sm8026217f8f.52.2026.10.07.15.06.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Oct 2026 15:06:26 -0700 (PDT) From: Stefano Brivio To: Anshu Kumari Subject: Re: [PATCH v7 5/7] dhcp: Change fill_one() to return void Message-ID: <20261008000625.0595fa64@elisabeth> In-Reply-To: <20261001131602.653553-6-anskuma@redhat.com> References: <20261001131602.653553-1-anskuma@redhat.com> <20261001131602.653553-6-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:26 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: IfUc_zoWIH41otrEFIaFDL1DyOxUIznDw8AMaI3IgJA_1791410789 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: SHH6O3ULF2NPHGJLI4LXFHNG3DIHPYJB X-Message-ID-Hash: SHH6O3ULF2NPHGJLI4LXFHNG3DIHPYJB 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:58 +0530 Anshu Kumari wrote: > fill_one() previously returned true on overflow, but callers > used this to log a skip message per option =E2=80=94 which becomes > noisy if the options field is full. This looks generated by a language model that tried to come up with some kind of explanation, which sounds plausible, but entirely misses the point, as it often (usually?) happens. The number of messages (noise) is exactly the same, before and after this change, and also after the change that depends on this one. The rest of the description itself (not the motivation), though: > Instead, rely on opts[].sent (already set by fill_one()) to > detect unsent options: a final reporting loop walks all > options and logs a single skip message for each one that > couldn't be sent. This also makes the return value > unnecessary, so change fill_one() to return void. ...is plain wrong. That loop isn't implemented here. I guess the language model generated this before this was split off from 6/7? Or it simply had access to both as context? I think there's no need for such a long explanation either: this just does what you wrote in the title and, say, "make[s] fill_one() ignore failures because the next change will report all of them in a separate loop". By the way, I see Laurent's comment in: https://archives.passt.top/passt-dev/f6c80395-bb48-4697-88af-820def5e040f= @redhat.com/ and indeed, those are unrelated changes, but this change isn't really self-contained (it can't be) in the sense that it drops an important functionality, which is then restored by a subsequent patch (but if one just applies just this patch, the functionality is lost). This becomes obvious by looking at this change in isolation, of course. But it could also be obvious by mentioning this aspect in the commit message for 6/7. So I think it would actually be better to keep those together (Laurent could probably not see the dependency because it wasn't explained) and mention this explicitly in the commit message for 6/7: because of option overload, we now need a single loop reporting errors. >=20 > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > Signed-off-by: Anshu Kumari > --- > v7: > - (new patch) Split out from the option overload patch into its own com= mit >=20 > --- > dhcp.c | 25 ++++++++----------------- > 1 file changed, 8 insertions(+), 17 deletions(-) >=20 > diff --git a/dhcp.c b/dhcp.c > index 4dccae7b..6f69fd62 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -418,16 +418,14 @@ const char *dhcp_opt_to_str(uint8_t code, char *buf= , size_t buf_len) > * @size:=09Usable size of @buf (excluding end marker) > * @o:=09=09Option number > * @offset:=09Current offset within @buf, updated on insertion > - * > - * Return: false if @buf has space to write the option, true otherwise > */ > -static bool fill_one(uint8_t *buf, size_t size, int o, int *offset) > +static void fill_one(uint8_t *buf, size_t size, int o, int *offset) > { > =09size_t slen =3D opts[o].slen; > =20 > =09/* If we don't have space to write the option, then just skip */ > =09if (*offset + 2 /* code and length of option */ + slen > size) > -=09=09return true; > +=09=09return; > =20 > =09buf[*offset] =3D o; > =09buf[*offset + 1] =3D slen; > @@ -439,7 +437,6 @@ static bool fill_one(uint8_t *buf, size_t size, int o= , int *offset) > =20 > =09opts[o].sent =3D 1; > =09*offset +=3D slen; > -=09return false; > } > =20 > /** > @@ -459,24 +456,18 @@ static int fill(struct msg *m) > =09 * option 53 at the beginning of the list. > =09 * Put it there explicitly, unless requested via option 55. > =09 */ > -=09if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) { > -=09=09if (fill_one(m->o, OPT_MAX, 53, &offset)) > -=09=09=09debug("DHCP: skipping option 53"); > -=09} > +=09if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) > +=09=09fill_one(m->o, OPT_MAX, 53, &offset); > =20 > =09for (i =3D 0; i < opts[55].clen; i++) { > =09=09o =3D opts[55].c[i]; > -=09=09if (opts[o].conf !=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} > +=09=09if (opts[o].conf !=3D OPT_UNSET) > +=09=09=09fill_one(m->o, OPT_MAX, o, &offset); > =09} > =20 > =09for (o =3D 0; o < 255; o++) { > -=09=09if (opts[o].conf !=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} > +=09=09if (opts[o].conf !=3D OPT_UNSET && !opts[o].sent) > +=09=09=09fill_one(m->o, OPT_MAX, o, &offset); > =09} > =20 > =09m->o[offset++] =3D 255; ...the change itself looks good to me. --=20 Stefano