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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3443 bytes --]

On Wed, Jun 17, 2026 at 06:52:35PM +0530, Anshu Kumari 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.

Uh.. this doesn't appear to be true...

> 
> 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)
>  		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);

..LGTM so far, but..

>  
>  	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);

..you've removed all the debug messages on failed inserts.  Maybe
there's a reason for that, but it's not mentioned in the commit
message, and is new in v4.

>  
>  	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;
> -- 
> 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 way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]