|WARNING| [13/13] examples/eventdev_pipeline: make signal handler safe

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169134

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-06

# DPDK Patch Review

## Patch 01/13: graph: do not call printf in signal

**Correctness:** No issues found.

**Style:** No issues found.

---

## Patch 02/13: examples: remove printf from signal handler

**Correctness:** No issues found.

In `examples/distributor/main.c`, the parameter is now unused so `__rte_unused` is correctly added.

**Style:** No issues found.

---

## Patch 03/13: examples/vmdq: do not print from signal handler

**Correctness:**

**Warning:** `stats_requested` is written from signal context and read from the main lcore, but is declared as `volatile sig_atomic_t`, which is correct. The check `if (stats_requested && lcore_id == rte_get_main_lcore())` then `stats_requested = 0` has a TOCTOU issue if the signal arrives between the check and the clear. However, the consequence is benign (an extra stats print on the next iteration). This is acceptable for a stats-on-demand feature.

**Style:** No issues found.

---

## Patch 04/13: examples/symmetric_mp: do not print or exit in handler

**Correctness:** No issues found.

The patch correctly changes the flow to set a flag in the handler, return 0 from `lcore_main()`, wait for lcores in `main()`, then print stats. The `print_stats()` signature change (removing `signum`) matches the new call site.

**Style:** No issues found.

---

## Patch 05/13: examples/vdpa: make signal handler safe

**Correctness:** No issues found.

The teardown now happens in `main()` after the input loop exits, and also when exiting interactive mode via 'q', which was missing before as noted in the commit message.

**Style:** No issues found.

---

## Patch 06/13: examples/vhost: make signal handler safe

**Correctness:** No issues found.

`quit` is `volatile sig_atomic_t`, written in the handler, checked in `switch_worker()`. The unregister call moves to `main()` after joining lcores.

**Style:** No issues found.

---

## Patch 07/13: examples/vhost_blk: do not tear down from signal handler

**Correctness:** No issues found.

The teardown moves to `main()` after the `sleep(1)` loop exits. The loop checks `quit`, which is `volatile sig_atomic_t`.

**Style:** No issues found.

---

## Patch 08/13: examples/ntb: do not print and re-raise from signal handler

**Correctness:** No issues found.

The previous code called `kill(getpid(), signum)` to re-raise the signal after restoring `SIG_DFL`, which is async-signal-safe but still complex. The new code sets `conf->stopped = 1` for each lcore, and the main loop in `ntb_fwd.c` already checks `conf->stopped`. This is cleaner and safer.

The commit message notes that `SIGINT` now stops forwarding and returns to the `ntb>` prompt instead of terminating, which is a behavior change but arguably an improvement (allows `quit` to do teardown).

**Style:**

`conf->stopped` is now written from signal context. It should be `volatile sig_atomic_t`, but the patch changed it to `volatile uint8_t`. On POSIX systems `uint8_t` is atomic, so this is acceptable. No flag needed.

---

## Patch 09/13: examples/ipsecgw: do not print from signal handler

**Correctness:** No issues found.

**Style:** No issues found.

---

## Patch 10/13: examples/l2fwd-macsec: remove print in signal handler

**Correctness:** No issues found.

**Style:** No issues found.

---

## Patch 11/13: examples/ethtool: fix exit flag and unchecked cmdline

**Correctness:**

**Error:** The call to `rte_panic()` in `ethapp.c` should not be in a signal-safe context check (it's in `ethapp_main()`, not a signal handler), so the addition of the NULL check and `rte_panic()` is fine. However, `rte_panic()` is a forbidden token per the guidelines (it's equivalent to `rte_exit()` which should not be introduced in new code). In this case it's being added to handle a NULL return from `cmdline_stdin_new()`.

**Suggested fix:** Use a different error handling approach:

```c
ctx_cmdline = cmdline_stdin_new(list_prompt_commands, "EthApp> ");
if (ctx_cmdline == NULL) {
	fprintf(stderr, "Cannot create cmdline instance\n");
	return;  /* or return an error code if ethapp_main's signature changes */
}
```

**Style:**

The change to `main.c` making `exit_now` volatile is correct, as it's written from one lcore and read from another.

---

## Patch 12/13: examples/vmdq_dcb: allow exit on signal

**Correctness:**

**Warning:** The loop in `lcore_main()` checks `quit`, which is `volatile sig_atomic_t` written from signal context and read from worker lcores. This is correct. However, the loop also calls `sighup_handler(int signum)` indirectly (via `signal(SIGHUP, sighup_handler)`), and `sighup_handler()` still calls `printf()` which is not async-signal-safe. This patch does not address that. The commit message says "add a SIGINT and SIGTERM handler", which it does, but the SIGHUP handler remains unsafe.

The original patch 03/13 (vmdq) addressed SIGHUP by deferring the stats print to the main lcore. This patch (vmdq_dcb) should do the same for SIGHUP.

**Suggested additional change:** Apply the same pattern to `sighup_handler` as was done in patch 03/13.

**Style:** No issues found.

---

## Patch 13/13: examples/eventdev_pipeline: make signal handler safe

**Correctness:**

**Warning:** The change from `rte_exit(1, ...)` to `_exit(1)` when `fdata->done` is already set is correct for signal safety (the commit message notes this is the "second-signal escape hatch"). However, the exit code is hardcoded to `1`. If the intent is "give up on clean shutdown", an exit code of 130 (128 + SIGINT) or similar would be more conventional. This is a minor style/convention issue, not a correctness bug, but worth noting.

**Style:**

`cdata.dump_dev_signal` is correctly changed to `volatile sig_atomic_t` since it's written from signal context.

The comment in the commit message about SIGTSTP depending on a scheduler lcore is accurate and documents existing behavior, so no flag needed there.

---

## Summary of Findings

### Errors

**Patch 11/13 - examples/ethtool: fix exit flag and unchecked cmdline**

- Use of `rte_panic()` for a non-critical error (failed to create cmdline instance). While the NULL check is good, `rte_panic()` is a forbidden function in new code (equivalent to `rte_exit()`, which calls exit() and is only for unrecoverable EAL failures). Use `fprintf(stderr, ...)` and return an error code instead.

### Warnings

**Patch 12/13 - examples/vmdq_dcb: allow exit on signal**

- The SIGHUP handler still calls `printf()`, which is not async-signal-safe. This patch addresses SIGINT/SIGTERM but leaves SIGHUP unsafe. Apply the same deferred-print pattern used in patch 03/13 (vmdq) to the SIGHUP handler here.

**Patch 13/13 - examples/eventdev_pipeline: make signal handler safe**

- The second-signal `_exit(1)` uses exit code 1. Conventionally, when giving up on clean shutdown after a second signal, the exit code should reflect the signal (e.g., 128 + SIGINT = 130). This is a minor process convention issue, not a correctness bug.

### Info

All other patches (01-10) are correct and follow the guidelines. The series significantly improves signal safety across the examples.