|WARNING| [v2] 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/172195

_AI Code Review_

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

# Review of DPDK Patch: Revert "examples/symmetric_mp: do not print or exit in handler"

## ERRORS

**1. Calling non-async-signal-safe functions in signal handler**

The `print_stats()` function, now used as a signal handler, calls `printf()` and `exit()`.
`printf()` is not async-signal-safe and can deadlock if the signal interrupts a malloc/free
or another stdio operation. This violates POSIX signal safety requirements.

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

	printf("\n");  /* ERROR: printf() not async-signal-safe */
	for (i = 0; i < num_ports; i++) {
		printf("Port %u: RX - %u, TX - %u, Drop - %u\n", ...);  /* ERROR */
	}
	exit(0);  /* ERROR: exit() not async-signal-safe */
}
```

**Suggested fix:** Use the atomic flag pattern that the reverted commit introduced.
Set a `volatile sig_atomic_t quit` flag in the handler, check it in the main loop,
and call `print_stats()` from normal context after the loop exits:

```c
static volatile sig_atomic_t quit;

static void
signal_handler(int signum)
{
	(void)signum;  /* unused parameter */
	quit = 1;
}

/* In main(): */
signal(SIGINT, signal_handler);
signal(SIGTERM, signal_handler);

/* In lcore_main(): */
while (quit == 0) {
	/* forwarding loop */
}

/* After rte_eal_mp_wait_lcore(): */
print_stats();
rte_eal_cleanup();
```

The commit message acknowledges the reverted approach was correct but claims it "crashes the secondaries"
when the primary calls `rte_eal_cleanup()`. That is a separate bug in the shutdown sequence,
not a reason to reintroduce unsafe signal handling.
The correct fix is to make the shutdown sequence safe (e.g., stop secondaries first, or use process monitoring),
not to avoid `rte_eal_cleanup()`.

---

**2. Unreachable code after infinite loop**

The `lcore_main()` function now has an infinite `for (;;)` loop with no exit condition,
making the `return 0;` removal correct but also ensuring the function never returns.
Combined with the unsafe signal handler that calls `exit(0)`, this means `rte_eal_cleanup()`
in `main()` is unreachable.

```c
static void
lcore_main(void *arg __rte_unused)
{
	/* ... */
	for (;;) {  /* infinite loop, never exits */
		/* forwarding */
	}
	/* removed: return 0; */
}

/* In main(): */
rte_eal_mp_remote_launch(lcore_main, NULL, CALL_MAIN);
/* removed: rte_eal_mp_wait_lcore(); */
/* removed: print_stats(); */

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

The signal handler jumps straight to `exit(0)`, so `main()` never regains control
and `rte_eal_cleanup()` is never called. This leaks EAL resources.

**Suggested fix:** Restore the `while (quit == 0)` loop and `rte_eal_mp_wait_lcore()` call.

---

**3. Missing parameter documentation**

The `print_stats()` function signature changed from `void` to `int signum`,
but the parameter is used only to match the signal handler prototype and is never read.
The comment does not document the parameter, and no `(void)signum;` or `__rte_unused` annotation is present.

```c
static void
print_stats(int signum)  /* parameter undocumented and unused */
{
	unsigned i;
	/* signum is never referenced */
```

**Suggested fix:** Add `__rte_unused` attribute or cast to void:

```c
static void
print_stats(int signum __rte_unused)
{
	/* ... */
}
```

Or:

```c
static void
print_stats(int signum)
{
	(void)signum;
	/* ... */
}
```

Note: The commit message says "v2 - need to mark arg unused" but the patch does not actually do this.

---

## WARNINGS

**1. Release notes missing for behavior change**

Reverting a previous fix that addressed unsafe signal handling is a behavior change
that affects application stability. The release notes should document this
(even for example code, since the multi-process examples are widely referenced).

---

## INFO

**1. Comment accuracy**

The new comment says "This is not safe way to exit for a real production application; it is just an example."
This is accurate -- calling `printf()` and `exit()` in a signal handler is indeed unsafe --
but it reads as justification for known-broken code in a shipped example.
DPDK examples should demonstrate correct patterns, not anti-patterns with disclaimers.

The comment further states "Doing proper shutdown with primary/secondary requires process monitoring
and rendezvous." This is true, but the reverted approach (atomic flag + cleanup in main context)
is a significant improvement over the current unsafe handler and does not require process monitoring --
it only requires the primary to finish its own cleanup before terminating,
which is what `rte_eal_cleanup()` is for.

---

## SUMMARY

This patch reintroduces three classes of bugs that the original commit fixed:

1. **Async-signal-safety violation** (`printf()` in handler can deadlock)
2. **Resource leak** (`rte_eal_cleanup()` is now unreachable)
3. **Incomplete implementation** (parameter marked unused in commit message but not in code)

The rationale for the revert -- that `rte_eal_cleanup()` crashes secondaries --
identifies a real issue but solves it by making the primary's shutdown equally broken.
The correct fix is to address the primary/secondary shutdown race,
not to revert to unsafe signal handling.

Review-Result: ERROR