|WARNING| eal: add some debug for unit test failures

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-16

# DPDK Patch Review

## Analysis

This patch adds debug logging to help diagnose unit test failures in ARM containers. The changes span three files: `app/test/commands.c`, `app/test/process.c`, and `lib/eal/common/malloc_heap.c`.

---

## ERRORS

### 1. Missing `errno.h` include in `app/test/commands.c`

**File:** `app/test/commands.c`

**Issue:** The patch adds a call to `strerror(errno)` on line 372, but there is no evidence in the visible diff that `errno.h` is included. While it may be transitively included, explicit inclusion is required when directly referencing `errno` or `strerror()`.

**Line:**
```c
printf("commands_init: calloc(%d) failed %s\n", commands_len, strerror(errno));
```

**Fix:** Add `#include <errno.h>` to the include section of `app/test/commands.c`.

---

### 2. Missing newline in error message format string

**File:** `app/test/commands.c`, line 372

**Issue:** The `printf()` format string is missing a newline separator between the size and the error string. The output will be:
```
commands_init: calloc(12345) failed No memory
```
instead of the more readable:
```
commands_init: calloc(12345) failed: No memory
```

**Line:**
```c
printf("commands_init: calloc(%d) failed %s\n", commands_len, strerror(errno));
```

**Fix:**
```c
printf("commands_init: calloc(%d) failed: %s\n", commands_len, strerror(errno));
```

---

### 3. Use of `printf()` in library code

**File:** `lib/eal/common/malloc_heap.c`, line 661

**Issue:** The added code uses `EAL_LOG(ERR, ...)` which is correct for library code. However, the error message lacks useful diagnostic information. It does not include the requested size, alignment, flags, or any context that would help diagnose why the allocation failed. This makes the debug message nearly useless for its stated purpose of diagnosing test failures.

**Line:**
```c
if (ret == NULL)
    EAL_LOG(ERR, "Error allocating from heap in legacy mode");
```

**Fix:** Include diagnostic information:
```c
if (ret == NULL)
    EAL_LOG(ERR, "Failed to allocate %zu bytes from heap %u (flags=0x%x, align=%zu, bound=%zu, contig=%d)",
            size, heap_id, flags, align, bound, contig);
```

---

## WARNINGS

### 1. Missing `#include <errno.h>` for `WIFEXITED`/`WEXITSTATUS` macros

**File:** `app/test/process.c`

**Issue:** The patch adds calls to `WIFEXITED()`, `WEXITSTATUS()`, `WIFSIGNALED()`, and `WTERMSIG()` macros. These are defined in `<sys/wait.h>`, not `<errno.h>`. While they may be transitively included, the file should explicitly include `<sys/wait.h>` when using wait status macros.

**Lines:** 203-210

**Fix:** Verify that `#include <sys/wait.h>` is present in `app/test/process.c`. If not, add it.

---

### 2. Debug code should not be in production paths

**General concern across all three files**

**Issue:** This patch is described as "Dirty patch to try to catch more info" in the commit message, indicating it is intended as temporary debug instrumentation rather than a production change. Debug patches should:
- Either be marked clearly as debug-only (e.g., wrapped in `#ifdef DEBUG_MALLOC` or similar)
- Or be submitted as proper production logging that remains in the codebase

Temporary debug patches sent to the public mailing list create maintenance burden and confusion about intent.

**Recommendation:** If this is meant to be temporary, consider either:
1. Running it locally or in a private CI environment instead of upstreaming
2. Converting it to proper production-quality diagnostic logging that will remain in the codebase
3. Wrapping the debug code in conditional compilation guards

---

### 3. Format specifier type mismatch

**File:** `app/test/commands.c`, line 372

**Issue:** `commands_len` is of type `size_t`, but the format specifier is `%d` (for `int`). This should be `%zu` for `size_t` to be type-safe and avoid potential truncation on 64-bit systems.

**Line:**
```c
printf("commands_init: calloc(%d) failed %s\n", commands_len, strerror(errno));
```

**Fix:**
```c
printf("commands_init: calloc(%zu) failed: %s\n", commands_len, strerror(errno));
```

---

## INFO

### 1. Inconsistent debug message style

**Observation:** The three files use different styles for debug messages:
- `commands.c`: uses `printf()` with function name prefix
- `process.c`: uses `printf()` with descriptive text but no function name
- `malloc_heap.c`: uses `EAL_LOG(ERR, ...)` with descriptive text

For maintainability, consider adopting a consistent style for debug messages across the test suite, such as always including the function name or file context.

---

### 2. Wait status output always printed

**File:** `app/test/process.c`, lines 203-210

**Observation:** The wait status is now printed unconditionally for every child process, regardless of success or failure. This may produce excessive output during normal test runs. Consider printing these messages only when the wait status indicates an error (non-zero exit code or termination by signal):

```c
if (WIFEXITED(status)) {
    int exit_code = WEXITSTATUS(status);
    if (exit_code != 0)
        printf("Child process exited with status %d\n", exit_code);
} else if (WIFSIGNALED(status)) {
    printf("Child process killed by signal %d\n", WTERMSIG(status));
} else {
    printf("Child process terminated with wait status %d\n", status);
}
```

---

### 3. Pre-existing: Missing error check on `strlcat()`

**File:** `app/test/commands.c` (context around line 376)

**Pre-existing issue (not introduced by this patch):** The call to `strlcat()` on line 377 (visible in context) does not check for truncation. While `strlcat()` is safer than `strcat()`, it can still fail to concatenate the full string if `commands_len` is insufficient. The return value should be checked.

This is an Info observation about surrounding code, not a defect in the patch itself.

---

Review-Result: ERROR