|WARNING| [3/3] net/af_packet: fix QinQ outer TPID on VLAN reinsertion

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/168268

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-24

# DPDK Patch Review

## Summary

This 3-patch series fixes VLAN handling in af_packet PMD and adds support for preserving the original TPID (802.1Q vs 802.1ad) when reinserting VLAN tags. The patches are well-structured, with patch 1 fixing a documentation bug, patch 2 adding the new API, and patch 3 using it in af_packet.

---

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

**No issues found.**

---

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

### Warnings

**Missing `__rte_experimental` tag**

The new function `rte_vlan_insert_tpid()` is a public API addition and must be marked as experimental.
Add `__rte_experimental` on the line before the function declaration:

```c
/**
 * Insert VLAN tag with the given TPID into mbuf.
 * ...
 */
__rte_experimental
static inline int rte_vlan_insert_tpid(struct rte_mbuf **m, uint16_t tpid)
```

**Missing release notes**

New API additions require an entry in the release notes (`doc/guides/rel_notes/release_26_03.rst` or current release).
Add a section under "New Features":

```rst
* **Added TPID parameter support for VLAN insertion.**

  Added ``rte_vlan_insert_tpid()`` to allow inserting VLAN tags with a
  caller-specified TPID, enabling correct reinsertion of 802.1ad (QinQ)
  outer tags.
```

---

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

### Errors

**Missing error check on `rte_pktmbuf_pool_create()`**

The test calls `rte_pktmbuf_pool_create()` and checks if the result is NULL,
but does not return `TEST_FAILED` -- it returns `-1`.
The test framework expects `TEST_FAILED` (or `TEST_SUCCESS`) as the return value.

```c
/* BAD - returns -1 instead of TEST_FAILED */
mp = rte_pktmbuf_pool_create(...);
if (mp == NULL) {
    fprintf(stderr, "Failed to create mempool\n");
    return -1;
}

/* GOOD */
mp = rte_pktmbuf_pool_create(...);
if (mp == NULL) {
    fprintf(stderr, "Failed to create mempool\n");
    return TEST_FAILED;
}
```

Apply this change to `test_vlan_insert()`.

### Info

**TEST_ASSERT vs fprintf**

The test mixes `TEST_ASSERT_*` macros (which print detailed failure messages)
with manual `fprintf(stderr, ...)` followed by early return.
For consistency, consider using `TEST_ASSERT_NOT_NULL` for the mempool allocation:

```c
mp = rte_pktmbuf_pool_create("vlan_insert_test_pool", 64, 0, 0,
                             RTE_MBUF_DEFAULT_BUF_SIZE,
                             SOCKET_ID_ANY);
TEST_ASSERT_NOT_NULL(mp, "Failed to create mempool");
```

This is a minor style preference; the current code is acceptable if you prefer the explicit cleanup path.

---

## Overall Assessment

The patch series correctly addresses the QinQ TPID preservation issue.
The implementation is sound, the test coverage is good, and the code follows DPDK style.

**Required changes:**
- Add `__rte_experimental` to `rte_vlan_insert_tpid()` in patch 2
- Add release notes entry for the new API in patch 2
- Fix return value in `test_vlan_insert()` to use `TEST_FAILED` instead of `-1` in patch 3

**Recommended change:**
- Use `TEST_ASSERT_NOT_NULL` for mempool allocation in patch 3 for consistency with the rest of the test