Re: [PATCH v7 5/7] dhcp: Change fill_one() to return void

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 — 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@re... 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.

Link: https://bugs.passt.top/show_bug.cgi?id=192 Signed-off-by: Anshu Kumari --- v7: - (new patch) Split out from the option overload patch into its own commit

--- dhcp.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)

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: Usable size of @buf (excluding end marker) * @o: Option number * @offset: Current 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) { size_t slen = opts[o].slen;

/* If we don't have space to write the option, then just skip */ if (*offset + 2 /* code and length of option */ + slen > size) - return true; + return;

buf[*offset] = o; buf[*offset + 1] = slen; @@ -439,7 +437,6 @@ static bool fill_one(uint8_t *buf, size_t size, int o, int *offset)

opts[o].sent = 1; *offset += slen; - return false; }

/** @@ -459,24 +456,18 @@ static int fill(struct msg *m) * option 53 at the beginning of the list. * Put it there explicitly, unless requested via option 55. */ - if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) { - if (fill_one(m->o, OPT_MAX, 53, &offset)) - debug("DHCP: skipping option 53"); - } + if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) + fill_one(m->o, OPT_MAX, 53, &offset);

for (i = 0; i < opts[55].clen; i++) { o = opts[55].c[i]; - if (opts[o].conf != OPT_UNSET) { - if (fill_one(m->o, OPT_MAX, o, &offset)) - debug("DHCP: skipping option %i", o); - } + if (opts[o].conf != OPT_UNSET) + fill_one(m->o, OPT_MAX, o, &offset); }

for (o = 0; o < 255; o++) { - if (opts[o].conf != OPT_UNSET && !opts[o].sent) { - if (fill_one(m->o, OPT_MAX, o, &offset)) - debug("DHCP: skipping option %i", o); - } + if (opts[o].conf != OPT_UNSET && !opts[o].sent) + fill_one(m->o, OPT_MAX, o, &offset); }

m->o[offset++] = 255;

...the change itself looks good to me. -- Stefano