|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

This patch reverts a change that attempted to make signal handlers safer by setting a flag and cleanly shutting down, but the revert is warranted because the clean shutdown approach doesn't work in a multi-process context where the primary calls `rte_eal_cleanup()` while secondaries are still running.

## ERRORS

**Unchecked return value from `rte_eal_cleanup()` at line 495 (added code)**

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

After the revert, `rte_eal_cleanup()` is called but its return value is not checked. This function can fail (returns negative errno on error) and the failure should be logged or handled. The application will exit immediately after due to process termination from the signal handler, but the unchecked call means any cleanup errors are silently ignored.

**Suggested fix:** Check the return value and log on error:

```c
/* clean up the EAL */
ret = rte_eal_cleanup();
if (ret != 0)
	RTE_LOG(ERR, APP, "EAL cleanup failed: %s\n", rte_strerror(-ret));
```

---

**Function `lcore_main()` no longer returns a value but is still used with `rte_eal_mp_remote_launch()`**

After the revert:
- Line 351: `lcore_main()` has an infinite loop `for (;;)` with no `return` statement
- Line 492: `rte_eal_mp_remote_launch(lcore_main, NULL, CALL_MAIN)` expects `lcore_main()` to return `int`

The function signature is `static int lcore_main(void *arg __rte_unused)` but the body never returns. This causes undefined behavior if the signal handler somehow doesn't terminate the process (e.g., if signals are blocked or the handler fails). The function should either return `int` properly or be declared `noreturn`.

**Suggested fix:** Add `__rte_noreturn` attribute:

```c
__rte_noreturn
static int
lcore_main(void *arg __rte_unused)
```

Or ensure a return path exists (but this example intends to run forever until signal).

---

**Resource leak: `mp` mempool and port resources not cleaned up on signal exit**

Line 113: `exit(0)` in the signal handler terminates the process immediately without cleanup:
- The mempool `mp` allocated at line 470-475 is not freed
- Ports configured at line 478 are not stopped or closed
- `rte_eal_mp_wait_lcore()` removed at line 493 means worker threads are not joined before cleanup

In the reverted code, the signal handler calls `exit(0)` directly, bypassing all cleanup. While the OS will reclaim memory, DPDK resources (hugepages, device state) may not be properly released, potentially affecting subsequent runs or leaving devices in inconsistent states.

This is acknowledged in the commit message ("Doing shutdown correctly in a multi-process application needs both processes to monitor each other") but it is still a correctness bug: the example leaks resources on every normal exit.

**Suggested fix:** Document the limitation more prominently and consider a basic cleanup path:

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

	printf("\n======  Symmetric multi process APP stats  ======\n");
	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);
	
	/* Note: In multi-process context, proper cleanup requires coordination.
	 * This example exits directly for simplicity. */
	_exit(0);  /* Use _exit to skip C library cleanup */
}
```

Or keep the signal-driven flag and add minimal cleanup that doesn't touch shared state.

---

## WARNINGS

**Calling `exit()` from a signal handler is not async-signal-safe**

Line 113: `exit(0)` in `print_stats()` signal handler.

POSIX lists `exit()` as **not** async-signal-safe. Calling it from a signal handler can deadlock if the signal interrupts a non-reentrant function. Only `_exit()` is safe.

The comment at line 97-99 acknowledges this is not safe ("This is not safe way to exit for a real production application"), but it's still a correctness issue for the example.

**Suggested fix:** Use `_exit()` instead:

```c
_exit(0);
```

---

**`printf()` in signal handler is not async-signal-safe**

Lines 108-111: `printf()` calls in `print_stats()` signal handler.

`printf()` is not async-signal-safe per POSIX. If the signal interrupts another `printf()` or memory allocation, this can deadlock or corrupt stdio state.

Again, acknowledged as unsafe by the comment, but it's a real issue.

**Suggested fix:** Use `write()` to stderr instead, or accept the limitation and document it clearly in the code comment.

---

**Missing bounds check on `num_ports` in `print_stats()`**

Line 109: Loop `for (i = 0; i < num_ports; i++)` accesses `pstats[p_num]` but `num_ports` comes from `rte_eth_dev_count_avail()` (line 465) and could be larger than the `pstats` array size if the application didn't allocate enough.

However, `pstats` is a global array sized `MAX_PORTS` (defined elsewhere), and line 467 checks `if (num_ports < 1 || num_ports > MAX_PORTS)` and aborts. So `num_ports <= MAX_PORTS` is guaranteed at the point `print_stats()` runs.

**Not a bug** - bounds are correctly enforced earlier. Do not flag.

---

**Signal handlers not restored or explicitly ignored**

Lines 449-450: Signal handlers installed with `signal()`:

```c
signal(SIGINT, print_stats);
signal(SIGTERM, print_stats);
```

After the signal fires once and `exit(0)` is called, the process terminates, so re-entrancy is not an issue here. However, `signal()` has implementation-defined behavior regarding handler persistence. POSIX recommends `sigaction()` for portable code.

For an example application, this is acceptable, but for robustness:

**Suggested fix:** Use `sigaction()` with `SA_RESETHAND` if single-shot behavior is desired, or document that the handler is not re-entrant safe.

---

**Removed `rte_eal_mp_wait_lcore()` means `main()` exits while workers are running**

Line 493 (removed): `rte_eal_mp_wait_lcore()` was deleted.

After `rte_eal_mp_remote_launch()` at line 492, the main thread immediately proceeds to `rte_eal_cleanup()` at line 495 without waiting for worker threads to finish. This is intentional (workers run forever until signal), but it means `main()` falls through to `rte_eal_cleanup()` while workers are still in `lcore_main()`.

The comment in the commit message explains this is the intended behavior (workers exit via signal), but it's worth noting that `rte_eal_cleanup()` is called before workers terminate, which could cause races if workers are still accessing EAL resources.

However, in practice the signal handler calls `exit(0)` which terminates all threads, so `rte_eal_cleanup()` is never reached during normal signal-driven exit. The only way to reach line 495 is if `rte_eal_mp_remote_launch()` fails, but that's not checked either.

**Suggested fix:** Check return value of `rte_eal_mp_remote_launch()`:

```c
ret = rte_eal_mp_remote_launch(lcore_main, NULL, CALL_MAIN);
if (ret != 0)
	rte_exit(EXIT_FAILURE, "Failed to launch workers\n");

/* Workers run forever until signal; main thread blocks or exits */
pause();  /* Wait for signal */
```

Or add a blocking wait construct so `main()` doesn't fall through to cleanup while workers run.

---

## INFO

**Code comment acknowledges the shutdown pattern is unsafe**

Lines 95-100: The comment explicitly states this is not a safe production pattern and that proper shutdown requires process monitoring. This is good documentation of the limitation. The revert is justified by the Bugzilla reference and commit message.

No action needed - this is informational context for reviewers.

---

Review-Result: ERROR