|WARNING| 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/172181

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

### Error 1: Forbidden function in signal handler
**Line:** `exit(0);` in `print_stats()` at line 112

The `print_stats()` function is now a signal handler (registered for `SIGINT` and `SIGTERM`), and it calls `exit(0)`, which is not async-signal-safe. Calling non-async-signal-safe functions from a signal handler causes undefined behavior.

**Why it matters:** Signal handlers can interrupt the program at any point. If `exit()` is called while another thread is in the middle of a malloc operation or holding a lock, the result is undefined behavior - potentially deadlock, corruption, or crash.

**Suggested fix:** Use only async-signal-safe operations in signal handlers. The correct pattern is to set a flag in the signal handler and check it in the main loop:

```c
static volatile sig_atomic_t quit;

static void
signal_handler(int signum)
{
	quit = 1;
}

/* In lcore_main: */
while (!quit) {
	/* ... */
}

/* In main after rte_eal_mp_wait_lcore(): */
if (quit)
	print_stats();
```

### Error 2: Missing call to rte_eal_mp_wait_lcore()
**Line:** Removed at line 493 (deletion shown in patch)

After calling `rte_eal_mp_remote_launch()`, the code must call `rte_eal_mp_wait_lcore()` to wait for worker threads to complete. Without this, the primary process proceeds immediately to `rte_eal_cleanup()` while worker threads are still running, causing crashes.

**Why it matters:** This creates a race condition where `rte_eal_cleanup()` tears down shared resources (mempool, rings, port state) while worker threads are actively accessing them, leading to segmentation faults and undefined behavior.

**Suggested fix:** Restore the call to `rte_eal_mp_wait_lcore()` before cleanup:

```c
rte_eal_mp_remote_launch(lcore_main, NULL, CALL_MAIN);
rte_eal_mp_wait_lcore();

print_stats();

/* clean up the EAL */
rte_eal_cleanup();
```

### Error 3: Unreachable cleanup code
**Line:** `rte_eal_cleanup();` at line 494

The `lcore_main()` function now contains an infinite loop (`for (;;)`) with no break or return path. This means `rte_eal_mp_remote_launch()` will never return, and the cleanup code at line 494 is unreachable.

**Why it matters:** Resources allocated by `rte_eal_init()` are never freed, and the application cannot exit cleanly except via signal, which itself is unsafe (see Error 1).

**Suggested fix:** Restore the `quit` flag and the conditional loop:

```c
while (!quit) {
	/* forwarding logic */
}

return 0;
```

---

## WARNINGS

### Warning 1: Commit message claims revert but makes code worse
The commit message states this reverts commit 9a0b3c409db1963a92d0a91868b1e45254845a8c to fix a crash where the primary calls `rte_eal_cleanup()` while secondaries are still polling. However, this patch does NOT fix that issue - it removes the wait for lcores, making the crash more likely, and introduces unsafe signal handling.

**Why it matters:** The stated problem (primary cleanup racing with secondary polling) requires inter-process coordination, but this patch removes intra-process coordination (waiting for local threads), making the problem worse.

**Suggested fix:** If reverting the original commit is truly necessary, restore it exactly as it was before commit 9a0b3c409. Better: fix the underlying multi-process synchronization issue properly rather than reverting to unsafe signal handling.

### Warning 2: Comment contradicts safe coding practice
**Lines:** 95-100

The comment states "This is not safe way to exit for a real production application" but then includes it anyway in a DPDK example. Examples set the pattern for how users write applications.

**Why it matters:** Users often copy example code. Including explicitly unsafe patterns in examples propagates bad practices into production code.

**Suggested fix:** Either implement the safe version (as the reverted commit attempted) or remove this example from DPDK if safe multi-process shutdown is too complex to demonstrate.

---

## Review Summary

This patch attempts to simplify multi-process shutdown by reverting to signal-based exit, but introduces three critical errors:

1. Calls non-async-signal-safe `exit()` from a signal handler (undefined behavior)
2. Removes the wait for worker threads to complete before cleanup (race condition, crashes)
3. Creates an infinite loop that makes cleanup code unreachable

The patch makes the code objectively worse than what it's reverting. If the original commit caused crashes, the correct fix is to add proper inter-process synchronization, not to introduce new undefined behavior and remove intra-process synchronization.

Review-Result: ERROR