Re: [PATCH v11 1/4] tap: break out building of udp header from tap_udp4_send function

[Stefano Brivio <sbrivio@redhat.com>]

On Thu,  6 Mar 2025 13:00:03 -0500
Jon Maloy <jmaloy@redhat.com> wrote:

> We will need to build the UDP header at other locations than in function
> tap_udp4_send(), so we break that part out to a separate function.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
> v2: Fix to satisfy coverity. After feedback from S. Brivio
> ---
>  tap.c | 34 +++++++++++++++++++++++++++-------
>  tap.h |  5 +++++
>  2 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 44b0fc0..16e3761 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -163,7 +163,7 @@ static void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src,
>  }
>  
>  /**
> - * tap_udp4_send() - Send UDP over IPv4 packet
> + * tap_push_uh4() - Build UDPv4 header with checksum
>   * @c:		Execution context
>   * @src:	IPv4 source address
>   * @sport:	UDP source port
> @@ -171,16 +171,14 @@ static void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src,
>   * @dport:	UDP destination port
>   * @in:		UDP payload contents (not including UDP header)
>   * @dlen:	UDP payload length (not including UDP header)
> + *
> + * Return: pointer at which to write the packet's payload
>   */
> -void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
> +void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport,

This patch didn't update the function comment, so it now takes a 'uh'
parameter which is not obvious at all. At a first glance, one might say
it's an "input" header.

Please fix this.

>  		   struct in_addr dst, in_port_t dport,
>  		   const void *in, size_t dlen)
>  {
>  	size_t l4len = dlen + sizeof(struct udphdr);
> -	char buf[USHRT_MAX];
> -	struct iphdr *ip4h = tap_push_l2h(c, buf, ETH_P_IP);
> -	struct udphdr *uh = tap_push_ip4h(ip4h, src, dst, l4len, IPPROTO_UDP);
> -	char *data = (char *)(uh + 1);
>  	const struct iovec iov = {
>  		.iov_base = (void *)in,
>  		.iov_len = dlen
> @@ -191,8 +189,30 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
>  	uh->dest = htons(dport);
>  	uh->len = htons(l4len);
>  	csum_udp4(uh, src, dst, &payload);
> -	memcpy(data, in, dlen);
> +	return (char *)uh + sizeof(*uh);
> +}
>  
> +/**
> + * tap_udp4_send() - Send UDP over IPv4 packet
> + * @c:		Execution context
> + * @src:	IPv4 source address
> + * @sport:	UDP source port
> + * @dst:	IPv4 destination address
> + * @dport:	UDP destination port
> + * @in:	UDP payload contents (not including UDP header)
> + * @dlen:	UDP payload length (not including UDP header)
> + */
> +void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
> +		   struct in_addr dst, in_port_t dport,
> +		   const void *in, size_t dlen)
> +{
> +	size_t l4len = dlen + sizeof(struct udphdr);
> +	char buf[USHRT_MAX];
> +	struct iphdr *ip4h = tap_push_l2h(c, buf, ETH_P_IP);
> +	struct udphdr *uh = tap_push_ip4h(ip4h, src, dst, l4len, IPPROTO_UDP);
> +	char *data = tap_push_uh4(uh, src, sport, dst, dport, in, dlen);
> +
> +	memcpy(data, in, dlen);
>  	tap_send_single(c, buf, dlen + (data - buf));
>  }
>  
> diff --git a/tap.h b/tap.h
> index a476a12..0b3eb93 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -6,6 +6,8 @@
>  #ifndef TAP_H
>  #define TAP_H
>  
> +struct udphdr;
> +
>  /**
>   * struct tap_hdr - tap backend specific headers
>   * @vnet_len:	Frame length (for qemu socket transport)
> @@ -42,6 +44,9 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len)
>  		thdr->vnet_len = htonl(l2len);
>  }
>  
> +void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport,
> +		   struct in_addr dst, in_port_t dport,
> +		   const void *in, size_t dlen);
>  void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
>  		   struct in_addr dst, in_port_t dport,
>  		   const void *in, size_t dlen);

-- 
Stefano