From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Ir2Fn1n/; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 54FFB5A0280 for ; Mon, 26 May 2025 16:19:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1748269156; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zOzHCcL6gUZjieKAJVVrp5JijdsKK2+oHDyPpT8MCnE=; b=Ir2Fn1n/dB6d71af/3HxfvFjWs8EbaQzQ9aQX6yb/PcLfv1J2gr/GdC2ah/S5ZguOQlePO qG6Z3GOopCCuQMWd+TgQIdNL1NvPYRqDywMdCTExaSgvNB2Ot2rd43uMGSPOoHjdmJIpFI dKFk3pGPcpJ4OZNRk9ziy0peLk+VtRA= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-650-e59ed_6tNhu4WUpwu6VXRQ-1; Mon, 26 May 2025 10:19:15 -0400 X-MC-Unique: e59ed_6tNhu4WUpwu6VXRQ-1 X-Mimecast-MFC-AGG-ID: e59ed_6tNhu4WUpwu6VXRQ_1748269154 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-44b1f5b91c1so17401785e9.1 for ; Mon, 26 May 2025 07:19:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1748269154; x=1748873954; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=QxIBAONV4Uq3zthnScNlHW0xxkf1NfnfdqCfOtzM0Go=; b=NCtaRJA/tKuk09qeBC/KnSL10k3mJ/XTbGzg3f7mYZ3znpAvCGJwdAnqSt4i4wYMZq p6wYoUrfku9ZFgboMMN7bfxZ57CAGGRUoztEVxtXWwtoMf4LlpnyrB+X7RebphXbRzVh ax3YiCjCXVHctlW9Yt7ckR4G7pKht14Hb8gP4iY4/ZwnB8HkXGt3Prxlm3+WvdlGKbPr /eXWtnF30hingUfiVOJv9YBdADtPERxHJcUoPAd91DN2ztxWPYJTxBoBxzunnBB6q2EF e/ZzRvOMe5Ht1YFvhV3pMCDm2LxnzDK56KiFxyDLCAIMtv+k48ZvbR3cS9cq2t2nsbEo 2rzA== X-Gm-Message-State: AOJu0YyQrN/ZyB9wZihh8MqZhNV4B6NTRsv+nzMRH+LA/DGQ3lyVWNOm 6kG0qndfUBq9hXltMAb7UQr01pue2IggG//OHzE8ZfKCV5e0AWR3QkcN7ON1VqQ7C0AGdeb+VrE Jd2NMQ21qfO00uwj/QFVlDOO3nY0txH1oaKHy9hb3dEOQrwmehaPxSvXiUiJvu1SV5YhJAi64+M vdzRMWZcRb48ePyC7j1Al56XKx99RrI/gPNEDE X-Gm-Gg: ASbGncsLkD7wlk4CEJTkdu2HbecfE3AfMYmDZ+5BF5sOdkQFwvZ0rI9wbHtIejho/NO sPOffm4gycFIzsbj9qVmQ+QqCFQISds2k7iWDRfmbC4riGwJeeWiMBo9ZWRdIW7ifhdwgBowJMP n7SvCFJG1/eCxYPYV3AGrVNDoEaYpWRBSuxAqij5eTWCiItPtdDKKejep7hK2kP1OG1LhD1qOXi /MrD6nxa58gMlP5ThhA67Dhu+VLeqIbe/ZYyOPM+kT9XlDQw2Td/l4yyy/duXwRKfGAav7KJrWw RkEAL8Z3Qi+O1G72m7a34IEQaCnYaItphTPoQcOV X-Received: by 2002:a5d:64e9:0:b0:3a4:db49:94aa with SMTP id ffacd0b85a97d-3a4db499627mr2535979f8f.21.1748269153902; Mon, 26 May 2025 07:19:13 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHRwk4Fn+3dHQYkjzT5HTQbx+7sRKkfJKakUrbGCreGdfbTRNTdtdKpaZHektOy6B4rfvuH1g== X-Received: by 2002:a5d:64e9:0:b0:3a4:db49:94aa with SMTP id ffacd0b85a97d-3a4db499627mr2535952f8f.21.1748269153351; Mon, 26 May 2025 07:19:13 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3a4d8d945e0sm3359128f8f.94.2025.05.26.07.19.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 May 2025 07:19:13 -0700 (PDT) Date: Mon, 26 May 2025 16:19:03 +0200 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v5 02/29] iov: Introduce iov_slice(), iov_tail_slice() and iov_tail_drop(). Message-ID: <20250526161903.170771ca@elisabeth> In-Reply-To: <20250417165136.2688884-3-lvivier@redhat.com> References: <20250417165136.2688884-1-lvivier@redhat.com> <20250417165136.2688884-3-lvivier@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: bf37ZlrlYRLVKXuEZB7w2-GT3Ej_z6IfzqXhiHBDLWM_1748269154 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Message-ID-Hash: U4JPW4GZY6HX6RIRM4EDNMHQXJT5UZHU X-Message-ID-Hash: U4JPW4GZY6HX6RIRM4EDNMHQXJT5UZHU X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 17 Apr 2025 18:51:09 +0200 Laurent Vivier wrote: > iov_tail_drop() discards a header from the iov_tail, >=20 > iov_slice() makes a new iov referencing a subset of the data > in the original iov. >=20 > iov_tail_slice() is a wrapper for iov_slice() using iov_tail >=20 > Signed-off-by: Laurent Vivier > --- > iov.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > iov.h | 6 ++++ > 2 files changed, 98 insertions(+) >=20 > diff --git a/iov.c b/iov.c > index 8c63b7ea6e31..047fcbce7fcd 100644 > --- a/iov.c > +++ b/iov.c > @@ -156,6 +156,59 @@ size_t iov_size(const struct iovec *iov, size_t iov_= cnt) > =09return len; > } > =20 > +/** > + * iov_slice - Make a new iov referencing a subset of the data in the o= riginal iov Nit: iov_slice() - Make ... > + * > + * @dst_iov:=09 Pointer to the destination array of struct iovec describ= ing > + *=09=09 the scatter/gather I/O vector to copy to. >From here on, you start mentioning "copy", but, I realised later, there's no "deep" copy done here. Perhaps a mix of "shallow copy" or "to duplicate references to [...]" etc. would be less confusing? > + * @dst_iov_cnt: Maximum number of elements in the destination iov array= . > + * @iov:=09 Pointer to the source array of struct iovec describing > + *=09=09 the scatter/gather I/O vector to copy from. > + * @iov_cnt:=09 Maximum number of elements in the source iov array. > + * @offset:=09 Offset within the source iov from where copying should st= art. I would mention explicitly this is only for the first iov, otherwise it might look like it's a fixed offset that applies to all elements (...until you find the offset =3D 0 assignment below), say: * @offset:=09Offset of the first source iov from where copying should star= t > + * @bytes:=09 On input, total number of bytes to copy from @iov to @dst_= iov, > + * =09=09 if @bytes is NULL, copy all the content of @iov, > + * =09=09 on output actual number of bytes copied. It's nice to avoid one additional argument, but to me it looks quite convoluted, especially as I started reviewing the next patches. What about: * @len:=09Maximum byte count of @iov to map onto @dst_iov, 0 means unlimit= ed * @copied:=09If given, number of bytes referenced by @dst_iov, set on retu= rn > + * > + * Returns:=09 The number of elements successfully copied to the destina= tion > + *=09=09 iov array, a negative value if there is not enough room in the > + *=09=09 destination iov array > + */ > +/* cppcheck-suppress staticFunction */ > +ssize_t iov_slice(struct iovec *dst_iov, size_t dst_iov_cnt, > +=09=09 const struct iovec *iov, size_t iov_cnt, > +=09=09 size_t offset, size_t *bytes) > +{ > +=09unsigned int i, j; > +=09size_t remaining =3D bytes ? *bytes : SIZE_MAX; Declarations from longest to shortest. > + > +=09i =3D iov_skip_bytes(iov, iov_cnt, offset, &offset); > + > +=09/* create a new iov array referencing a subset of the source one */ ...that's what I would have expected, but actually, the destination iov is already there. Maybe (also in the function description) use "assign iov references" rather than "create" or "make a new iov"? > +=09for (j =3D 0; i < iov_cnt && j < dst_iov_cnt && remaining; i++) { 'j' progresses with 'i'. Strictly speaking, you could use a single counter, but it might complicate things later. Still, why shouldn't j be post-decremented together with i, say: =09for (j =3D 0; i < iov_cnt && j < dst_iov_cnt && remaining; i++, j++) { ? > +=09=09size_t len =3D MIN(remaining, iov[i].iov_len - offset); > + > +=09=09dst_iov[j].iov_base =3D (char *)iov[i].iov_base + offset; > +=09=09dst_iov[j].iov_len =3D len; > +=09=09j++; > +=09=09remaining -=3D len; > +=09=09offset =3D 0; > +=09} > +=09if (j =3D=3D dst_iov_cnt) { > +=09=09if (bytes) { > +=09=09=09if (remaining) > +=09=09=09=09return -1; > +=09=09} else if (i !=3D iov_cnt) { > +=09=09=09return -1; ...this looks subtle. I'm wondering: what if we skipped enough bytes, so that i !=3D iov_cnt but we're now referencing everything from dst_iov as expected? Say that we get as input: - iov[0]: abcd, iov[1]: efgh - offset: 4 - dst_iov_cnt: 1 - iov_cnt: 2 (it's iov[0] and iov[1]) now we have: - iov_dst[0]: efgh - j =3D=3D 1 - i =3D=3D 1, iov_cnt =3D=3D 2 ...but we did "copy" everything, right? Unless I'm missing something, we should store the result from iov_skip_bytes() (say, 'skipped'), and then subtract it here. Actually, the whole "error checking" here looks a bit fragile, and I wonder if we shouldn't rather always set 'remaining' to the remaining bytes (even if it's a bit of a waste, when bytes is NULL), then signal failure whenever remaining !=3D 0. > +=09=09} > +=09} > + > +=09if (bytes) > +=09=09*bytes -=3D remaining; So, assuming it's desired (I'm fairly sure it is from the next patches), 'bytes' is an upper limit, rather than the number of bytes we _must_ copy (as the current function comment would seem to imply). If it's not desired, then we should also return error if we "copied" less than 'bytes'. > + > +=09return j; > +} > + > /** > * iov_tail_prune() - Remove any unneeded buffers from an IOV tail > * @tail:=09IO vector tail (modified) > @@ -191,6 +244,21 @@ size_t iov_tail_size(struct iov_tail *tail) > =09return iov_size(tail->iov, tail->cnt) - tail->off; > } > =20 > +/** > + * iov_tail_drop() - Discard a header from an IOV tail Mixing 'header' with 'tail' makes this a bit confusing. Yes, these vectors are used for headers, but what about calling it simply 'item', to match the context of this function? > + * @tail:=09IO vector tail > + * @len:=09length to move the head of the tail > + * > + * Returns:=09true if the tail still contains any bytes, otherwise false > + */ > +/* cppcheck-suppress unusedFunction */ > +bool iov_tail_drop(struct iov_tail *tail, size_t len) > +{ > +=09tail->off =3D tail->off + len; > + > +=09return iov_tail_prune(tail); > +} > + > /** > * iov_peek_header_() - Get pointer to a header from an IOV tail > * @tail:=09IOV tail to get header from > @@ -247,3 +315,27 @@ void *iov_remove_header_(struct iov_tail *tail, size= _t len, size_t align) > =09tail->off =3D tail->off + len; > =09return p; > } > + > +/** > + * iov_tail_slice - Make a new iov referencing a subset of the data iov_tail_slice() - ... Same comment as above: we're just setting references in a passed iov, not really "making" one. > + *=09=09 in an iov_tail > + * > + * @dst_iov: Pointer to the destination array of struct iovec descri= bing > + *=09=09 the scatter/gather I/O vector to copy to. > + * @dst_iov_cnt: Maximum number of elements in the destination iov array= . > + * @tail:=09 Pointer to the source iov_tail > + * @bytes:=09 On input, total number of bytes to copy from @iov to @dst_= iov, > + * =09=09 if @bytes is NULL, copy all the content of @iov, > + * =09=09 on output actual number of bytes copied. > + * > + * Returns: The number of elements successfully copied to the desti= nation > + *=09=09 iov array, a negative value if there is not enough room in the > + *=09=09 destination iov array > + */ > +/* cppcheck-suppress unusedFunction */ > +int iov_tail_slice(struct iovec *dst_iov, size_t dst_iov_cnt, > + struct iov_tail *tail, size_t *bytes) > +{ > +=09return iov_slice(dst_iov, dst_iov_cnt, &tail->iov[0], tail->cnt, > +=09=09=09 tail->off, bytes); > +} > diff --git a/iov.h b/iov.h > index 9855bf0c0c32..809f84a2c0a0 100644 > --- a/iov.h > +++ b/iov.h > @@ -28,6 +28,9 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov= _cnt, > size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, > size_t offset, void *buf, size_t bytes); > size_t iov_size(const struct iovec *iov, size_t iov_cnt); > +ssize_t iov_slice(struct iovec *dst_iov, size_t dst_iov_cnt, > +=09=09 const struct iovec *iov, size_t iov_cnt, > +=09=09 size_t offset, size_t *bytes); > =20 > /* > * DOC: Theory of Operation, struct iov_tail > @@ -72,8 +75,11 @@ struct iov_tail { > =20 > bool iov_tail_prune(struct iov_tail *tail); > size_t iov_tail_size(struct iov_tail *tail); > +bool iov_tail_drop(struct iov_tail *tail, size_t len); > void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align); > void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align= ); > +int iov_tail_slice(struct iovec *dst_iov, size_t dst_iov_cnt, > +=09=09 struct iov_tail *tail, size_t *bytes); > =20 > /** > * IOV_PEEK_HEADER() - Get typed pointer to a header from an IOV tail --=20 Stefano