|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: Unsafe Signal Handler - exit() in Signal Context
**Line:** `exit(0);` in `print_stats()` at the end of the function

The patch makes `print_stats()` a signal handler and calls `exit(0)` from within it. `exit()` is not async-signal-safe according to POSIX. Calling it from a signal handler can deadlock if the signal interrupts `malloc()`, `free()`, or other non-reentrant functions that `exit()` cleanup may invoke (stdio flush, atexit handlers, destructors).

**Why it matters:** This can cause hangs or crashes when the application is interrupted, especially under load when the signal is likely to interrupt allocation or I/O.

**Suggested fix:** If a simple exit-on-signal pattern is required for an example, use `_exit(0)` instead of `exit(0)`. `_exit()` is async-signal-safe and terminates immediately without cleanup:

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

	printf("\nAggregate statistics ==============================\n");
	for (i = 0; i < proc_info->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);
}
```

### Error 2: Unreachable Code After Infinite Loop
**Lines:** `for (;;) { ... }` in `lcore_main()`, followed by removed `return 0;`

The function `lcore_main()` now contains an infinite loop with no break or return path. The function is declared to return `int` (it's an lcore function), but the infinite loop makes any code after it unreachable. While the signal handler now calls `exit(0)` to terminate, the function signature still promises to return an `int`, and the compiler may warn about a missing return statement.

**Why it matters:** Violates function contract and produces compiler warnings.

**Suggested fix:** Change the function return type to `void` if the function is never expected to return, or keep the infinite loop but document that termination is via signal handler only. Alternatively, if using the signal-safe `_exit()` approach above, consider keeping a flag-based exit path:

```c
static volatile sig_atomic_t quit;

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

static int
lcore_main(void *arg __rte_unused)
{
	/* ... */
	while (!quit) {
		/* ... packet processing ... */
	}

	/* Print stats before exit */
	unsigned i;
	printf("\nAggregate statistics ==============================\n");
	for (i = 0; i < proc_info->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);

	return 0;
}
```

### Error 3: Missing rte_eal_mp_wait_lcore() - Resource Leak
**Line:** Removed `rte_eal_mp_wait_lcore();` after `rte_eal_mp_remote_launch()`

The patch removes `rte_eal_mp_wait_lcore()` which waits for all launched lcores to finish. With the new signal handler calling `exit(0)`, worker threads may still be running when the process exits, causing abrupt termination of threads mid-operation. This can leave resources in inconsistent states and prevents graceful shutdown.

**Why it matters:** Worker threads are not joined before process exit, violating thread lifecycle management. Resources held by worker threads (locks, file descriptors, etc.) are not properly released.

**Suggested fix:** If using a flag-based approach (see Error 2), restore the wait:

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

If keeping the `exit(0)` approach, document explicitly that this is unsafe for production use and is demonstration-only code.

---

## WARNINGS

### Warning 1: Removed rte_eal_cleanup() Call
**Context:** The patch removes the call to `rte_eal_cleanup()` at the end of `main()`

While the commit message explains this is intentional (calling `rte_eal_cleanup()` in multi-process context was causing crashes), removing it without replacement means EAL resources are never released. For an example application that now exits via signal handler, this may be acceptable, but it should be documented.

**Suggested fix:** Add a comment explaining why cleanup is omitted:

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

/* Note: rte_eal_cleanup() is not called because this example exits
 * via signal handler. Proper multi-process shutdown would require
 * coordinated termination of all processes. */
```

### Warning 2: Unclear Comment About Production Safety
**Line:** `/* This is not safe way to exit for a real production application; it is just an example. */`

The comment acknowledges the safety issue but doesn't specify *what* is unsafe. Given the use of `exit()` in a signal handler and the lack of thread synchronization, the comment should be more specific.

**Suggested fix:**

```c
/*
 * Signal handler configured for SIGTERM and SIGINT to print stats on exit.
 *
 * WARNING: This example uses exit() from a signal handler, which is not
 * async-signal-safe. Worker threads are not gracefully stopped before exit.
 * This is demonstration code only; production applications must use proper
 * shutdown signaling (e.g., setting a volatile flag) and coordinated
 * termination across processes.
 */
```

---

## INFO

### Info 1: Commit Message Describes the Problem but Not the Solution Trade-offs
The commit message explains why the previous approach (calling `rte_eal_cleanup()`) was incorrect, but it doesn't explain the safety implications of the reverted approach. It would be helpful to document that this revert trades primary/secondary crash issues for signal-safety issues, and that both approaches are unsuitable for production.

---

## SUMMARY

The patch reverts to a simpler but unsafe signal handling approach for an example application. The primary issues are:

1. **Critical:** `exit()` called from signal handler is not async-signal-safe
2. **Critical:** Infinite loop in `lcore_main()` makes return statement unreachable
3. **Critical:** Worker threads not properly joined before process exit

While the patch may address the immediate multi-process crash issue described in the commit message, it introduces signal-safety violations that could cause different failure modes. For an example application, using `_exit()` instead of `exit()` would be a minimal fix, though a flag-based graceful shutdown would be more appropriate.

Review-Result: ERROR