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

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