|WARNING| [v3,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/169878

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-21

# DPDK Patch Review - series.patch

This review covers patches 1-9 from the series against DPDK coding guidelines (AGENTS.md).

---

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

### Errors

**Statistics accumulation using `=` instead of `+=`** (line in `lib/bbdev/rte_bbdev.c`):

```c
*stats = dev->data->queues[queue_id].queue_stats;
```

This is a direct assignment of a struct, not statistics accumulation. The guideline about `stats->rx_packets = nb_rx` vs `stats->rx_packets += nb_rx` applies to incremental counter updates in loops or callbacks, not to retrieving a snapshot of accumulated statistics from internal state. This is the correct pattern for a "get stats" function that copies out the current counter values.

**NONE** - no issues found.

### Warnings

**New public function missing `RTE_EXPORT_*` macro placement**:

The export macro is present:

```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bbdev_queue_stats_get, 26.07)
int rte_bbdev_queue_stats_get(uint16_t dev_id, uint16_t queue_id, struct rte_bbdev_stats *stats)
```

However, the guideline states "Place the macro on the line immediately **before** the function definition in the `.c` file" and shows examples where the macro is on its own line, followed by the return type on the next line. The code here has the macro on one line and the return type immediately following on the same logical line (though split in the patch). For clarity and consistency with examples in AGENTS.md, this should be:

```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bbdev_queue_stats_get, 26.07)
int
rte_bbdev_queue_stats_get(uint16_t dev_id, uint16_t queue_id, struct rte_bbdev_stats *stats)
```

---

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

### Errors

NONE

### Warnings

NONE

**Review**: The patch correctly changes `RTE_EXPORT_INTERNAL_SYMBOL` to `RTE_EXPORT_SYMBOL` and removes `__rte_internal` tags from stub functions that are called through public inlined API. The commit message clearly states the rationale (applications need to call these stubs when GFNI is unavailable, so they cannot be internal-only). This is a valid fix.

---

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

### Errors

NONE

### Warnings

NONE

**Review**: Moving process forking helpers from an inline header to a `.c` file is reasonable. The code was previously using internal API (`rte_devargs_*`, `rte_eal_driver_path_*`) which required `ALLOW_INTERNAL_API`; moving it to a `.c` file allows that flag to be scoped to just the one compilation unit instead of every consumer of `process.h`. The refactoring is correct.

---

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

### Errors

NONE

### Warnings

NONE

**Review**: Replaces direct access to `rte_eth_devices[port_id].data->mac_addrs` with `rte_eth_macaddr_get()` calls. This removes reliance on internal `ethdev_driver.h` header. The changes are correct; the variables change from pointer type to value type (`struct rte_ether_addr` instead of `struct rte_ether_addr *`), and comparisons are updated accordingly (`&member_mac1` instead of `member_mac1`).

---

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

### Errors

NONE

### Warnings

NONE

**Review**: Replaces use of `net_ring` driver (which required internal bus API) with `net_null` driver and public `rte_bus_find_by_name()` / iterator API. The commit message states `net_null` is enabled in all CI test builds, making it a safer dependency than `net_ring`. The refactoring is correct.

---

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

### Errors

NONE

### Warnings

NONE

**Review**: Removes internal `bus->find_device()` usage and replaces it with the public `RTE_DEV_FOREACH()` iterator. The logic is equivalent but uses only public API. The code is correct.

---

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

### Errors

NONE

### Warnings

NONE

**Review**: Removes the global `ALLOW_INTERNAL_API` flag from the test suite meson.build and adds it selectively to the `.c` files that actually need internal API. This is a good hygiene improvement. The patch correctly identifies which test files need the define and which do not. The change to `app/test/meson.build` removes 3 lines (global `-DALLOW_INTERNAL_API`), and 6 `.c` files get the `#define ALLOW_INTERNAL_API` added at the top. This is correct scoping.

---

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

### Errors

NONE

### Warnings

NONE

**Review**: Optimizes ABI reference generation by disabling header checks, developer mode, apps, docs, and examples when building the reference for ABI comparison. The rationale is sound (only drivers and libs are needed for ABI comparison). The patch correctly passes these flags only when building the reference (`REF_OPTS`), not the current build (`OPTS`). The changes are correct.

---

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

### Errors

NONE

### Warnings

NONE

**Review**: Adds a CI step that runs `dpdk-testpmd` and `dpdk-test` binaries compiled from the ABI reference release against the current release's libraries/drivers. This provides an additional level of ABI validation (catching symbol regressions that static ABI checks might miss). The script logic:
1. Copies reference binaries (`dpdk-testpmd`, `dpdk-test`) into the `build/app/` directory temporarily
2. Runs `devtools/test-null.sh` (for testpmd)
3. Runs `meson test` (for unit tests), skipping tests that don't exist in the reference build
4. Restores the original binaries

The logic is sound. The `DPDK_TEST_SKIP` calculation uses `grep -vxFf` to find tests in the current build that are not in the reference, which is correct. The patch changes `-Ddisable_apps=*` to `-Denable_apps=test,test-pmd` to build the needed apps in the reference. The patch keeps the binaries now instead of deleting them (`rm -rf reference/usr/local/bin/*` followed by `cp` instead of `rm -rf reference/usr/local/bin`). This is correct.

---

## Summary

### Errors: 0

### Warnings: 1

**Patch 1/9** (Warning):
- `RTE_EXPORT_EXPERIMENTAL_SYMBOL` macro should be on its own line before the return type, per the style shown in AGENTS.md examples. Current formatting has the macro and return type on adjacent lines. Recommend:
  ```c
  RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bbdev_queue_stats_get, 26.07)
  int
  rte_bbdev_queue_stats_get(uint16_t dev_id, uint16_t queue_id, struct rte_bbdev_stats *stats)
  ```

### Overall Assessment

This is a well-structured patch series focused on reducing internal API usage in tests and improving CI infrastructure. The patches are correct, well-documented, and follow DPDK contribution guidelines. The one style warning is minor and does not affect correctness.