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

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

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

On Tue, May 26, 2026 at 06:01:11PM +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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

One pre-existing oddity noted.  Might be nice to address (as a
separate patch) if you have the chance, but it's not a big deal.

> ---
> v2:
>   - Renamed parameter cap → size.
> ---
>  dhcp.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index e1c95ad..e126063 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -279,28 +279,27 @@ 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

Pre-existing wart: It's very unusual for bool-valued functions to
report failure with 'true' even though though it *is* usual for
int-valued functions to report failure with negative =~ non-zero =~
"trueish" values.

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