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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  5 Jun 2024 17:21:22 +0200
Laurent Vivier <lvivier@redhat.com> 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.
> 
> 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)
>   * @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

Now, this becomes an output parameter if SYN is set in flags, but it's
otherwise an input parameter (and it must be zero, otherwise the data
offset field we send will be wrong).

I think having it always as output parameter (that is, setting it to
zero on non-SYN in this function, not in the caller) would be less
fragile and easier to describe in the comment, too.

Or, even simpler, pass it as input parameter, and calculate it in the
caller. The caller sets 'flags' anyway.

>   *
> - * Return: negative error code on connection reset, 0 otherwise
> + * Return: < 0 error code on connection reset,
> + *           0 if there is no flag to send

As you use one tab to indent "1 otherwise" below, you could use one
here as well.

> + *	     1 otherwise
>   */
> -static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
> +static int tcp_fill_flag_header(struct ctx *c, struct tcp_tap_conn *conn,
> +				int flags, struct tcphdr *th, char *data,
> +				size_t *optlen)
>  {
> -	struct tcp_flags_t *payload;
>  	struct tcp_info tinfo = { 0 };
>  	socklen_t sl = sizeof(tinfo);
>  	int s = conn->sock;
> -	size_t optlen = 0;
> -	struct tcphdr *th;
> -	struct iovec *iov;
> -	size_t l4len;
> -	char *data;
>  
>  	if (SEQ_GE(conn->seq_ack_to_tap, conn->seq_from_tap) &&
>  	    !flags && conn->wnd_to_tap)
> @@ -1588,20 +1589,11 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	if (!tcp_update_seqack_wnd(c, conn, flags, &tinfo) && !flags)
>  		return 0;
>  
> -	if (CONN_V4(conn))
> -		iov = tcp4_l2_flags_iov[tcp4_flags_used++];
> -	else
> -		iov = tcp6_l2_flags_iov[tcp6_flags_used++];
> -
> -	payload = iov[TCP_IOV_PAYLOAD].iov_base;
> -	th = &payload->th;
> -	data = payload->opts;
> -
>  	if (flags & SYN) {
>  		int mss;
>  
>  		/* Options: MSS, NOP and window scale (8 bytes) */
> -		optlen = OPT_MSS_LEN + 1 + OPT_WS_LEN;
> +		*optlen = OPT_MSS_LEN + 1 + OPT_WS_LEN;
>  
>  		*data++ = OPT_MSS;
>  		*data++ = OPT_MSS_LEN;
> @@ -1635,17 +1627,13 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  		flags |= ACK;
>  	}
>  
> -	th->doff = (sizeof(*th) + optlen) / 4;
> +	th->doff = (sizeof(*th) + *optlen) / 4;
>  
>  	th->ack = !!(flags & ACK);
>  	th->rst = !!(flags & RST);
>  	th->syn = !!(flags & SYN);
>  	th->fin = !!(flags & FIN);
>  
> -	l4len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL,
> -					conn->seq_to_tap);
> -	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
> -
>  	if (th->ack) {
>  		if (SEQ_GE(conn->seq_ack_to_tap, conn->seq_from_tap))
>  			conn_flag(c, conn, ~ACK_TO_TAP_DUE);
> @@ -1660,6 +1648,33 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	if (th->fin || th->syn)
>  		conn->seq_to_tap++;
>  
> +	return 1;
> +}
> +
> +static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
> +{
> +	struct tcp_flags_t *payload;
> +	size_t optlen = 0;
> +	struct iovec *iov;
> +	size_t l4len;
> +	int ret;
> +
> +	if (CONN_V4(conn))
> +		iov = tcp4_l2_flags_iov[tcp4_flags_used++];
> +	else
> +		iov = tcp6_l2_flags_iov[tcp6_flags_used++];
> +
> +	payload = iov[TCP_IOV_PAYLOAD].iov_base;
> +
> +	ret = tcp_fill_flag_header(c, conn, flags, &payload->th,
> +				   payload->opts, &optlen);
> +	if (ret <= 0)
> +		return ret;
> +
> +	l4len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL,
> +					conn->seq_to_tap);
> +	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
> +
>  	if (flags & DUP_ACK) {
>  		struct iovec *dup_iov;
>  		int i;

-- 
Stefano