|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: net/bonding: reject control operations in secondary

## Summary
This patch adds process-type checks to prevent secondary processes from executing bonding control-plane operations. The approach is sound, but there are several issues with implementation consistency, return value handling, and one potential correctness bug.

---

## Errors

### 1. Inconsistent error return values in `bond_api_primary_only()`
**File:** `drivers/net/bonding/rte_eth_bond_api.c`

The helper function `bond_api_primary_only()` returns `-1` on error, but all callers check for `!= 0` and then return their own error codes (`-1` or `-ENOTSUP`). This creates unnecessary inconsistency.

The other two helper functions (`bond_8023ad_primary_only()` and `bond_ethdev_primary_only()`) return `-ENOTSUP` directly and callers propagate it correctly.

**Fix:** Change `bond_api_primary_only()` to return `-ENOTSUP` like the other helpers:

```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;  /* was: return -1; */
}
```

Then simplify all call sites from:
```c
if (bond_api_primary_only(__func__) != 0)
	return -1;
```
to:
```c
if (bond_api_primary_only(__func__) != 0)
	return bond_api_primary_only(__func__);
```
or even better, factor it into a single call:
```c
int ret;
if ((ret = bond_api_primary_only(__func__)) != 0)
	return ret;
```

### 2. Missing return value propagation in `bond_ethdev_mac_addr_remove()`
**File:** `drivers/net/bonding/rte_eth_bond_pmd.c`, function `bond_ethdev_mac_addr_remove()`

This function has `void` return type but all other similar functions return `int`. The early-return check was added but there's no way to signal the error to the caller since the function signature is `void`.

This is less critical because the function signature is dictated by the ethdev ops structure, but the inconsistency should be noted. The current implementation (silent return on secondary) is acceptable given the constraint, but verify this matches the ethdev API contract for this callback.

---

## Warnings

### 1. Redundant check-then-return pattern
**Multiple locations**

Many call sites follow this pattern:
```c
if (bond_api_primary_only(__func__) != 0)
	return -1;
```

This checks the return value then returns a hardcoded error code, losing the original error value (`-ENOTSUP`). While not a bug (the callers are consistent), it's better to propagate the actual error code:

```c
int ret;
if ((ret = bond_api_primary_only(__func__)) != 0)
	return ret;
```

This applies to all call sites in `rte_eth_bond_api.c` after fixing Error #1.

### 2. Extra blank line
**File:** `drivers/net/bonding/rte_eth_bond_api.c`, line 162

There is an unnecessary extra blank line after the `bond_api_primary_only()` function definition:

```c
	return -1;
}


RTE_EXPORT_SYMBOL(rte_eth_bond_create)
```

Remove one blank line.

### 3. Process-type check pattern could use a macro
**Multiple files**

The same process-type check is repeated in three different helper functions across three files. Consider defining a common macro in a bonding internal header to reduce duplication:

```c
#define BOND_PRIMARY_ONLY(func_name) \
	do { \
		if (rte_eal_process_type() == RTE_PROC_SECONDARY) { \
			RTE_BOND_LOG(ERR, "%s not supported in secondary process", \
				     (func_name)); \
			return -ENOTSUP; \
		} \
	} while (0)
```

Then use: `BOND_PRIMARY_ONLY(__func__);` at the start of each function.

This is a style suggestion for consistency, not required.

---

## Info

### 1. Commit message and patch structure
The commit message clearly explains the rationale (prerequisite for future secondary datapath support) and references Bugzilla ID 1900. The patch structure is logical, grouping changes by file/subsystem. Well done.

### 2. Naming consistency
The three helper functions have consistent naming patterns:
- `bond_api_primary_only()`
- `bond_8023ad_primary_only()`  
- `bond_ethdev_primary_only()`

This makes the code easy to follow.

### 3. Coverage appears complete
The patch covers all the major control-plane entry points:
- Device lifecycle (create, free, start, stop, configure)
- Member management (add, remove)
- Configuration (mode, MAC, MTU, promiscuous, multicast, RSS, VLAN)
- LACP/802.3ad control
- Statistics reset

Good coverage for a first-pass secondary-process hardening.

---

## Correctness Check Summary

 No use-after-free  
 No resource leaks  
 No missing error checks  
 No race conditions introduced (checks are read-only on immutable process type)  
 No NULL dereferences  
 Inconsistent error return values (see Error #1)  
 No usage of deprecated API  
 No forbidden tokens  

---

## Final Recommendation

**Fix the return value inconsistency in Error #1** before merging. The other warnings are minor style issues. The patch is functionally correct and achieves its stated goal of preventing secondary processes from modifying bonding control state.