From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202502 header.b=LOw6N6fR; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 78D8A5A026F for ; Thu, 03 Apr 2025 04:36:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1743647773; bh=PwafkChcvi5eB+zf9CaCB1ScotEcFRrYN819ijc6h3w=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LOw6N6fRYScQYdn5VMALtTsfXc5dIT7I+fwIV1OoXIoiF8L/dmeXLEhdzlZZtSwfi yQyyQ3S+45N5Jza5aGDIQY3k8Bdk3oQe9WuNAl6U3/PnDZ6rxKOe19vedJ4LSJK/fk e7wMYk5yaa0JMxdFf0LewmW5k29bIrr2y+PyvqwsgGaNRXXxcr3vaENxbdj75N1xkf IF3dbE0GJE28Hv9N3SBrKJjuqMWBb6QOAkiEo4CQ0B4mqYCFNjV26O3bh4hupZd87X xZbfoobOwNwZ9POikb41o1yY9MJ7Arrm6A8lVLzKj/uGPftdwLJ2Ga6YFQFtvQs/dn 9NDaw8zDSzNKQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ZSm7x2bsJz4x3p; Thu, 3 Apr 2025 13:36:13 +1100 (AEDT) Date: Thu, 3 Apr 2025 13:36:07 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 02/18] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER() Message-ID: References: <20250402172343.858187-1-lvivier@redhat.com> <20250402172343.858187-3-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4qbT7vtyb2o0uPcL" Content-Disposition: inline In-Reply-To: <20250402172343.858187-3-lvivier@redhat.com> Message-ID-Hash: J6VV2P6NYEART57LOJJ7GKI3DLF47ABD X-Message-ID-Hash: J6VV2P6NYEART57LOJJ7GKI3DLF47ABD X-MailFrom: dgibson@gandalf.ozlabs.org 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: --4qbT7vtyb2o0uPcL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Apr 02, 2025 at 07:23:27PM +0200, Laurent Vivier wrote: > Provide a temporary variable of the wanted type to store > the header if the memory in the iovec array is not contiguous. >=20 > Also introduce iov_tail_ptr(), iov_drop() and iov_copy() > for later use. Not sure if these belong in the same patch. > Signed-off-by: Laurent Vivier > --- > iov.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++---- > iov.h | 53 ++++++++++++++++------ > tcp_buf.c | 2 +- > 3 files changed, 163 insertions(+), 25 deletions(-) >=20 > diff --git a/iov.c b/iov.c > index 8c63b7ea6e31..d96fc2ab594b 100644 > --- a/iov.c > +++ b/iov.c > @@ -108,7 +108,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t i= ov_cnt, > * > * Returns: The number of bytes successfully copied. > */ > -/* cppcheck-suppress unusedFunction */ > +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ > size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, > size_t offset, void *buf, size_t bytes) > { > @@ -126,6 +126,7 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov= _cnt, > /* copying data */ > for (copied =3D 0; copied < bytes && i < iov_cnt; i++) { > size_t len =3D MIN(iov[i].iov_len - offset, bytes - copied); > + /* NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker) */ > memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset, > len); > copied +=3D len; > @@ -156,6 +157,45 @@ size_t iov_size(const struct iovec *iov, size_t iov_= cnt) > return len; > } > =20 > +/** > + * iov_copy - Copy data from one scatter/gather I/O vector (struct iovec= ) to > + * another. I think this function name is slightly misleading and the description is very misleading. What the function does is make a new iov referencing a subset of the data in the original iov. It doesn't copy the reference data - the dst iov will alias (some of) the same memory as the src iov. I'd suggest iov_slice() maybe? > + * @dst_iov: Pointer to the destination array of struct iovec descri= bing > + * the scatter/gather I/O vector to copy to. > + * @dst_iov_cnt: Number of elements in the destination iov array. Maybe worth emphasising that this is a maximum, we might not use all of the= m. > + * @iov: Pointer to the source array of struct iovec describing > + * the scatter/gather I/O vector to copy from. > + * @iov_cnt: Number of elements in the source iov array. > + * @offset: Offset within the source iov from where copying should = start. > + * @bytes: Total number of bytes to copy from iov to dst_iov. > + * > + * Returns: The number of elements successfully copied to the desti= nation > + * iov array. So, this is obviously useful, because it's effectively the "after" value of dst_iov. However, it doesn't necessarily distinguish the (AFAICT) three exit conditions: 1) "success"; iov_size(dst_iov) =3D=3D bytes afterwards 2) iov_size(dst_iov) < bytes, because we ran out of data in the source iov 3) iov_size(dst_iov) < bytes, because we needed more than @dst_iov_cnt "slots" to reference that much of @iov. On "failure", whether it's (2) or (3) seems like it could matter, so it would be nice if we can deduce that from the return value. > + */ > +/* cppcheck-suppress unusedFunction */ > +unsigned iov_copy(struct iovec *dst_iov, size_t dst_iov_cnt, > + const struct iovec *iov, size_t iov_cnt, > + size_t offset, size_t bytes) > +{ > + unsigned int i, j; > + > + i =3D iov_skip_bytes(iov, iov_cnt, offset, &offset); > + > + /* copying data */ > + for (j =3D 0; i < iov_cnt && j < dst_iov_cnt && bytes; i++) { > + size_t len =3D MIN(bytes, iov[i].iov_len - offset); > + > + dst_iov[j].iov_base =3D (char *)iov[i].iov_base + offset; > + dst_iov[j].iov_len =3D len; > + j++; > + bytes -=3D len; > + offset =3D 0; > + } > + > + return j; > +} > + > /** > * iov_tail_prune() - Remove any unneeded buffers from an IOV tail > * @tail: IO vector tail (modified) > @@ -192,7 +232,50 @@ size_t iov_tail_size(struct iov_tail *tail) > } > =20 > /** > - * iov_peek_header_() - Get pointer to a header from an IOV tail > + * iov_drop() - update head of the tail Cute phrasing, but kind of confusing. I think this wants similar phrasing to remove_header, since it's doing basically the same thing, but discarding rather than retreiving the data. > + * @tail: IO vector tail > + * @len: length to move the head of the tail > + * > + * Returns: true if the tail still contains any bytes, otherwise false > + */ > +/* cppcheck-suppress unusedFunction */ > +bool iov_drop(struct iov_tail *tail, size_t len) > +{ > + tail->off =3D tail->off + len; > + > + return iov_tail_prune(tail); > +} > + > +/** > + * iov_tail_ptr() - get a pointer to the head of the tail > + * @tail: IO vector tail > + * @off: Byte offset in the tail to skip > + * @len: Length of the data to get, in bytes > + * > + * Returns: pointer to the data in the tail, NULL if the > + * tail doesn't contains @len bytes. This is a dangerous interface, because it returns a pointer with no indication of how many bytes after it are actually valid. I can see it being useful internally, but I don't think it should be exported. > + */ > +/* cppcheck-suppress unusedFunction */ > +void *iov_tail_ptr(struct iov_tail *tail, size_t off, size_t len) > +{ > + const struct iovec *iov; > + size_t cnt, i; > + > + i =3D iov_skip_bytes(tail->iov, tail->cnt, tail->off + off, &off); > + if (i =3D=3D tail->cnt) > + return NULL; > + > + iov =3D &tail->iov[i]; > + cnt =3D tail->cnt - i; > + > + if (iov_size(iov, cnt) < off + len) > + return NULL; > + > + return (char *)iov[0].iov_base + off; > +} > + > +/** > + * iov_check_header - Check if a header can be accessed > * @tail: IOV tail to get header from > * @len: Length of header to get, in bytes > * @align: Required alignment of header, in bytes > @@ -203,8 +286,7 @@ size_t iov_tail_size(struct iov_tail *tail) > * overruns the IO vector, is not contiguous or doesn't have the > * requested alignment. > */ > -/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ > -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) > +static void *iov_check_header(struct iov_tail *tail, size_t len, size_t = align) > { > char *p; > =20 > @@ -225,7 +307,36 @@ void *iov_peek_header_(struct iov_tail *tail, size_t= len, size_t align) > } > =20 > /** > - * iov_remove_header_() - Remove a header from an IOV tail > + * iov_peek_header_ - Get pointer to a header from an IOV tail > + * @tail: IOV tail to get header from Needs a description for @v. > + * @len: Length of header to get, in bytes > + * @align: Required alignment of header, in bytes > + * > + * @tail may be pruned, but will represent the same bytes as before. > + * > + * Returns: Pointer to the first @len logical bytes of the tail, or to > + * a copy if that overruns the IO vector, is not contiguous or > + * doesn't have the requested alignment. NULL if that overruns = the > + * IO vector. > + */ > +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ > +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_= t align) > +{ > + char *p =3D iov_check_header(tail, len, align); > + size_t l; > + > + if (p) > + return p; > + > + l =3D iov_to_buf(tail->iov, tail->cnt, tail->off, v, len); > + if (l !=3D len) > + return NULL; > + > + return v; > +} > + > +/** > + * iov_remove_header_ - Remove a header from an IOV tail > * @tail: IOV tail to remove header from (modified) Needs description of @v. > * @len: Length of header to remove, in bytes > * @align: Required alignment of header, in bytes > @@ -233,17 +344,19 @@ void *iov_peek_header_(struct iov_tail *tail, size_= t len, size_t align) > * On success, @tail is updated so that it longer includes the bytes of = the > * returned header. > * > - * Returns: Pointer to the first @len logical bytes of the tail, NULL if= that > - * overruns the IO vector, is not contiguous or doesn't have the > - * requested alignment. > + * Returns: Pointer to the first @len logical bytes of the tail, or to > + * a copy if that overruns the IO vector, is not contiguous or > + * doesn't have the requested alignment. NULL if that overruns = the > + * IO vector. > */ > -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align) > +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, siz= e_t align) > { > - char *p =3D iov_peek_header_(tail, len, align); > + char *p =3D iov_peek_header_(tail, v, len, align); > =20 > if (!p) > return NULL; > =20 > tail->off =3D tail->off + len; > + > return p; > } > diff --git a/iov.h b/iov.h > index 9855bf0c0c32..f9b5fdf0803e 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); > +unsigned iov_copy(struct iovec *dst_iov, size_t dst_iov_cnt, > + const struct iovec *iov, size_t iov_cnt, > + size_t offset, size_t bytes); > =20 > /* > * DOC: Theory of Operation, struct iov_tail > @@ -70,38 +73,60 @@ struct iov_tail { > #define IOV_TAIL(iov_, cnt_, off_) \ > (struct iov_tail){ .iov =3D (iov_), .cnt =3D (cnt_), .off =3D (off_) } > =20 > +/** > + * IOV_TAIL_FROM_BUF() - Create a new IOV tail from a buffer > + * @buf_: Buffer address to use in the iovec > + * @len_: Buffer size > + * @off_: Byte offset in the buffer where the tail begins > + */ > +#define IOV_TAIL_FROM_BUF(buf_, len_, off_) \ > + IOV_TAIL((&(const struct iovec){ .iov_base =3D (buf_), .iov_len =3D (le= n_) }), 1, (off_)) > + > bool iov_tail_prune(struct iov_tail *tail); > size_t iov_tail_size(struct iov_tail *tail); > -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= ); > +bool iov_drop(struct iov_tail *tail, size_t len); > +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_= t align); > +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, siz= e_t align); > +void *iov_tail_ptr(struct iov_tail *tail, size_t off, size_t len); > =20 > /** > * IOV_PEEK_HEADER() - Get typed pointer to a header from an IOV tail > * @tail_: IOV tail to get header from > - * @type_: Data type of the header > + * @var_: Temporary buffer of the type of the header to use if > + * the memory in the iovec array is not contiguous. > * > * @tail_ may be pruned, but will represent the same bytes as before. > * > - * Returns: Pointer of type (@type_ *) located at the start of @tail_, N= ULL if > - * we can't get a contiguous and aligned pointer. > + * Returns: Pointer of type (@type_ *) located at the start of @tail_ This is no longer quite accurate: the data is the same, but it's not necessarily a pointer into @tail_ proper. There's also still a NULL return case, just more limited than it was before. > */ > -#define IOV_PEEK_HEADER(tail_, type_) \ > - ((type_ *)(iov_peek_header_((tail_), \ > - sizeof(type_), __alignof__(type_)))) > + > +#define IOV_PEEK_HEADER(tail_, var_) \ > + ((__typeof__(var_) *)(iov_peek_header_((tail_), &(var_), \ > + sizeof(var_), __alignof__(var_)))) > =20 > /** > * IOV_REMOVE_HEADER() - Remove and return typed header from an IOV tail > * @tail_: IOV tail to remove header from (modified) > - * @type_: Data type of the header to remove > + * @var_: Temporary buffer of the type of the header to use if > + * the memory in the iovec array is not contiguous. > * > * On success, @tail_ is updated so that it longer includes the bytes of= the > * returned header. > * > - * Returns: Pointer of type (@type_ *) located at the old start of @tail= _, NULL > - * if we can't get a contiguous and aligned pointer. > + * Returns: Pointer of type (@type_ *) located at the old start of @tail_ > + */ > + > +#define IOV_REMOVE_HEADER(tail_, var_) \ > + ((__typeof__(var_) *)(iov_remove_header_((tail_), &(var_), \ > + sizeof(var_), __alignof__(var_)))) > + > +/** IOV_DROP_HEADER() -- Remove a typed header from an IOV tail > + * @tail_: IOV tail to remove header from (modified) > + * @type_: Data type of the header to remove > + * > + * Returns: true if the tail still contains any bytes, otherwise false > */ > -#define IOV_REMOVE_HEADER(tail_, type_) \ > - ((type_ *)(iov_remove_header_((tail_), \ > - sizeof(type_), __alignof__(type_)))) > +#define IOV_DROP_HEADER(tail_, type_) \ > + iov_drop((tail_), sizeof(type_)) > =20 > #endif /* IOVEC_H */ > diff --git a/tcp_buf.c b/tcp_buf.c > index 05305636b503..4bcc1acb245a 100644 > --- a/tcp_buf.c > +++ b/tcp_buf.c > @@ -160,7 +160,7 @@ static void tcp_l2_buf_fill_headers(const struct tcp_= tap_conn *conn, > uint32_t seq, bool no_tcp_csum) > { > struct iov_tail tail =3D IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); > - struct tcphdr *th =3D IOV_REMOVE_HEADER(&tail, struct tcphdr); > + struct tcphdr thc, *th =3D IOV_REMOVE_HEADER(&tail, thc); > struct tap_hdr *taph =3D iov[TCP_IOV_TAP].iov_base; > const struct flowside *tapside =3D TAPFLOW(conn); > const struct in_addr *a4 =3D inany_v4(&tapside->oaddr); --=20 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 --4qbT7vtyb2o0uPcL Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmft9BcACgkQzQJF27ox 2GdCUBAAm7B3w3VhzzvXkqzngqHnKQKeErqdjbLLlV1MsO440PMM6PaCn1bb+786 FJCfBIGZkL2LedyCpzt+kbaExZmp0bVNS+4cnztBYWRZDZUv3RAbA5wo30IPq4Jo ujSvglwTabLR4B4nR4TGWHdn3l0PYMdPr5tcX3E2cnzvgecHL/utN1SN00w4poJL Jq6TxSDZ+BzOH3tj7jMqm76sIrUBSvyLWJeeGHzhX0AdgGfJhoFy4kKoArBCbWZo NTBsMCeN5jofxI4bdNWLgaoC9SO9tJXdQcSdFr8EsDtwXESbqpfIshPQwmNu8LxC b1/UrzgGFzYwdlUmDgpJVxV1/pa4IfQs+DqMRIlZlRZ6CFvXhFTGSE/Xi1M0v4u7 4UIC84tdFNKpWSqQ/d6bbplT01VS5lFFop2wqIrUXVp00MsqekHoh5vfx1ASACDd +DRhL3oL+hWY7V+c1Mw0jxSimzLjoLA0WqOdfivPHKkHpzlViJJ7aApmzYJA1Kg2 MltLWvxTDRq1Ldb1ia2bAE3LLYVxxaDmqef1WfQWXzKc2StWyKZZKYby8j/1fl7F FbaCA3iTs0ZrMNc/Ent+MxOpaaErcVSYGeUA97v7N4NKOFTpeNGtdOsdVq1nwxrP Y7cbSrTHrYEXhfWKR/tr3eHG9gys+MALnnlU08lgNX9SdPXT62A= =p1g4 -----END PGP SIGNATURE----- --4qbT7vtyb2o0uPcL--