Re: [PATCH v3 1/1] examples/mp_server: warn on promisc enable failure

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 16 Sep 2025 11:51:14 -0700
mamcgove@microsoft.com wrote:

> From: Matthew G McGovern <mamcgove@microsoft.com>
> 
> 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 <mamcgove@microsoft.com>
> 
> ---

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.