RE: [PATCH dpdk v2 16/16] ipv6: add function to check ipv6 version

[Morten Brørup]

> From: Robin Jarry [mailto:rja...@redhat.com]
> Sent: Tuesday, 1 October 2024 10.17
> 
> This is a slow path version to verify the version field in IPv6
> headers.

It doesn't look slow to me. No need to mention slow path here.

> 
> Signed-off-by: Robin Jarry <rja...@redhat.com>
> ---
> 
> Notes:
>     v2: new patch
> 
>  app/test/test_net_ipv6.c | 16 ++++++++++++++++
>  lib/net/rte_ip6.h        | 17 +++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/app/test/test_net_ipv6.c b/app/test/test_net_ipv6.c
> index b087b5c60d73..79a43a2b5491 100644
> --- a/app/test/test_net_ipv6.c
> +++ b/app/test/test_net_ipv6.c
> @@ -11,6 +11,21 @@ static const struct rte_ipv6_addr bcast_addr = {
>  };
>  static const struct rte_ipv6_addr zero_addr = { 0 };
> 
> +static int
> +test_ipv6_check_version(void)
> +{
> +     struct rte_ipv6_hdr h;
> +
> +     h.vtc_flow = 0;
> +     TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
> +     h.vtc_flow = RTE_BE32(0x7f00ba44);
> +     TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
> +     h.vtc_flow = RTE_BE32(0x6badcaca);
> +     TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), 0, "");
> +
> +     return 0;
> +}
> +
>  static int
>  test_ipv6_addr_mask(void)
>  {
> @@ -191,6 +206,7 @@ test_ether_mcast_from_ipv6(void)
>  static int
>  test_net_ipv6(void)
>  {
> +     TEST_ASSERT_SUCCESS(test_ipv6_check_version(), "");
>       TEST_ASSERT_SUCCESS(test_ipv6_addr_mask(), "");
>       TEST_ASSERT_SUCCESS(test_ipv6_addr_eq_prefix(), "");
>       TEST_ASSERT_SUCCESS(test_ipv6_addr_kind(), "");
> diff --git a/lib/net/rte_ip6.h b/lib/net/rte_ip6.h
> index c552fa54c095..de3ddc0348c5 100644
> --- a/lib/net/rte_ip6.h
> +++ b/lib/net/rte_ip6.h
> @@ -323,6 +323,23 @@ struct rte_ipv6_hdr {
>       struct rte_ipv6_addr dst_addr;  /**< IP address of
> destination host(s). */
>  } __rte_packed;
> 
> +#define RTE_IPV6_VTC_FLOW_VERSION RTE_BE32(0x60000000)
> +#define RTE_IPV6_VTC_FLOW_VERSION_MASK RTE_BE32(0xf0000000)
> +
> +/**
> + * Check that the IPv6 header version field is valid according to RFC
> 8200 section 3.
> + *
> + * @return
> + *   0 if the version field is valid. -EINVAL otherwise.
> + */
> +static inline int rte_ipv6_check_version(const struct rte_ipv6_hdr
> *ip)
> +{
> +     rte_be32_t v = ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK;
> +     if (v != RTE_IPV6_VTC_FLOW_VERSION)
> +             return -EINVAL;
> +     return 0;
> +}

Personally, I would prefer following the convention of rte_ether functions to 
return boolean (as int)...

static inline int 
rte_is_<zero/unicast/multicast/broadcast/universal/local_admin/valid_assigned>_ether_addr(const
 struct rte_ether_addr *ea)

static inline int rte_is_ipv6_version(const struct rte_ipv6_hdr *ip)
{
        return ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK == 
RTE_IPV6_VTC_FLOW_VERSION;
}

Or, at your preference, an optimized variant:
static inline int rte_is_version_ipv6(const struct rte_ipv6_hdr *ip)
{
        return *(const unsigned char *)ip & 0xf0 == 0x60;
}

The first nibble is also used for version in IPv4, so an IPv4 variant would 
look similar:
static inline int rte_is_version_ipv4(const struct rte_ip_hdr *ip)
{
        return *(const unsigned char *)ip & 0xf0 == 0x40;
}