|SUCCESS| [v3] net: fix VLAN packet type

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163234

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-22

git sha: (not provided in patch - this is from patchwork)
Author: Robin Jarry <rjarry at redhat.com>
Subject: [PATCH dpdk v3] net: fix VLAN packet type

This patch fixes an issue in rte_net_get_ptype() where single VLAN
frames are incorrectly identified as QinQ due to a bitwise-or conflict
between L2 ptype constants during VLAN parsing.

> diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
> index 458b4814a9c9..0228f1eb2f18 100644
> --- a/lib/net/rte_net.c
> +++ b/lib/net/rte_net.c
> @@ -331,8 +331,8 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  	struct rte_net_hdr_lens local_hdr_lens;
>  	const struct rte_ether_hdr *eh;
>  	struct rte_ether_hdr eh_copy;
> -	uint32_t pkt_type = RTE_PTYPE_L2_ETHER;
>  	uint32_t off = 0, vlan_depth = 0;
> +	uint32_t pkt_type = 0;
>  	uint16_t proto;
>  	int ret;

The variable declarations should follow the reverse xmas tree style as
documented in the coding style guide. After this change, the declaration
of pkt_type comes after off/vlan_depth. Does this violate the expected
ordering where longer lines should come first?

[ ... ]

> @@ -392,6 +392,8 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  	}
>  
>  l3:
> +	if (pkt_type == 0)
> +		pkt_type = RTE_PTYPE_L2_ETHER;

According to the coding style guide section on comparisons, should this
use an explicit comparison operator? The guide states "Do not use ! for
tests unless it is a boolean" and shows preference for explicit
comparisons like "if (p != NULL)". Should this be written as:

	if (pkt_type != 0)

or would the current form be acceptable since pkt_type is an integer
type?

>  	if ((layers & RTE_PTYPE_L3_MASK) == 0)
>  		return pkt_type;

[ ... ]

> diff --git a/app/test/test_net_ptype.c b/app/test/test_net_ptype.c
> new file mode 100644
> index 000000000000..bfe85da13543
> --- /dev/null
> +++ b/app/test/test_net_ptype.c

[ ... ]

> +static int
> +test_get_ptype(struct rte_mempool *pool, const char *name,
> +	       const uint8_t *pktdata, size_t len, uint32_t expected_ptype,
> +	       uint8_t expected_l2_len, uint16_t expected_l3_len,
> +	       uint8_t expected_l4_len)
> +{
> +	struct rte_net_hdr_lens hdr_lens;
> +	struct rte_mbuf *m;
> +	uint32_t ptype;
> +	char *data;
> +
> +	RTE_LOG(INFO, EAL, "%s: %s\n", __func__, name);
> +
> +	m = rte_pktmbuf_alloc(pool);
> +	RTE_TEST_ASSERT_NOT_NULL(m, "cannot allocate mbuf");
> +
> +	data = rte_pktmbuf_append(m, len);
> +	if (data == NULL) {
> +		rte_pktmbuf_free(m);
> +		RTE_TEST_ASSERT_NOT_NULL(data, "cannot append data");
> +	}

Does this code leak the mbuf m if RTE_TEST_ASSERT_NOT_NULL triggers?
RTE_TEST_ASSERT_NOT_NULL appears to be a macro that returns from the
function on failure. After rte_pktmbuf_free(m), if the assertion fails,
does the function return without any additional cleanup being needed?
Looking at the code flow, rte_pktmbuf_free() has already been called
before the assertion, so this should be safe, but the pattern is
unusual.

> +
> +	memcpy(data, pktdata, len);
> +
> +	memset(&hdr_lens, 0, sizeof(hdr_lens));
> +	ptype = rte_net_get_ptype(m, &hdr_lens, RTE_PTYPE_ALL_MASK);
> +
> +	rte_pktmbuf_free(m);
> +
> +	RTE_TEST_ASSERT_EQUAL(ptype, expected_ptype,
> +		"unexpected ptype: got 0x%x, expected 0x%x",
> +		ptype, expected_ptype);
> +	RTE_TEST_ASSERT_EQUAL(hdr_lens.l2_len, expected_l2_len,
> +		"unexpected l2_len: got %u, expected %u",
> +		hdr_lens.l2_len, expected_l2_len);
> +	RTE_TEST_ASSERT_EQUAL(hdr_lens.l3_len, expected_l3_len,
> +		"unexpected l3_len: got %u, expected %u",
> +		hdr_lens.l3_len, expected_l3_len);
> +	RTE_TEST_ASSERT_EQUAL(hdr_lens.l4_len, expected_l4_len,
> +		"unexpected l4_len: got %u, expected %u",
> +		hdr_lens.l4_len, expected_l4_len);
> +
> +	return 0;
> +}

[ ... ]