|WARNING| dev: hide internal type for device comparison

dpdklab at iol.unh.edu dpdklab at iol.unh.edu
Mon Jul 6 18:13:33 CEST 2026


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

_AI Code Review_

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

# DPDK Patch Review

## Patch Summary
This patch moves `rte_dev_cmp_t` from `rte_dev.h` (public API) to `bus_driver.h` (internal API), claiming it's only used in bus operations and not referenced by any public API.

---

## FINDINGS

### Errors

**1. Missing release notes for ABI change**

Removing `rte_dev_cmp_t` from `rte_dev.h` is an ABI break for any application that used this type. The patch must document this in the current release notes under an API/ABI changes section.

**Fix:** Add entry to `doc/guides/rel_notes/release_25_07.rst` (or current release):
```rst
API Changes
-----------

* **Moved rte_dev_cmp_t to internal header.**

  The ``rte_dev_cmp_t`` type has been moved from ``rte_dev.h`` to
  ``bus_driver.h`` as it is only used by bus driver implementations
  and not by applications.
```

---

### Warnings

**1. Forward declaration style inconsistency**

The patch adds forward declarations to `bus_driver.h`:
```c
struct rte_dev_iterator;
struct rte_devargs;
struct rte_device;
struct rte_driver;
```

However, only `struct rte_dev_iterator` and `struct rte_driver` are new additions. The existing code already had `struct rte_devargs` and `struct rte_device` visible (they were brought in via the now-removed `#include <rte_dev.h>`).

While the forward declarations are correct, verify that all users of `bus_driver.h` that need the full definitions of these types have appropriate includes. The test file correctly adds `#include <rte_dev.h>`, but check if any other code paths exist.

**Fix:** Audit all `#include <bus_driver.h>` users to ensure they include headers for any types they use from the forward-declared structures.

---

### Info

**1. Documentation duplication**

The Doxygen comment for `rte_dev_cmp_t` is copied verbatim between the two locations. This is appropriate for the move, but ensure the comment remains synchronized if future edits occur. Consider adding a note that this is an internal type if that's the intended scope.

**2. Include order verification**

The test file `app/test/test_vdev.c` now includes `rte_dev.h` before `bus_driver.h`. This is correct per the include ordering guidelines (DPDK EAL includes before misc library includes). Verify this pattern is followed wherever `bus_driver.h` is included and needs `rte_dev_cmp_t`.

---

## Summary

**Must fix (Error):**
- Add release notes entry documenting the API change

**Should verify (Warning):**
- Confirm all `bus_driver.h` users that need full struct definitions have appropriate includes

**Code quality:**
- The approach is sound: moving an internal type from public to internal headers reduces API surface
- Forward declarations are used correctly
- Test file includes updated appropriately

The patch is a reasonable cleanup but requires documentation of the ABI/API change before merging.


More information about the test-report mailing list