|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

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

**Line 104-112:** The signal handler `print_stats()` calls `printf()` and `exit()`.

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

	printf("\n\n===============\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);
}
```

**Why it matters:** Calling non-async-signal-safe functions from a signal handler is undefined behavior. `printf()` acquires locks internally and can deadlock if the signal arrives while the main thread holds the same lock. `exit()` runs atexit handlers and is also not async-signal-safe.

**Suggested fix:** Set a volatile sig_atomic_t flag in the handler and check it in the main loop, then exit cleanly from normal context. The original code being reverted had this correct pattern.

### 2. Infinite loop with no exit path (Error)

**Line 353:** `lcore_main()` contains an infinite `for (;;)` loop with no mechanism to break out.

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

**Why it matters:** After this revert, there is no way for worker lcores to exit the forwarding loop. The signal handler calls `exit(0)` which terminates the entire process without waiting for worker threads, potentially corrupting shared state and leaving resources uncleaned.

**Suggested fix:** Keep the `quit` flag approach from the original (reverted) code, or use an RTE atomic variable checked in the loop.

### 3. Missing lcore synchronization before cleanup (Error)

**Line 492-495:** The patch removes `rte_eal_mp_wait_lcore()` call.

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

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

**Why it matters:** `rte_eal_cleanup()` is called immediately after launching worker lcores, without waiting for them to finish. This races with the workers which are still accessing DPDK resources. If the signal handler hasn't fired yet, cleanup proceeds while workers are running, causing use-after-free and corruption.

**Suggested fix:** Restore `rte_eal_mp_wait_lcore()` before cleanup.

## WARNINGS

### 1. Missing return statement in non-void function (Warning)

**Line 363:** `lcore_main()` is declared to return `int` but the infinite loop has no return statement.

```c
static int
lcore_main(void *arg __rte_unused)
{
	/* ... */
	for (;;) {
		/* ... */
	}
}
```

**Suggested fix:** Since the signal handler calls `exit()`, either change the return type to `void` or add an unreachable `return 0;` after the loop for compiler satisfaction (with a comment explaining it's unreachable).

### 2. Commit reverts correct bug fix (Warning)

The commit message states the revert is necessary because "the primary calls rte_eal_cleanup() on SIGINT while secondaries are still polling" which crashes secondaries. However, the reverted approach (volatile flag + controlled shutdown) is architecturally correct. The real issue is likely that the example needs additional IPC to coordinate shutdown between primary and secondary processes, not that signal-safe exit is wrong.

**Suggested approach:** Instead of reverting to unsafe signal handling, add proper multi-process coordination or document that SIGINT should only be sent to primary after secondaries have exited.

## Correctness Summary

This patch reintroduces three critical bugs:

1. **Undefined behavior** in signal handler (non-async-signal-safe function calls)
2. **Process termination** without worker thread synchronization (race with `rte_eal_cleanup()`)
3. **Abrupt exit** from signal handler corrupts shared multi-process state

The stated motivation (fixing secondary crashes) addresses a real issue, but the solution chosen makes the example unsafe even in single-process scenarios and violates POSIX signal handling rules.

---

Review-Result: ERROR