Re: [PATCH v5 1/8] tcp: extract buffer management from tcp_send_flag()

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

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

On Tue, Jun 11, 2024 at 01:42:20PM +0200, Laurent Vivier wrote:
> On 11/06/2024 07:31, David Gibson wrote:
> > On Wed, Jun 05, 2024 at 05:21:22PM +0200, Laurent Vivier wrote:
> > > This commit isolates the internal data structure management used for storing
> > > data (e.g., tcp4_l2_flags_iov[], tcp6_l2_flags_iov[], tcp4_flags_ip[],
> > > tcp4_flags[], ...) from the tcp_send_flag() function. The extracted
> > > functionality is relocated to a new function named tcp_fill_flag_header().
> > > 
> > > tcp_fill_flag_header() is now a generic function that accepts parameters such
> > > as struct tcphdr and a data pointer. tcp_send_flag() utilizes this parameter to
> > > pass memory pointers from tcp4_l2_flags_iov[] and tcp6_l2_flags_iov[].
> > > 
> > > This separation sets the stage for utilizing tcp_fill_flag_header() to
> > > set the memory provided by the guest via vhost-user in future developments.
> > 
> > Thanks for the commit message, it makes this much clearer.
> > 
> > I have a number of comments below, but they're basically all cosmetic.
> > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > >   tcp.c | 63 ++++++++++++++++++++++++++++++++++++-----------------------
> > >   1 file changed, 39 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/tcp.c b/tcp.c
> > > index 06acb41e4d90..68d4afa05a36 100644
> > > --- a/tcp.c
> > > +++ b/tcp.c
> > > @@ -1549,24 +1549,25 @@ static void tcp_update_seqack_from_tap(const struct ctx *c,
> > >   }
> > >   /**
> > > - * tcp_send_flag() - Send segment with flags to tap (no payload)
> > > + * tcp_fill_flag_header() - Prepare header for flags-only segment (no payload)
> > 
> > I don't love the name tcp_fill_flag_header(), although it's not
> > terrible.  Maybe tcp_prepare_flags() would be better?
> > 
> > >    * @c:		Execution context
> > >    * @conn:	Connection pointer
> > >    * @flags:	TCP flags: if not set, send segment only if ACK is due
> > > + * @th:		TCP header to update
> > > + * @data:	buffer to store TCP option
> > > + * @optlen:	size of the TCP option buffer
> > 
> > Worth noting this is an output parameter here...
> > 
> > >    *
> > > - * Return: negative error code on connection reset, 0 otherwise
> > > + * Return: < 0 error code on connection reset,
> > > + *           0 if there is no flag to send
> > > + *	     1 otherwise
> > 
> > .. or, since optlen will always be positive on success cases, you
> > could just return it.
> > 
> 
> We cannot return optlen here as optlen can be 0 (it is not zero only with
> SYN), and 0 means no flags to send. We can have flags to send with optlen
> equal to 0.

Oh, my mistake, sorry.  We could change it to the l4len which would
avoid that, but it looks like that would be more awkward in the
caller.

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