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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu,  1 Oct 2026 18:45:58 +0530
Anshu Kumari <anskuma@redhat.com> 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@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.

> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
> 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