|WARNING| [v2] ipc: fix multi-process disabling

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-04

# DPDK Patch Review

## ERRORS

**Error 1: Missing return value check in `eal_mcfg_update_from_internal()`**

Line: `rte_mp_disable();`

When `rte_mp_disable()` is called in `eal_mcfg_update_from_internal()`, its return value is ignored.
The function returns `bool` indicating success/failure, and the code that changes multi-process status should verify it succeeded.

```c
/* Current code in eal_common_mcfg.c */
if (internal_conf->no_shconf)
    rte_mp_disable(); /* multi-process requires shared config */

/* Suggested fix */
if (internal_conf->no_shconf) {
    if (!rte_mp_disable())
        EAL_LOG(WARNING, "Failed to disable multi-process");
}
```

**Error 2: Variable declared after statement without blank line separator**

Line: `DIR *mp_dir;` and following lines in `rte_mp_request_sync()`

The patch moves the `EAL_LOG(DEBUG, "request: %s", req->name);` statement to appear before variable declarations,
violating the requirement to have an empty line between declarations and statements.

The original code had:
```c
const struct internal_config *internal_conf = ...;

EAL_LOG(DEBUG, "request: %s", req->name);
```

After the patch removes `internal_conf` declaration but keeps the `EAL_LOG()` early, the function now has:
```c
rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
       const struct timespec *ts)
{
    DIR *mp_dir;
    struct dirent *ent;
    struct timespec now, end;

    EAL_LOG(DEBUG, "request: %s", req->name);
```

But this appears to be pre-existing - the `EAL_LOG()` was already before the check_input() in the original code based on context.
Actually, reviewing more carefully, the declarations are at the top of the function block, and `EAL_LOG()` comes after them - this is correct C99 style.

**Correction**: On re-reading, the variable declarations `DIR *mp_dir;` etc. are at the start of the function,
and `EAL_LOG(DEBUG, ...)` comes after. This follows the acceptable C99 pattern. Not an error.

## WARNINGS

**Warning 1: Missing Reviewed-by tag**

The patch is v2 addressing review comments but carries no Reviewed-by tag from v1 reviewers.
For a bug fix going to stable, maintainer review acknowledgment should be present.

**Warning 2: Function parameter ordering inconsistency**

In `rte_mp_reply()`, the debug log is now before the input validation:
```c
EAL_LOG(DEBUG, "reply: %s", msg->name);
const struct internal_config *internal_conf = ...;

if (check_input(msg) != 0)
    return -1;
```

After the patch removes the `internal_conf` declaration, this becomes:
```c
EAL_LOG(DEBUG, "reply: %s", msg->name);

if (check_input(msg) != 0)
    return -1;
```

The debug log references `msg->name` before validating that `msg` is valid via `check_input()`.
If `msg` is NULL or malformed, this could crash.
Move the `EAL_LOG()` after `check_input()`.

**Warning 3: Inconsistent function naming convention**

The new function `eal_mp_is_disabled()` uses negative logic (is_disabled), while the existing related function is `__rte_mp_enable()` (positive logic).
For consistency and clarity, consider `eal_mp_is_enabled()` returning the inverse boolean, or document why the negative form was chosen.

The comment in the patch message says "reverse the logic eal_mp_is_enabled -> eal_mp_is_disabled to map the meaning of the values in enum mp_status",
so this was an intentional design choice, but the negative form makes call sites less readable:
```c
if (!eal_mp_is_disabled())  /* double negative */
```
versus
```c
if (eal_mp_is_enabled())    /* clearer */
```

## INFO

**Info 1: Pre-existing: Missing blank line in rte_mp_reply()**

In the original code before this patch, `rte_mp_reply()` has:
```c
int
rte_mp_reply(struct rte_mp_msg *msg, const char *peer)
{
    EAL_LOG(DEBUG, "reply: %s", msg->name);
    const struct internal_config *internal_conf = ...;
```

Declaration `internal_conf` immediately follows the `EAL_LOG()` statement without a blank line.
This violates the style requirement for a blank line between statements and declarations.
Pre-existing issue, not introduced by this patch.

**Info 2: Windows stub implementation**

The Windows implementation of `eal_mp_is_disabled()` in `lib/eal/windows/eal.c` unconditionally returns `true`.
This is correct for Windows (no multi-process support), but worth noting that Windows DPDK applications calling the new function will always see multi-process as disabled.

Review-Result: ERROR