public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: Laurent Vivier <lvivier@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v5 4/8] udp: refactor UDP header update functions
Date: Wed, 12 Jun 2024 00:10:17 +0200	[thread overview]
Message-ID: <20240612001017.31692649@elisabeth> (raw)
In-Reply-To: <20240605152129.1641658-5-lvivier@redhat.com>

On Wed,  5 Jun 2024 17:21:25 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> This commit refactors the udp_update_hdr4() and udp_update_hdr6() functions
> to improve code portability by replacing the udp_meta_t parameter with
> more specific parameters for the IPv4 and IPv6 headers (iphdr/ipv6hdr)
> and the source socket address (sockaddr_in/sockaddr_in6).
> It also moves the tap_hdr_update() function call inside the udp_tap_send()
> function not to have to pass the TAP header to udp_update_hdr4() and
> udp_update_hdr6()
> 
> This refactor reduces complexity by making the functions more modular and
> ensuring that each function operates on more narrowly scoped data structures.
> This will facilitate future backend introduction like vhost-user.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  udp.c | 60 +++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index 3abafc994537..4295d48046a6 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -556,7 +556,8 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
>  /**
>   * udp_update_hdr4() - Update headers for one IPv4 datagram
>   * @c:		Execution context
> - * @bm:		Pointer to udp_meta_t to update
> + * @ip4h:	Pre-filled IPv4 header (except for tot_len and saddr)
> + * @s_in:	Source socket address, filled in by recvmmsg()
>   * @bp:		Pointer to udp_payload_t to update
>   * @dstport:	Destination port number
>   * @dlen:	Length of UDP payload
> @@ -565,15 +566,16 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
>   * Return: size of IPv4 payload (UDP header + data)
>   */
>  static size_t udp_update_hdr4(const struct ctx *c,
> -			      struct udp_meta_t *bm, struct udp_payload_t *bp,
> +			      struct iphdr *ip4h, const struct sockaddr_in *s_in,
> +			      struct udp_payload_t *bp,
>  			      in_port_t dstport, size_t dlen,
>  			      const struct timespec *now)
>  {
> -	in_port_t srcport = ntohs(bm->s_in.sa4.sin_port);
> +	in_port_t srcport = ntohs(s_in->sin_port);
>  	const struct in_addr dst = c->ip4.addr_seen;
> -	struct in_addr src = bm->s_in.sa4.sin_addr;
> +	struct in_addr src = s_in->sin_addr;
>  	size_t l4len = dlen + sizeof(bp->uh);
> -	size_t l3len = l4len + sizeof(bm->ip4h);
> +	size_t l3len = l4len + sizeof(*ip4h);

Nit: while at it, it would be nice to reorder declarations from longest
to shortest, as well.

>  
>  	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
>  	    IN4_ARE_ADDR_EQUAL(&src, &c->ip4.dns_host) && srcport == 53 &&
> @@ -594,24 +596,24 @@ static size_t udp_update_hdr4(const struct ctx *c,
>  		src = c->ip4.gw;
>  	}
>  
> -	bm->ip4h.tot_len = htons(l3len);
> -	bm->ip4h.daddr = dst.s_addr;
> -	bm->ip4h.saddr = src.s_addr;
> -	bm->ip4h.check = csum_ip4_header(l3len, IPPROTO_UDP, src, dst);
> +	ip4h->tot_len = htons(l3len);
> +	ip4h->daddr = dst.s_addr;
> +	ip4h->saddr = src.s_addr;
> +	ip4h->check = csum_ip4_header(l3len, IPPROTO_UDP, src, dst);
>  
> -	bp->uh.source = bm->s_in.sa4.sin_port;
> +	bp->uh.source = s_in->sin_port;
>  	bp->uh.dest = htons(dstport);
>  	bp->uh.len = htons(l4len);
>  	csum_udp4(&bp->uh, src, dst, bp->data, dlen);
>  
> -	tap_hdr_update(&bm->taph, l3len + sizeof(udp4_eth_hdr));
>  	return l4len;
>  }
>  
>  /**
>   * udp_update_hdr6() - Update headers for one IPv6 datagram
>   * @c:		Execution context
> - * @bm:		Pointer to udp_meta_t to update
> + * @ip6h:	Pre-filled IPv6 header (except for payload_len and addresses)
> + * @s_in:	Source socket address, filled in by recvmmsg()
>   * @bp:		Pointer to udp_payload_t to update
>   * @dstport:	Destination port number
>   * @dlen:	Length of UDP payload
> @@ -620,13 +622,14 @@ static size_t udp_update_hdr4(const struct ctx *c,
>   * Return: size of IPv6 payload (UDP header + data)
>   */
>  static size_t udp_update_hdr6(const struct ctx *c,
> -			      struct udp_meta_t *bm, struct udp_payload_t *bp,
> +			      struct ipv6hdr *ip6h, struct sockaddr_in6 *s_in6,
> +			      struct udp_payload_t *bp,
>  			      in_port_t dstport, size_t dlen,
>  			      const struct timespec *now)
>  {
> -	const struct in6_addr *src = &bm->s_in.sa6.sin6_addr;
> +	const struct in6_addr *src = &s_in6->sin6_addr;
>  	const struct in6_addr *dst = &c->ip6.addr_seen;
> -	in_port_t srcport = ntohs(bm->s_in.sa6.sin6_port);
> +	in_port_t srcport = ntohs(s_in6->sin6_port);

Same here.

>  	uint16_t l4len = dlen + sizeof(bp->uh);
>  
>  	if (IN6_IS_ADDR_LINKLOCAL(src)) {
> @@ -663,19 +666,18 @@ static size_t udp_update_hdr6(const struct ctx *c,
>  
>  	}
>  
> -	bm->ip6h.payload_len = htons(l4len);
> -	bm->ip6h.daddr = *dst;
> -	bm->ip6h.saddr = *src;
> -	bm->ip6h.version = 6;
> -	bm->ip6h.nexthdr = IPPROTO_UDP;
> -	bm->ip6h.hop_limit = 255;
> +	ip6h->payload_len = htons(l4len);
> +	ip6h->daddr = *dst;
> +	ip6h->saddr = *src;
> +	ip6h->version = 6;
> +	ip6h->nexthdr = IPPROTO_UDP;
> +	ip6h->hop_limit = 255;
>  
> -	bp->uh.source = bm->s_in.sa6.sin6_port;
> +	bp->uh.source = s_in6->sin6_port;
>  	bp->uh.dest = htons(dstport);
> -	bp->uh.len = bm->ip6h.payload_len;
> +	bp->uh.len = ip6h->payload_len;
>  	csum_udp6(&bp->uh, src, dst, bp->data, dlen);
>  
> -	tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) + sizeof(udp6_eth_hdr));
>  	return l4len;
>  }
>  
> @@ -708,11 +710,17 @@ static void udp_tap_send(const struct ctx *c,
>  		size_t l4len;
>  
>  		if (v6) {
> -			l4len = udp_update_hdr6(c, bm, bp, dstport,
> +			l4len = udp_update_hdr6(c, &bm->ip6h,
> +						&bm->s_in.sa6, bp, dstport,
>  						udp6_l2_mh_sock[i].msg_len, now);
> +			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) +
> +					     sizeof(udp6_eth_hdr));

You're summing sizeof(udp6_eth_hdr) to l4len + sizeof(bm->ip6h), so
this should be aligned as follows:

			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) +
						  sizeof(udp6_eth_hdr));

