On Tue, 16 Sep 2025 11:51:14 -0700
[email protected] wrote:
> From: Matthew G McGovern <[email protected]>
>
> Problem: Some devices do not allow promiscuous mode.
>
> Testpmd handles this by warning and ignoring failures from
> rte_eth_promiscuous_enable.
>
> Changes: make mp example apps warn if promiscuous mode cannot be
> enabled, rather than failing.
>
> Signed-off-by: Matthew G McGovern <[email protected]>
>
> ---
AI review spotted a bug in this. Note, examples don't have to use RTE_LOG().
Only do so if other code in the example does.
## DPDK Patch Review: examples/mp_server: warn on promisc enable failure
### Summary
This patch changes promiscuous mode enable failure handling in two
multi-process example applications from a fatal error to a warning, aligning
behavior with testpmd. While the intent is good, there's a **critical bug** in
the implementation.
---
### Critical Bug (Error - Must Fix)
**Wrong argument in fprintf format string** - Both changes print `retval` (the
error code) where the port number should be:
```c
fprintf(stderr,
"Error during enabling promiscuous mode for port %u: %s - ignore\n",
retval, rte_strerror(-retval));
// ^^^^^^ BUG: should be port_num (init.c) or port (main.c)
```
The `%u` format specifier expects the port number, but receives the negative
error code. This will print nonsensical port numbers (e.g., "port 4294967287"
for `-ENOTSUP`).
**Fix:**
```c
// In init.c:
fprintf(stderr,
"Error during enabling promiscuous mode for port %u: %s - ignore\n",
port_num, rte_strerror(-retval));
// In main.c:
fprintf(stderr,
"Error during enabling promiscuous mode for port %u: %s - ignore\n",
port, rte_strerror(-retval));
```
---
### Commit Message Review
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✓ | 49 characters |
| Lowercase after colon | ✓ | |
| Imperative mood | ✓ | "warn" is imperative |
| No trailing period | ✓ | |
| Body wrapped at 75 chars | ✓ | |
| Body doesn't start with "It" | ✓ | |
| Signed-off-by present | ✓ | |
**Warning: Subject prefix mismatch** - The subject says `examples/mp_server:`
but the patch also modifies `examples/multi_process/symmetric_mp/main.c`.
Consider using `examples/multi_process:` to accurately reflect the scope of
changes.
---
### Code Style Review
| Check | Status |
|-------|--------|
| Line length ≤100 chars | ✓ |
| No trailing whitespace | ✓ |
| Consistent with surrounding code | ✓ |
---
### Design Considerations (Info)
1. **Consistency with testpmd**: The approach matches how testpmd handles this,
which is good for consistency across DPDK examples.
2. **Using fprintf vs RTE_LOG**: Example code commonly uses `fprintf(stderr,
...)` rather than `RTE_LOG()`, so this is acceptable style for examples.
3. **Message wording**: The "- ignore" suffix is slightly awkward. Consider:
`"...port %u: %s (continuing anyway)\n"` or simply `"Warning: ..."`
---
### Verdict
**Changes Requested** - The patch has the right intent but contains a
significant bug that would produce incorrect/confusing output. After fixing the
format string arguments, this should be straightforward to accept.
