|WARNING| [v3] 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/172196

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

**1. Missing `rte_eal_mp_wait_lcore()` after remote launch (Error)**

Line: `rte_eal_mp_remote_launch(lcore_main, NULL, CALL_MAIN);`

After `rte_eal_mp_remote_launch()`, the application must call `rte_eal_mp_wait_lcore()` before cleanup or exit. The removed call is necessary to wait for all worker lcores to finish before proceeding. Without it, the main thread may continue while workers are still running, or the application may hang at exit.

**Suggested fix:** Restore the `rte_eal_mp_wait_lcore()` call:
```c
rte_eal_mp_remote_launch(lcore_main, NULL, CALL_MAIN);
rte_eal_mp_wait_lcore();
```

**2. Infinite loop in `lcore_main()` with no clean exit (Error)**

Line: `for (;;) {`

The function now contains an infinite loop with no break condition and does not return. This means `rte_eal_mp_wait_lcore()` will never complete even if it were present. Worker threads must be able to terminate cleanly for proper application shutdown.

Combined with error #1, this creates a situation where:
- If `rte_eal_mp_wait_lcore()` is present, it will never return (workers loop forever)
- If `rte_eal_mp_wait_lcore()` is absent, workers continue running after main exits

**Suggested fix:** Keep the `quit` flag mechanism to allow controlled termination of worker threads.

**3. `exit()` called from signal handler (Error)**

Line: `exit(0);` in `print_stats()`

Calling `exit()` from a signal handler is unsafe. `exit()` is not async-signal-safe per POSIX and calls atexit handlers, flushes stdio buffers, and performs cleanup that may deadlock if interrupted code held locks. This violates the signal safety requirements mentioned in AGENTS.md under "Code that is not safe against signals."

The original code with a `volatile sig_atomic_t quit` flag was the correct signal-safe approach.

**Suggested fix:** Restore the signal-safe flag-based termination:
```c
static volatile sig_atomic_t quit;

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

## WARNINGS

**1. Removal of cleanup path breaks resource management (Warning)**

The patch removes the normal cleanup flow (wait for workers, print stats, cleanup EAL) in favor of abrupt `exit()` from a signal handler. This prevents proper resource cleanup including:
- Stopping ports
- Freeing mempools  
- Releasing EAL resources

While the commit message states this is intentional for simplicity, it demonstrates an anti-pattern that example code should not promote.

**2. Documentation of unsafe practice does not make it safe (Warning)**

The comment states:
```c
* This is not safe way to exit for a real production application; it is just an example.
```

While documenting the limitation is better than nothing, example code should demonstrate correct patterns, not incorrect ones with a disclaimer. The original code with the `quit` flag showed a proper signal-safe termination mechanism and should be retained.

---

## Summary

This revert reintroduces multiple correctness bugs in the name of simplicity:
1. Calls async-signal-unsafe `exit()` from signal handler
2. Creates infinite loops in worker threads with no termination path
3. Removes the `rte_eal_mp_wait_lcore()` call needed for proper lcore synchronization
4. Abandons proper cleanup in favor of abrupt exit

The original code being reverted had the correct signal-safe pattern. The multi-process crash mentioned in the commit message should be fixed differently (e.g., coordinate shutdown between processes) rather than reverting to unsafe code.

Review-Result: ERROR