Re: [PATCH v4 1/4] dhcp: Refactor fill_one() to operate on a generic buffer

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 17 Jun 2026 18:52:35 +0530
Anshu Kumari <anskuma@redhat.com> wrote:

> Change fill_one() to accept a buffer pointer and capacity instead of
> a struct msg pointer.  This is a pure refactor with no behavior change,
> preparing for option overload support where fill_one() will also write
> into the file and sname fields.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=192
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
> v4:
>   - Unchanged from v3.
> 
> v3:
>   - Restored removed comments: "If we don't have space to write the
>     option, then just skip" and "Move to option".
> 
> v2:
>   - Renamed parameter cap → size.
> ---
>  dhcp.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 1ff8cba..8e3ffd6 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -131,28 +131,29 @@ struct msg {
>  } __attribute__((__packed__));
>  
>  /**
> - * fill_one() - Fill a single option in message
> - * @m:		Message to fill
> + * fill_one() - Fill a single option into a buffer
> + * @buf:	Buffer to write option
> + * @size:	Usable size of @buf (excluding end marker)
>   * @o:		Option number
> - * @offset:	Current offset within options field, updated on insertion
> + * @offset:	Current offset within @buf, updated on insertion
>   *
> - * Return: false if m has space to write the option, true otherwise
> + * Return: false if @buf has space to write the option, true otherwise
>   */
> -static bool fill_one(struct msg *m, int o, int *offset)
> +static bool 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 > OPT_MAX)
> +	if (*offset + 2 + slen > size)

This looks like an automatic linter dropping comments, but I think the
original comment is quite useful. See also Laurent's review of v2:

  https://archives.passt.top/passt-dev/3b36426b-798c-489c-aad2-abcde13d2e93@redhat.com/

>  		return true;
>  
> -	m->o[*offset] = o;
> -	m->o[*offset + 1] = slen;
> +	buf[*offset] = o;
> +	buf[*offset + 1] = slen;
>  
>  	/* Move to option */
>  	*offset += 2;
>  
> -	memcpy(&m->o[*offset], opts[o].s, slen);
> +	memcpy(&buf[*offset], opts[o].s, slen);

I think slightly more idiomatic: buf + *offset. Otherwise you have a
pointer, dereference it, and then take its address. It looks a bit
convoluted.

Conversely, using buf[*offset] above makes sense because you need to
dereference buf anyway.

>  
>  	opts[o].sent = 1;
>  	*offset += slen;
> @@ -177,20 +178,17 @@ 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))
> -		if (fill_one(m, 53, &offset))
> -			 debug("DHCP: skipping option 53");
> +		fill_one(m->o, OPT_MAX, 53, &offset);
>  
>  	for (i = 0; i < opts[55].clen; i++) {
>  		o = opts[55].c[i];
>  		if (opts[o].slen != -1)
> -			if (fill_one(m, o, &offset))
> -				debug("DHCP: skipping option %i", o);
> +			fill_one(m->o, OPT_MAX, o, &offset);
>  	}
>  
>  	for (o = 0; o < 255; o++) {
>  		if (opts[o].slen != -1 && !opts[o].sent)
> -			if (fill_one(m, o, &offset))
> -				debug("DHCP: skipping option %i", o);
> +			fill_one(m->o, OPT_MAX, o, &offset);
>  	}
>  
>  	m->o[offset++] = 255;

I also had the same comment as David's about dropping debug()
messages... until I reached 2/4, which is a good indication that they
should go away later.

-- 
Stefano