|WARNING| [RFC] net/bonding: reject control operations in secondary

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds secondary process rejection for bonding control operations. The code is mostly correct, but there are several issues to address.

---

## Errors

### 1. Inconsistent error return values in `bond_api_primary_only()`
**File**: `drivers/net/bonding/rte_eth_bond_api.c`  
**Lines**: 153-161, callers at 168-169, 210-211, etc.

The helper function `bond_api_primary_only()` returns `-1` on error, but all its callers also return `-1`. In DPDK, `-1` is acceptable, but the pattern is inconsistent with the other two helpers (`bond_8023ad_primary_only()` and `bond_ethdev_primary_only()`) which return `-ENOTSUP`. The callers in this file then propagate `-1` instead of `-ENOTSUP`, creating inconsistent error codes across the bonding API.

**Fix**: Change `bond_api_primary_only()` to return `-ENOTSUP` instead of `-1`, and update all callers to propagate the actual error code:
```c
static int
bond_api_primary_only(const char *op)
{
	if (rte_eal_process_type() != RTE_PROC_SECONDARY)
		return 0;

	RTE_BOND_LOG(ERR, "%s not supported in secondary process", op);
	return -ENOTSUP;
}

/* Example caller fix */
int
rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
{
	int ret;

	ret = bond_api_primary_only(__func__);
	if (ret != 0)
		return ret;
	/* ... */
}
```

### 2. Error path resource leak in `bond_ethdev_rx_queue_setup()`
**File**: `drivers/net/bonding/rte_eth_bond_pmd.c`  
**Lines**: 2433-2451

The function allocates `bd_rx_q` with `rte_zmalloc_socket()` at line 2437-2439. The secondary process check added at lines 2434-2435 returns `-ENOTSUP` **after** the allocation, but the existing `if (bd_rx_q == NULL)` check at line 2440 would return without freeing. However, on closer inspection, the patch adds the check **before** the allocation, so there is no leak here. This is correct.

(After re-checking: the patch adds the check at lines 2434-2435, which is **before** the `rte_zmalloc_socket()` at lines 2437-2439, so no leak occurs. This is not an error.)

---

## Warnings

### 1. Missing release notes update
**Context**: New functionality affecting API behavior

The patch changes the behavior of all bonding control functions when called from a secondary process. Previously they may have silently failed or behaved unpredictably; now they explicitly return `-ENOTSUP`. This is a user-visible behavior change that should be documented in the release notes under a "Behavioral Change" or "Bug Fix" section.

**Suggestion**: Add an entry to `doc/guides/rel_notes/release_26_03.rst` (or the appropriate current release file):
```rst
* **bonding: Reject control operations in secondary processes.**

  The bonding PMD now returns ``-ENOTSUP`` when control-plane functions
  (member add/remove, mode set, LACP configuration, etc.) are called from
  secondary processes. This prevents undefined behavior and ensures that
  bonding state remains consistent when owned by the primary process.
```

### 2. All helper functions could be static inline
**Files**: All three modified files  
**Functions**: `bond_api_primary_only()`, `bond_8023ad_primary_only()`, `bond_ethdev_primary_only()`

The three process-check helper functions are each used only within their respective `.c` files and are trivial single-condition checks. Declaring them `static inline` would allow the compiler to eliminate the function call overhead (though the compiler may inline them anyway at `-O2`).

**Suggestion**: Not critical, but consider `static inline` for micro-optimization:
```c
static inline int
bond_api_primary_only(const char *op)
{
	/* ... */
}
```

### 3. Function name passed to logger is not useful in all cases
**Context**: All three helper functions use `__func__` from the caller

When `bond_api_primary_only(__func__)` is called from `rte_eth_bond_create()`, the log message will say `rte_eth_bond_create not supported in secondary process`. This is correct and helpful. However, the pattern is that all callers pass `__func__`, which means the helper's `op` parameter is always the calling function's name. The helpers themselves could just use `__func__` directly and avoid the parameter, or the current pattern is fine for clarity. This is a minor style observation, not a real issue.

---

## Info

### 1. Patch targets future work
**Context**: Commit message mentions "prerequisite for future limited secondary datapath support"

The patch correctly blocks control-plane operations but leaves datapath (burst functions) as-is. The commit message indicates this is preparation for future secondary process datapath support. Consider adding a code comment in one of the modified files (e.g., at the top of `rte_eth_bond_pmd.c` or near the first helper function) explaining the current state and pointing to the future work, so maintainers understand the context:
```c
/*
 * Secondary process support: Control-plane operations are rejected
 * (see bond_ethdev_primary_only() checks in dev_ops).
 * Datapath support for secondary processes is not yet implemented;
 * safe stub burst functions are installed instead (see bond_ethdev_configure()).
 */
```

### 2. Whitespace inconsistency
**File**: `drivers/net/bonding/rte_eth_bond_api.c`  
**Line**: 162 (blank line after helper function)

There are two consecutive blank lines after `bond_api_primary_only()` (lines 161-162). DPDK style typically uses a single blank line between functions. This is minor.

---

## Final Check

**Correctness bugs**: One error-code inconsistency issue in `bond_api_primary_only()` and its callers.

**Style/process false positives removed**: The `bond_ethdev_rx_queue_setup()` allocation/check order was initially flagged but is actually correct (check comes before allocation).

**Clean patches**: No other issues found.