|WARNING| [v2,9/9] ci: run reference binaries against current ABI

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-23

# DPDK Patch Review

## Patch 1/9: bbdev: add per-queue statistics API

### Errors

**Missing Cc: stable at dpdk.org for fix backport consideration**
This patch adds a new API function, but the old code had applications directly accessing internal structures (`rte_bbdev_devices[].data->queues[].queue_stats`).
The old pattern required ALLOW_INTERNAL_API and exposed internal state.
While not strictly a bug fix, this provides a stable replacement for what was previously internal access.
Consider adding `Cc: stable at dpdk.org` if this API should be backported to LTS branches so applications can migrate off internal access.

### Warnings

**Experimental symbol version uses incorrect format**
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bbdev_queue_stats_get, 26.07)
```
The second parameter should be the DPDK release version, not a date format.
For a patch targeting the 26.11 release (per the release notes), use:
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bbdev_queue_stats_get, 26.11)
```

**Inconsistent error return documentation**
The Doxygen states `-ERANGE` for invalid queue_id, but the implementation uses `VALID_QUEUE_OR_RET_ERR()` macro.
Verify the actual error code returned by that macro matches the documentation.
If the macro returns a different error code (e.g., `-EINVAL`), update the Doxygen or the implementation to match.

---

## Patch 2/9: hash: fix GFNI stubs export

### Info

**Patch is correct**
The change from `RTE_EXPORT_INTERNAL_SYMBOL` to `RTE_EXPORT_SYMBOL` is the right fix for stubs called from inlined public API.
Removal of `__rte_internal` from the header declarations is also correct.

---

## Patch 3/9: test: uninline helper for forking

### Info

**Patch is correct**
Moving the fork helpers out of the header into process.c is a reasonable refactor.
The code now requires ALLOW_INTERNAL_API only in process.c, not everywhere that includes process.h.

---

## Patch 4/9: test/bonding: get MAC address with public API

### Info

**Patch is correct**
Replacing `rte_eth_devices[].data->mac_addrs` with `rte_eth_macaddr_get()` is the right approach.
The changes from pointer variables to value variables are consistent with the new API.

---

## Patch 5/9: test/devargs: rely on net null driver

### Info

**Patch is correct**
Standardizing on net_null removes the dependency on net_ring and the internal bus API.
The test coverage is maintained with a driver enabled in all CI builds.

---

## Patch 6/9: test/vdev: find device with public API

### Warnings

**Comment does not match code behavior**
```c
/* iterate all vdevs, and ensure we find dev0 and dev1 */
RTE_DEV_FOREACH(dev, "bus=vdev", &dev_iter) {
    if (dev == dev0)
        dev0 = NULL;
    if (dev == dev1)
        dev1 = NULL;
}
```
After the loop, the code checks `if (dev0 != NULL || dev1 != NULL)` and fails if either is non-NULL.
The comment says "ensure we find," but the variables are being nulled out when found, then checked for NULL later.
This is correct logic but the comment could be clearer:
```c
/* Iterate all vdevs, nulling out dev0/dev1 when found.
 * After the loop, both should be NULL if both devices were found. */
```

---

## Patch 7/9: test: limit internal API usage

### Errors

**test_pdump.c missing header**
After adding `#define ALLOW_INTERNAL_API`, test_pdump.c must include a header that provides the internal symbols it uses.
Verify this file compiles.
If internal API is used, the corresponding header (e.g., from lib/pdump or drivers) must be included.

### Warnings

**Inconsistent placement of ALLOW_INTERNAL_API**
Some files place `#define ALLOW_INTERNAL_API` after the copyright header with no blank line (test_devargs.c, test_external_mem.c, test_malloc.c, test_mempool.c, test_pdump.c),
while process.c has a blank line before the includes.
For consistency, use the same style across all files.
Recommend:
```c
/* SPDX-License-Identifier: ... */
/* Copyright ... */

#define ALLOW_INTERNAL_API

#include <...>
```

