Re: [PATCH v4 4/5] iov: Add IOV_PUT_HEADER() and with_header() to write header data back to iov_tail

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

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

On Tue, Mar 24, 2026 at 08:44:40AM +0100, Laurent Vivier wrote:
> On 3/24/26 03:48, David Gibson wrote:
> > On Tue, Mar 24, 2026 at 01:41:18PM +1100, David Gibson wrote:
> > > On Mon, Mar 23, 2026 at 03:31:50PM +0100, Laurent Vivier wrote:
> > > > Add iov_put_header_() and its wrapper macro IOV_PUT_HEADER() as a
> > > > counterpart to IOV_PEEK_HEADER(). This writes header data back to an
> > > > iov_tail after modification. If the header pointer matches the
> > > > original iov buffer location, the data was already modified in place
> > > > and no copy is needed. Otherwise, it copies the data back using
> > > > iov_from_buf().
> > > > 
> > > > Add with_header(), a for-loop macro that combines IOV_PEEK_HEADER()
> > > > and IOV_PUT_HEADER() to allow modifying a header in place within a
> > > > block scope.
> > > > 
> > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > ---
> > > >   iov.c | 22 ++++++++++++++++++++++
> > > >   iov.h | 25 ++++++++++++++++++++++++-
> > > >   2 files changed, 46 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/iov.c b/iov.c
> > > > index 8134b8c9f988..7fc9c3c78a32 100644
> > > > --- a/iov.c
> > > > +++ b/iov.c
> > > > @@ -308,6 +308,28 @@ void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
> > > >   	return v;
> > > >   }
> > > > +/**
> > > > + * iov_put_header_() - Write header back to an IOV tail
> > > > + * @tail:	IOV tail to write header to
> > > > + * @v:		Pointer to header data to write
> > > > + * @len:	Length of header to write, in bytes
> > > > + *
> > > > + * Return: number of bytes written
> > > > + */
> > > > +/* cppcheck-suppress unusedFunction */
> > > > +size_t iov_put_header_(const struct iov_tail *tail, const void *v, size_t len)
> > > > +{
> > > > +	size_t l = len;
> > > > +
> > > > +	/* iov_peek_header_() already called iov_check_header() */
> > > > +	if ((char *)tail->iov[0].iov_base + tail->off != v)
> > > > +		l = iov_from_buf(tail->iov, tail->cnt, tail->off, v, len);
> > > > +
> > > > +	assert(l == len);
> > > > +
> > > > +	return l;
> > > > +}
> > > > +
> > > >   /**
> > > >    * iov_remove_header_() - Remove a header from an IOV tail
> > > >    * @tail:	IOV tail to remove header from (modified)
> > > > diff --git a/iov.h b/iov.h
> > > > index d295d05b3bab..4ce425ccdbe5 100644
> > > > --- a/iov.h
> > > > +++ b/iov.h
> > > > @@ -90,6 +90,7 @@ bool iov_tail_prune(struct iov_tail *tail);
> > > >   size_t iov_tail_size(struct iov_tail *tail);
> > > >   bool iov_drop_header(struct iov_tail *tail, size_t len);
> > > >   void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align);
> > > > +size_t iov_put_header_(const struct iov_tail *tail, const void *v, size_t len);
> > > >   void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align);
> > > >   ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt,
> > > >   		       struct iov_tail *tail);
> > > > @@ -112,6 +113,16 @@ ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt,
> > > >   					       sizeof(var_),		\
> > > >   					       __alignof__(var_))))
> > > > +/**
> > > > + * IOV_PUT_HEADER() - Write header back to an IOV tail
> > > > + * @tail_:	IOV tail to write header to
> > > > + * @var_:	Pointer to a variable containing the header data to write
> > > > + *
> > > > + * Return: number of bytes written
> > > > + */
> > > > +#define IOV_PUT_HEADER(tail_, var_)					\
> > > > +	(iov_put_header_((tail_), (var_), sizeof(*var_)))
> > > > +
> > > >   /**
> > > >    * IOV_REMOVE_HEADER() - Remove and return typed header from an IOV tail
> > > >    * @tail_:	IOV tail to remove header from (modified)
> > > > @@ -130,7 +141,8 @@ ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt,
> > > >   	((__typeof__(var_) *)(iov_remove_header_((tail_), &(var_),	\
> > > >   				    sizeof(var_), __alignof__(var_))))
> > > > -/** IOV_DROP_HEADER() - Remove a typed header from an IOV tail
> > > > +/**
> > > > + * 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
> > > >    *
> > > > @@ -138,4 +150,15 @@ ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt,
> > > >    */
> > > >   #define IOV_DROP_HEADER(tail_, type_) 	iov_drop_header((tail_), sizeof(type_))
> > > > +/**
> > > > + * with_header() - Execute a block on a given header
> > > > + * @type:	Data type of the header to modify
> > > 
> > > We already use __typeof__ in IOV_PEEK_HEADER(), so we should be able
> > > to use that to avoid explicitly passing the type.
> > > 
> > > > + * @hdr_:	Variable name to receive the header pointer
> > > > + * @tail_:	IOV tail to peek/put the header from/to
> > > > + */
> > > > +#define with_header(type_, hdr_, tail_)					\
> > > > +	for (type_ store_, *hdr_ = IOV_PEEK_HEADER(tail_, store_); 	\
> > > > +	     hdr_;							\
> > > > +	     IOV_PUT_HEADER(tail_, hdr_), hdr_ = NULL)
> > > > +
> > 
> > I know I suggested this, but looking at it now, I'm wondering if the
> > fact that you _must not_ alter the tail in the block is too
> > non-obvious a constraint :/.  In particular this means you can never
> > work with multiple headers at once, say:
> > 	with_header(iphdr) {
> > 		with_header(udphdr) {
> > 			...
> > 		}
> > 	}
> > 
> 
> You're right, but I don't think there is an easy way to stack them (we need
> to add a "rewind" function to do that or to save and store the tail state).
> I think it introduces unnecessary complexity.

Right, I tend to agree.

> Whereas using "with_header()" makes the code easier to read.

See, the fact that there's several vital, invisible constraints about
how you use it means I'm not sure that's really true any more.

> If we want to keep it simple, the easy way is to copy all the headers to a
> buffer (but we cannot have the size to copy without decoding the headers),
> update them and write them back. But if we do that we can have alignment
> warning coming from the compiler when we want to get a point to one of the
> headers (when we do things like udp_xxx(&headers->uh,...)

Few (do any?) paths both read and write the headers to/from iov; they
generally do one or the other.  The existing PEEK and REMOVE work well
on read-only paths.  For write-only paths we could instead always
build the header in a local outside the iov (separate local for each
header).  Then PUSH_HEADER onto the iov_tail at the right point, which
would do an unconditional iov_from_buf().

Yes, that means we sacrifice in-place acess even in cases we could use
it.  Honestly though, given the sizes of the headers, I'm not sure
that's actually going to be a win compared to the extra complexity and
conditionals we need for with_header().

-- 
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 --]