Re: [PATCH 1/3] net: fix VLAN insert doc comment for shared-mbuf error code

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 24 Aug 2026 15:22:25 -0400
William Bland <blandwwa@gmail.com> wrote:

> The implementation returns -EINVAL when the mbuf is shared or
> indirect, but the doc comment inaccurately stated -EPERM.
> 
> Fixes: c974021a5949 ("ether: add soft vlan encap/decap")
> 
> Signed-off-by: William Bland <blandwwa@gmail.com>
> ---
>  lib/net/rte_ether.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
> index c9a0b536c3..cb10d8fb06 100644
> --- a/lib/net/rte_ether.h
> +++ b/lib/net/rte_ether.h
> @@ -387,7 +387,8 @@ static inline int rte_vlan_strip(struct rte_mbuf *m)
>   *   The packet mbuf.
>   * @return
>   *   - 0: On success
> - *   -EPERM: mbuf is shared overwriting would be unsafe
> + *   -EINVAL: overwriting would be unsafe because mbuf is shared or
> + *            indirect, or mbuf's first segment is too short
>   *   -ENOSPC: not enough headroom in mbuf
>   */
>  static inline int rte_vlan_insert(struct rte_mbuf **m)

This patch series is good and addresses bugs. But I would like it
to go a little further by:

1. Add cc: stable@dpdk.org to bugfixes
2. Do a little more refactoring so the new function can be marked experimental
3. Be more careful in the af_packet pkt_strip flag handling
4. Use test.h unit_test_suite_runner() pattern in existing test.

Long winded AI description of same stuff...

Review: [PATCH 0/3] VLAN insert with TPID / af_packet QinQ fix

Series applies cleanly on main (d55ccd4). All three Fixes: hashes
resolve. No correctness issues in the header change or the tests;
one pre-existing flag-handling problem in the af_packet Rx path is
touched by patch 3 and should be fixed separately (see below).

Patch 1/3: net: fix VLAN insert doc comment for shared-mbuf error code

Warning: Missing Cc: stable@dpdk.org. This is a fix (has a Fixes:
tag) for text that shipped in every stable branch.

Patch 2/3: net: add VLAN insert function with a TPID argument

Warning: rte_vlan_insert_tpid() is new public API and is not marked
__rte_experimental. The precedent in the same library is
rte_ipv4_cksum_simple() in rte_ip4.h, which carries the
"@warning @b EXPERIMENTAL" Doxygen note and the attribute.

Note the complication: as structured, the stable rte_vlan_insert()
wrapper calls rte_vlan_insert_tpid(). If the latter is marked
experimental, every translation unit that includes rte_ether.h
without ALLOW_EXPERIMENTAL_API gets the deprecated-attribute warning
from inside the wrapper body, which breaks existing users of the
stable function under -Werror. Suggested structure: move the body
into an unmarked helper and have both public functions call it.

	static inline int
	__rte_vlan_insert(struct rte_mbuf **m, uint16_t tpid)
	{
		/* current body */
	}

	static inline int rte_vlan_insert(struct rte_mbuf **m)
	{
		return __rte_vlan_insert(m, RTE_ETHER_TYPE_VLAN);
	}

	__rte_experimental
	static inline int
	rte_vlan_insert_tpid(struct rte_mbuf **m, uint16_t tpid)
	{
		return __rte_vlan_insert(m, tpid);
	}

Warning: No release notes entry. A new public function in lib/net
needs a line under "New Features" in
doc/guides/rel_notes/release_26_11.rst.

Warning: The unit tests for rte_vlan_insert() and
rte_vlan_insert_tpid() (app/test/test_net_ether.c, app/test/meson.build)
are added in patch 3, the af_packet fix. They test the library API
introduced here and should be part of this patch. That also keeps
the driver fix self-contained for backporting.

Patch 3/3: net/af_packet: fix QinQ outer TPID on VLAN reinsertion

Warning: The Rx VLAN block sets RTE_MBUF_F_RX_VLAN_STRIPPED
unconditionally and then, when vlan_strip is off, relies on
rte_vlan_insert_tpid() to clear it again. The non-strip path only
ends up with correct flags because the insert helper undoes the
flag; if the insert fails the mbuf is delivered with STRIPPED set
and an error logged. This dates from d41d39bcf7 ("net/af_packet:
reinsert stripped VLAN tag"), not from this patch, but this patch
reshapes exactly that block and is the place to sort it out. Split
it: one patch that fixes the flag handling (Fixes: d41d39bcf7,
Cc: stable), then the TPID change on top. Suggested shape for the
first:

	if (ppd->tp_status & TP_STATUS_VLAN_VALID) {
		mbuf->vlan_tci = ppd->tp_vlan_tci;
		mbuf->ol_flags |= RTE_MBUF_F_RX_VLAN;

		if (pkt_q->vlan_strip) {
			mbuf->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED;
		} else if (rte_vlan_insert(&mbuf) != 0) {
			PMD_LOG(ERR, "Failed to reinsert VLAN tag");
			mbuf->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED;
		}
	}

Warning: Missing Cc: stable@dpdk.org on a bug fix. Because the fix
depends on the new function from patch 2, a backport will need that
patch as well; worth stating in the commit message so the stable
maintainers know to pick both.

Warning: The tests add a second REGISTER_FAST_TEST
(vlan_insert_autotest) to test_net_ether.c alongside
net_ether_autotest. Use unit_test_suite_runner() instead: convert
the existing test_net_ether() into a suite with TEST_CASE entries
for the four address tests, add the two VLAN insert cases, and
create/free the mempool in the suite's .setup/.teardown so the
individual cases do not need a mempool parameter. That also
replaces the fprintf()/return -1 error reporting in
test_vlan_insert() with the TEST_ASSERT macros used elsewhere in
the file.

Info: test_vlan_insert_tpid() builds a frame that already carries an
802.1Q tag and then inserts an 802.1ad outer tag, but only checks
the new outer header. Checking that the pushed-down inner tag
survived (new vh->eth_proto == RTE_ETHER_TYPE_VLAN, and the following
rte_vlan_hdr still has TCI 200 / eth_proto IPv4) is what actually
exercises the QinQ case the test is named for.

Info: alloc_frame() comment says the header is "followed by
payload_len zero bytes" but rte_pktmbuf_append() does not zero the
data; either memset the payload or drop "zero" from the comment.