public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Laurent Vivier <lvivier@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v8 17/30] dhcp: Convert to iov_tail
Date: Fri, 8 Aug 2025 11:33:24 +0200	[thread overview]
Message-ID: <b716d5a8-7e73-45ff-8339-5661642a5c30@redhat.com> (raw)
In-Reply-To: <aJLcMFpd1jea8Dcc@zatzit>

On 06/08/2025 06:38, David Gibson wrote:
> On Tue, Aug 05, 2025 at 05:46:15PM +0200, Laurent Vivier wrote:
>> Use packet_data() and extract headers using IOV_REMOVE_HEADER()
>> and IOV_PEEK_HEADER() rather than packet_get().
> 
> Unlike the previous patch, I think using iov_tail does work here,
> because there's a single scan through the options, rather than
> repeatedly scanning for options of specific types.
> 
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   dhcp.c | 46 ++++++++++++++++++++++++++++------------------
>>   1 file changed, 28 insertions(+), 18 deletions(-)
>>
>> diff --git a/dhcp.c b/dhcp.c
>> index b0de04be6f27..cf73d4b07767 100644
>> --- a/dhcp.c
>> +++ b/dhcp.c
>> @@ -302,27 +302,33 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
>>    */
>>   int dhcp(const struct ctx *c, const struct pool *p)
>>   {
>> -	size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
>>   	char macstr[ETH_ADDRSTRLEN];
>> +	size_t mlen, dlen, opt_len;
>>   	struct in_addr mask, dst;
>> +	struct ethhdr eh_storage;
>> +	struct iphdr iph_storage;
>> +	struct udphdr uh_storage;
>>   	const struct ethhdr *eh;
>>   	const struct iphdr *iph;
>>   	const struct udphdr *uh;
>> +	struct iov_tail data;
>>   	struct msg const *m;
> 
> Pre-existing, but I'm a bit baffled as to what the (const *) is doing here.
> 
>>   	struct msg reply;
>>   	unsigned int i;
>> +	struct msg m_storage;
>>   
>> -	eh  = packet_get(p, 0, offset, sizeof(*eh),  NULL);
>> -	offset += sizeof(*eh);
>> +	if (!packet_data(p, 0, &data))
>> +		return -1;
>>   
>> -	iph = packet_get(p, 0, offset, sizeof(*iph), NULL);
>> +	eh = IOV_REMOVE_HEADER(&data, eh_storage);
>> +	iph = IOV_PEEK_HEADER(&data, iph_storage);
>>   	if (!eh || !iph)
>>   		return -1;
>>   
>> -	offset += iph->ihl * 4UL;
>> -	uh  = packet_get(p, 0, offset, sizeof(*uh),  &mlen);
>> -	offset += sizeof(*uh);
>> +	if (!iov_tail_drop(&data, iph->ihl * 4UL))
>> +		return -1;
>>   
>> +	uh = IOV_REMOVE_HEADER(&data, uh_storage);
>>   	if (!uh)
>>   		return -1;
>>   
>> @@ -332,7 +338,10 @@ int dhcp(const struct ctx *c, const struct pool *p)
>>   	if (c->no_dhcp)
>>   		return 1;
>>   
>> -	m   = packet_get(p, 0, offset, offsetof(struct msg, o), &opt_len);
>> +	mlen = iov_tail_size(&data);
>> +	m = (struct msg const *)iov_remove_header_(&data, &m_storage,
>> +						   offsetof(struct msg, o),
>> +						   __alignof__(struct msg));
>>   	if (!m						||
>>   	    mlen  != ntohs(uh->len) - sizeof(*uh)	||
>>   	    mlen  <  offsetof(struct msg, o)		||
>> @@ -355,27 +364,28 @@ int dhcp(const struct ctx *c, const struct pool *p)
>>   	memset(&reply.file,	0,		sizeof(reply.file));
>>   	reply.magic		= m->magic;
>>   
>> -	offset += offsetof(struct msg, o);
>> -
>>   	for (i = 0; i < ARRAY_SIZE(opts); i++)
>>   		opts[i].clen = -1;
>>   
>> -	while (opt_off + 2 < opt_len) {
>> -		const uint8_t *olen, *val;
>> +	opt_len = iov_tail_size(&data);
>> +	while (opt_len >= 2) {
>> +		uint8_t olen_storage, type_storage;
>> +		const uint8_t *olen;
>>   		uint8_t *type;
>>   
>> -		type = packet_get(p, 0, offset + opt_off,	1,	NULL);
>> -		olen = packet_get(p, 0, offset + opt_off + 1,	1,	NULL);
>> +		type = IOV_REMOVE_HEADER(&data, type_storage);
>> +		olen = IOV_REMOVE_HEADER(&data, olen_storage);
> 
> It seems a bit mad to access single bytes via 8-byte pointers, but
> it's probably not worth the hassle of handling it differently in this
> one case.
> 
>>   		if (!type || !olen)
>>   			return -1;
>>   
>> -		val =  packet_get(p, 0, offset + opt_off + 2,	*olen,	NULL);
>> -		if (!val)
>> +		opt_len = iov_tail_size(&data);
>> +		if (opt_len < *olen)
>>   			return -1;
>>   
>> -		memcpy(&opts[*type].c, val, *olen);
>> +		iov_to_buf(&data.iov[0], data.cnt, data.off, &opts[*type].c, *olen);
> 
> So, IIUC, if *olen is much too big, this is still safe..
> 
>>   		opts[*type].clen = *olen;
> 
> .. but recording *olen unedited as the length of the option is
> probably wrong in that case.

I don't understand how to edit *olen. There is no change regarding the original code.

> 
>> -		opt_off += *olen + 2;
>> +		iov_tail_drop(&data, *olen);
>> +		opt_len -= *olen;
> 
> Isn't the stanza above doing the equivalent of an
> iov_remove_header_()?

No, in fact iov_remove_header_() copy to the buffer only if the data are discontinuous, in 
this case we want to copy the data unconditionally to edit them later. We don't want to 
edit the data in the iovec buffer.

> 
>>   	}
>>   
>>   	opts[80].slen = -1;
> 

Thanks,
Laurent


  reply	other threads:[~2025-08-08  9:33 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-08-05 15:45 [PATCH v8 00/30] Introduce discontiguous frames management Laurent Vivier
2025-08-05 15:45 ` [PATCH v8 01/30] arp: Don't mix incoming and outgoing buffers Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 02/30] iov: Introduce iov_tail_clone() and iov_tail_drop() Laurent Vivier
2025-08-06  1:32   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 03/30] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER() Laurent Vivier
2025-08-06  1:45   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 04/30] tap: Use iov_tail with tap_add_packet() Laurent Vivier
2025-08-06  1:56   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 05/30] packet: Use iov_tail with packet_add() Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 06/30] packet: Add packet_data() Laurent Vivier
2025-08-06  2:14   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 07/30] arp: Convert to iov_tail Laurent Vivier
2025-08-06  2:17   ` David Gibson
2025-08-07 12:58     ` Laurent Vivier
2025-08-07 13:11       ` Stefano Brivio
2025-08-13  2:21         ` David Gibson
2025-08-05 15:46 ` [PATCH v8 08/30] ndp: " Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 09/30] icmp: " Laurent Vivier
2025-08-06  2:20   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 10/30] udp: " Laurent Vivier
2025-08-06  2:23   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 11/30] tcp: Convert tcp_tap_handler() to use iov_tail Laurent Vivier
2025-08-06  2:35   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 12/30] tcp: Convert tcp_data_from_tap() " Laurent Vivier
2025-08-06  2:37   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 13/30] dhcpv6: move offset initialization out of dhcpv6_opt() Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 14/30] dhcpv6: Extract sending of NotOnLink status Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 15/30] dhcpv6: Convert to iov_tail Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 16/30] dhcpv6: Use iov_tail in dhcpv6_opt() Laurent Vivier
2025-08-06  4:14   ` David Gibson
2025-08-08 13:59     ` Laurent Vivier
2025-08-13  2:29       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 17/30] dhcp: Convert to iov_tail Laurent Vivier
2025-08-06  4:38   ` David Gibson
2025-08-08  9:33     ` Laurent Vivier [this message]
2025-08-13  2:27       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 18/30] ip: Use iov_tail in ipv6_l4hdr() Laurent Vivier
2025-08-06  5:12   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 19/30] tap: Convert tap4_handler() to iov_tail Laurent Vivier
2025-08-06  5:17   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 20/30] tap: Convert tap6_handler() " Laurent Vivier
2025-08-06  6:21   ` David Gibson
2025-08-08 13:57     ` Laurent Vivier
2025-08-13  3:22       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 21/30] packet: rename packet_data() to packet_get() Laurent Vivier
2025-08-06  6:22   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 22/30] arp: use iov_tail rather than pool Laurent Vivier
2025-08-06  6:24   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 23/30] dhcp: " Laurent Vivier
2025-08-06  6:26   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 24/30] dhcpv6: " Laurent Vivier
2025-08-06  6:27   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 25/30] icmp: " Laurent Vivier
2025-08-06  6:29   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 26/30] ndp: " Laurent Vivier
2025-08-06  6:31   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 27/30] packet: remove PACKET_POOL() and PACKET_POOL_P() Laurent Vivier
2025-08-06  6:32   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 28/30] packet: remove unused parameter from PACKET_POOL_DECL() Laurent Vivier
2025-08-06  6:33   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 29/30] packet: Refactor vhost-user memory region handling Laurent Vivier
2025-08-07  6:10   ` David Gibson
2025-08-07  9:05     ` Laurent Vivier
2025-08-07 11:44       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 30/30] packet: Add support for multi-vector packets Laurent Vivier
2025-08-07  6:17   ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b716d5a8-7e73-45ff-8339-5661642a5c30@redhat.com \
    --to=lvivier@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).