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

[Laurent Vivier <lvivier@redhat.com>]

On 5/26/26 14:31, 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>
> ---
> 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
>    */
> -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)

Perhaps we can keep the comments?

> +	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 */

ditto

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

Reviewed-by: Laurent Vivier <lvivier@redhat.com>