|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

None identified.

### Warnings

1. **New experimental symbol uses incorrect version format**
   ```c
   RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bbdev_queue_stats_get, 26.07)
   ```
   The version should match the release being developed. The patch targets release 26.11 (per the release notes file name), but the export macro specifies 26.07. These must be consistent.

2. **Function placement after export macro**
   The export macro should appear immediately before the function definition in the .c file (which it does), but there's inconsistent spacing. The function should start on the line immediately after the macro with no blank line.

### Info

- The API addition is properly documented in the release notes.
- The new API is correctly marked as `__rte_experimental` in the header.
- The implementation correctly validates inputs before accessing internal structures.
- The test code now uses the public API instead of directly accessing internal structures, which is the correct approach.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- The fix correctly changes `RTE_EXPORT_INTERNAL_SYMBOL` to `RTE_EXPORT_SYMBOL` for the GFNI stub functions. This is appropriate because inlined API functions call these stubs, and applications must be able to link against them without requiring `ALLOW_INTERNAL_API`.
- The `__rte_internal` annotations are correctly removed from the function declarations.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Moving code from an inline header to a .c file is a reasonable refactoring when the code is not performance-critical and uses internal API.
- The new process.c file is correctly added to the meson build only for non-Windows platforms.
- Function signatures are properly exposed in the header for callers.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Replacing direct access to `rte_eth_devices[].data->mac_addrs` with `rte_eth_macaddr_get()` is correct and removes dependency on internal ethdev headers.
- The change from pointer (`struct rte_ether_addr *`) to value (`struct rte_ether_addr`) for `member_mac1` and `member_mac2` variables is correct since `rte_eth_macaddr_get()` copies the address into the caller's buffer.
- All comparisons are correctly updated to use the address-of operator (`&member_mac1`, `&member_mac2`) to match the `rte_is_same_ether_addr()` signature.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Standardizing on `net_null` (which is always enabled in CI builds) instead of `net_ring` reduces dependencies and test code complexity.
- Replacing `bus_driver.h` (internal) with `rte_bus.h` (public) removes unnecessary internal API usage.
- The removed `rte_bus->parse()` call was using internal bus API that is not appropriate for unit tests.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Replacing internal bus `find_device()` calls with the public `RTE_DEV_FOREACH()` iterator is the correct approach for test code.
- The simplified `find_vdev_by_name()` helper is clearer and uses only public API.
- Removing the kvargs-based matching complexity is appropriate since the test only needs simple name-based device lookup.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Removing the global `-DALLOW_INTERNAL_API` from the test build and moving it to individual files that genuinely require internal API is good practice.
- The specific files that define `ALLOW_INTERNAL_API` are appropriate:
  - `process.c`: uses `rte_eal_get_runtime_dir()` and other EAL internals for process spawning
  - `test_devargs.c`: tests internal devargs parsing
  - `test_external_mem.c`, `test_malloc.c`, `test_mempool.c`: test memory subsystem internals
  - `test_pdump.c`, `virtual_pmd.c`: use ethdev driver-level internal API

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- The optimization to speed up ABI reference generation by disabling unnecessary components (examples, apps, docs, tests) is reasonable.
- The comment correctly notes that only libraries and drivers are needed for ABI checking.
- Similar changes are made consistently in both `.ci/linux-build.sh` and `devtools/test-meson-builds.sh`.

---

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

### Errors

1. **Empty test skip list will cause variable expansion issues**
   ```bash
   ABI_SKIP_TESTS: none
   ```
   Later in the script:
   ```bash
   for t in ${ABI_SKIP_TESTS}; do
       DPDK_TEST_SKIP=${DPDK_TEST_SKIP+$DPDK_TEST_SKIP,}$t
   done
   ```
   When `ABI_SKIP_TESTS="none"`, this loop will execute once with `t="none"`, which will be passed to the test framework as a test name to skip. If there's no test named "none", this is harmless but incorrect.

   **Fix**: Use an empty string for the default and guard the loop:
   ```bash
   ABI_SKIP_TESTS:
   ```
   ```bash
   if [ -n "$ABI_SKIP_TESTS" ]; then
       for t in ${ABI_SKIP_TESTS}; do
           DPDK_TEST_SKIP=${DPDK_TEST_SKIP+$DPDK_TEST_SKIP,}$t
       done
   fi
   ```

### Warnings

None identified beyond the error above.

### Info

- The concept of running reference binaries (testpmd, unit tests) against current libraries is a valuable additional ABI compatibility check.
- The approach of copying reference binaries into the build directory, running tests, then restoring the original is correct.
- The `ABI_SKIP_TESTS` mechanism for skipping tests that didn't exist in the reference release is a sensible design.

---

## Summary

### Critical Issues Requiring Fix

**Patch 9/9:**
- Empty `ABI_SKIP_TESTS` list will add "none" as a test to skip (harmless but incorrect).

### Warnings to Address

**Patch 1/9:**
- Export symbol version (26.07) does not match target release (26.11).
- Minor formatting inconsistency (blank line after export macro).

### Overall Assessment

This patch series successfully removes most unnecessary internal API usage from the test suite and adds useful infrastructure for ABI validation. The changes are generally well-structured, with correct use of public APIs to replace internal access patterns.

The single error identified (empty test skip list handling) should be corrected. The version mismatch warning in patch 1 should also be addressed to ensure consistency between the release notes and symbol versioning.