---

## Patch 8/9: ci: make ABI reference generation faster

### Info

**Patch is correct**
The optimizations (disabling apps, docs, examples, tests, and include checks) are reasonable for ABI reference generation, where only the shared library ABI matters.

---

## Patch 9/9: ci: run reference binaries against current ABI

### Errors

**Undefined variable ABI_SKIP_TESTS in script**
The script uses `${ABI_SKIP_TESTS}` in a loop:
```bash
for t in ${ABI_SKIP_TESTS}; do
    DPDK_TEST_SKIP=${DPDK_TEST_SKIP+$DPDK_TEST_SKIP,}$t
done
```
If `ABI_SKIP_TESTS` is unset or empty, the `for` loop will iterate over an empty list, which is fine.
However, if the variable is set to `none` (as in the GitHub workflow file), the loop will execute once with `t=none`, adding `"none"` to the DPDK_TEST_SKIP list.
If `"none"` is not a valid test name, this may produce a benign warning or be silently ignored.
Consider handling the `none` case explicitly:
```bash
if [ "${ABI_SKIP_TESTS:-}" != "" ] && [ "$ABI_SKIP_TESTS" != "none" ]; then
    for t in $ABI_SKIP_TESTS; do
        DPDK_TEST_SKIP=${DPDK_TEST_SKIP+$DPDK_TEST_SKIP,}$t
    done
fi
```

**Missing blank line removal in reference cleanup**
```bash
find reference/usr/local -name '*.a' -delete
rm -rf reference/usr/local/bin/*
cp $refsrcdir/build/app/dpdk-test reference/usr/local/bin/dpdk-test
```
After `rm -rf reference/usr/local/bin/*`, the directory still exists, so the `cp` commands work.
However, the original code had `rm -rf reference/usr/local/bin` (removing the directory).
The change to `rm -rf reference/usr/local/bin/*` (keeping the directory) is intentional, but the comment does not explain why.
Add a comment:
```bash
# Keep bin directory but remove default contents; we copy reference binaries below
rm -rf reference/usr/local/bin/*
```

**Failed variable check may not detect all failures**
```bash
failed=
configure_coredump
mv -f build/app/dpdk-testpmd build/app/dpdk-testpmd.ori
cp reference/usr/local/bin/dpdk-testpmd build/app/dpdk-testpmd
devtools/test-null.sh || failed="true"
mv -f build/app/dpdk-testpmd.ori build/app/dpdk-testpmd
catch_coredump
[ "$failed" != "true" ]
```
If `mv -f build/app/dpdk-testpmd.ori build/app/dpdk-testpmd` fails (e.g., .ori file missing due to earlier failure), the script will exit with non-zero before the final check.
Consider trapping errors:
```bash
set +e  # Allow commands to fail
mv -f build/app/dpdk-testpmd build/app/dpdk-testpmd.ori
cp reference/usr/local/bin/dpdk-testpmd build/app/dpdk-testpmd
devtools/test-null.sh || failed="true"
mv -f build/app/dpdk-testpmd.ori build/app/dpdk-testpmd || true
set -e
catch_coredump
[ "$failed" != "true" ]
```

---

## Summary

**Errors:**
- Patch 1: Experimental symbol version format (26.07 should be 26.11)
- Patch 7: Verify test_pdump.c includes required internal headers
- Patch 9: Handle ABI_SKIP_TESTS="none" case explicitly; add error trapping for mv/cp failure

**Warnings:**
- Patch 1: Consider Cc: stable at dpdk.org; verify error code documentation matches implementation
- Patch 6: Comment could be clearer about dev0/dev1 nulling logic
- Patch 7: Inconsistent placement of ALLOW_INTERNAL_API
- Patch 9: Add comment explaining why `bin/*` vs `bin` removal

**Correctness bugs:** None identified.
**Style/process issues flagged above are actual problems** (experimental symbol version, potential script failures).