|WARNING| [v2] Revert "examples/symmetric_mp: do not print or exit in handler"

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Analysis

This patch reverts a previous commit that attempted to make signal handling "safe" by setting a flag instead of calling `exit()` directly. The revert restores the original behavior where the signal handler prints stats and calls `exit(0)`.

## ERRORS

**1. Signal handler calls non-async-signal-safe functions (Error)**

Line 112: `printf()` is not async-signal-safe per POSIX.
Line 113: `exit()` is not async-signal-safe (only `_exit()` is).

The signal handler `print_stats()` calls `printf()` to print statistics and then calls `exit(0)`. Neither function is async-signal-safe, meaning they cannot be called from a signal handler without risking deadlock or corruption if the signal interrupts the same function.

```c
static void
print_stats(int signum)
{
	unsigned i;

	printf("\nAggregate statistics ======================================\n");
	for (i = 0; i < num_ports; i++)
		printf("Port %u: RX - %u, TX - %u, Drop - %u\n", (unsigned)p_num,
				pstats[p_num].rx, pstats[p_num].tx, pstats[p_num].drop);
	exit(0);  /* not async-signal-safe */
}
```

While the comment acknowledges this is "not safe way to exit for a real production application", marking non-async-signal-safe usage in signal handlers is a correctness bug that could cause the example to hang or crash depending on when the signal arrives.

**Suggested fix:** Use a `volatile sig_atomic_t` flag in the signal handler and check it in the main loop, as the reverted commit did, or use `_exit()` instead of `exit()` and `write()` instead of `printf()` (though formatted output from a signal handler remains problematic).

**2. Unused function parameter not marked (Error)**

Line 112: The `signum` parameter in `print_stats(int signum)` is not used in the function body but is not marked with `__rte_unused`.

```c
static void
print_stats(int signum)
{
	/* signum is never referenced */
```

**Suggested fix:**
```c
static void
print_stats(int signum __rte_unused)
```

Note: The commit message says "v2 - need to mark arg unused" but the patch does not actually add the `__rte_unused` annotation.

**3. Infinite loop with no exit path (Error)**

Line 353: `lcore_main()` now has `for (;;)` with no break condition and no return statement. The only exit is via signal - `exit(0)`, leaving worker lcores spinning until process termination.

```c
for (;;) {
	struct rte_mbuf *buf[PKT_BURST];
	/* packet processing loop */
}
```

This is effectively a hang from the perspective of `rte_eal_mp_wait_lcore()` -- if that function were still called (it was removed in this patch), it would block forever. The patch removes the `rte_eal_mp_wait_lcore()` call, but the pattern of launching workers that never return is problematic: there is no way to shut down cleanly even if the application wanted to.

**Suggested fix:** Restore the `volatile sig_atomic_t quit` flag and check it in the loop, allowing workers to return when signaled. If the goal is to keep the example simple, at minimum document that the workers never return and the only exit is via signal.

## WARNINGS

**1. Resource leak on signal exit (Warning)**

The signal handler calls `exit(0)` directly, bypassing the `rte_eal_cleanup()` call on line 496. This leaves EAL resources (hugepages, device mappings, secondary process shared memory) un-cleaned on every normal shutdown.

While `exit()` will release process-private resources via OS cleanup, shared resources (memzones, rings accessed by secondary processes) may be left in an inconsistent state.

**Suggested fix:** If `exit()` is unavoidable in the signal handler, at minimum call `rte_eal_cleanup()` before `exit(0)`. However, `rte_eal_cleanup()` is also not async-signal-safe, reinforcing that signal-based shutdown is fundamentally unsafe for this use case.

**2. Missing blank line after variable declarations (Warning)**

Line 112-113: The function `print_stats()` declares `unsigned i;` and immediately starts the function body without a blank line separator.

```c
static void
print_stats(int signum)
{
	unsigned i;

	printf("\nAggregate statistics ======================================\n");
```

Per DPDK style, there should be a blank line between variable declarations and statements.

**Suggested fix:** Add a blank line after `unsigned i;`.

## INFO

**1. Pre-existing: array index `p_num` should be `i` (Info - pre-existing)**

Lines 111-112: The loop iterates `for (i = 0; i < num_ports; i++)` but prints `pstats[p_num]` instead of `pstats[i]`. This would print the same port's stats `num_ports` times, which is a logic error.

However, this bug exists in the code before the patch (it is not introduced by the revert) and is therefore pre-existing. The patch does not touch these lines.

**Suggested fix (for a separate patch):**
```c
for (i = 0; i < num_ports; i++)
	printf("Port %u: RX - %u, TX - %u, Drop - %u\n", (unsigned)i,
			pstats[i].rx, pstats[i].tx, pstats[i].drop);
```

**2. Comment clarity (Info)**

Line 95-99: The comment says "This is not safe way to exit for a real production application" but does not specify *why* it is unsafe (async-signal-safety) or what the safe alternative would be (signal flag + main loop check).

**Suggested improvement:**
```c
/*
 * Signal handler configured for SIGTERM and SIGINT to print stats on exit.
 *
 * WARNING: Calls printf() and exit(), which are not async-signal-safe.
 * This is unsafe if the signal interrupts those functions in the main thread.
 * A production application should use a volatile sig_atomic_t flag checked
 * in the main loop, and coordinate shutdown across primary/secondary processes.
 */
```

Review-Result: ERROR