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

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. Thanks, Laurent