>  		} else {
> -			l4len = udp_update_hdr4(c, bm, bp, dstport,
> +			l4len = udp_update_hdr4(c, &bm->ip4h,
> +						&bm->s_in.sa4, bp, dstport,
>  						udp4_l2_mh_sock[i].msg_len, now);
> +			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip4h) +
> +					     sizeof(udp4_eth_hdr));

Same here:
			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip4h) +
						  sizeof(udp4_eth_hdr));

>  		}
>  		tap_iov[i][UDP_IOV_PAYLOAD].iov_len = l4len;
>  	}

-- 
Stefano


  reply	other threads:[~2024-06-11 22:10 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-05 15:21 [PATCH v5 0/8] Add vhost-user support to passt (part 2) Laurent Vivier
2024-06-05 15:21 ` [PATCH v5 1/8] tcp: extract buffer management from tcp_send_flag() Laurent Vivier
2024-06-11  5:31   ` David Gibson
2024-06-11 11:42     ` Laurent Vivier
2024-06-12  6:31       ` David Gibson
2024-06-11 22:09   ` Stefano Brivio
2024-06-12  6:32     ` David Gibson
2024-06-05 15:21 ` [PATCH v5 2/8] tcp: move buffers management functions to their own file Laurent Vivier
2024-06-11 22:09   ` Stefano Brivio
2024-06-12  6:14   ` David Gibson
2024-06-12 12:03     ` Stefano Brivio
2024-06-05 15:21 ` [PATCH v5 3/8] tap: refactor packets handling functions Laurent Vivier
2024-06-11 22:09   ` Stefano Brivio
2024-06-12  6:18     ` David Gibson
2024-06-12  6:34       ` Stefano Brivio
2024-06-12  6:37         ` David Gibson
2024-06-12  6:21   ` David Gibson
2024-06-05 15:21 ` [PATCH v5 4/8] udp: refactor UDP header update functions Laurent Vivier
2024-06-11 22:10   ` Stefano Brivio [this message]
2024-06-12  6:27   ` David Gibson
2024-06-05 15:21 ` [PATCH v5 5/8] udp: rename udp_sock_handler() to udp_buf_sock_handler() Laurent Vivier
2024-06-12  6:28   ` David Gibson
2024-06-05 15:21 ` [PATCH v5 6/8] vhost-user: compare mode MODE_PASTA and not MODE_PASST Laurent Vivier
2024-06-11 22:10   ` Stefano Brivio
2024-06-05 15:21 ` [PATCH v5 7/8] iov: remove iov_copy() Laurent Vivier
2024-06-05 15:21 ` [PATCH v5 8/8] tap: use in->buf_size rather than sizeof(pkt_buf) Laurent Vivier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240612001017.31692649@elisabeth \
    --to=sbrivio@redhat.com \
    --cc=lvivier@redhat.com \
    --cc=passt-dev@passt.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).