|WARNING| [v2, 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/169162

_AI Code Review_

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

# Code Review for DPDK Patch Series: Signal Handler Safety

## Overview
This patch series addresses signal handler safety issues in DPDK examples and applications. The changes remove non-async-signal-safe function calls (primarily `printf()` and library teardown functions) from signal handlers, replacing them with flag-setting patterns where cleanup occurs in normal execution context.

---

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

**Findings:** None

The change correctly removes `printf()` from the signal handler and sets a flag instead. The flag `force_quit` should ideally be `volatile sig_atomic_t` for strict correctness, but it appears to be used only to exit the program and is likely already defined appropriately elsewhere in the codebase.

---

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

**Findings:** None

All changes correctly remove `printf()` calls from signal handlers. The existing `force_quit` or equivalent flags are set instead, allowing normal shutdown flow.

---

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

**Findings:**

### Error 1: Missing `volatile` on `stats_requested`
**Location:** `examples/vmdq/main.c:468`

```c
static volatile sig_atomic_t stats_requested;
```

This is correct. However, ensure this flag is only accessed atomically.

### Warning 1: Potential Race in Main Lcore Check
**Location:** `examples/vmdq/main.c:546-549`

```c
if (stats_requested && lcore_id == rte_get_main_lcore()) {
    stats_requested = 0;
    print_stats();
}
```

The pattern `if (flag) { flag = 0; }` on a `volatile sig_atomic_t` is correct for signal handling. However, if `rte_get_main_lcore()` could theoretically change during execution or if multiple threads could execute this code, there could be a race. In this context, since `lcore_id` is a parameter to `lcore_main()` and is constant for the lifetime of that function, and only the main lcore clears the flag, this is acceptable.

---

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

**Findings:** None

The patch correctly moves teardown out of the signal handler into `main()` after `rte_eal_mp_wait_lcore()`. The addition of `return 0;` at the end of `lcore_main()` is appropriate.

---

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

**Findings:** None

The change removes library teardown (`vdpa_sample_quit()`) and `exit()` from the signal handler, deferring cleanup to `main()`. The flag pattern is correct.

---

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

**Findings:** None

Similar to patch 05, this correctly defers `unregister_drivers()` to `main()` after joining lcores.

---

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

**Findings:** None

The patch correctly moves controller destruction from signal context to `main()` after the sleep loop exits. The null check on `ctrlr` before accessing it is appropriate.

---

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

**Findings:**

### Info 1: Behavior Change
**Location:** `examples/ntb/ntb_fwd.c:950-960`

The new behavior stops forwarding but does not terminate the application -- it returns to the `ntb>` prompt where `quit` must be typed. This is a functional change from the original "kill self with signal" approach. It should be noted in the commit message that SIGINT no longer terminates the application.

The commit message does state this: "SIGINT no longer terminates the application; it stops forwarding and returns to the ntb> prompt, where quit does the teardown." This is acceptable, but the maintainer should confirm this is the desired behavior.

### Warning 1: Volatile on Array vs Individual Fields
**Location:** `examples/ntb/ntb_fwd.c:44`

```c
volatile uint8_t stopped;
```

The `stopped` field is now `volatile`, which is correct. However, note that `volatile` on a struct member does not make the entire struct volatile. In this case, that's fine -- only `stopped` needs volatility.

---

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

**Findings:** None

Straightforward removal of `printf()` from signal handler.

---

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

**Findings:** None

Straightforward removal of `printf()` from signal handler.

---

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

**Findings:**

### Error 1: Missing Error Handling After `rte_panic()`
**Location:** `examples/ethtool/ethtool-app/ethapp.c:919-920`

```c
if (ctx_cmdline == NULL)
    rte_panic("Cannot create cmdline instance\n");

cmdline_interact(ctx_cmdline);
```

While `rte_panic()` does not return, the code is correct. However, note that `rte_panic()` is generally discouraged in examples (it's acceptable in drivers and libraries but examples should demonstrate proper error handling). A better pattern would be:

```c
if (ctx_cmdline == NULL) {
    fprintf(stderr, "Cannot create cmdline instance\n");
    return;
}
```

This is a **Warning**-level issue: `rte_panic()` in example code is not ideal practice, though not strictly wrong.

### Correctness Note: `volatile int exit_now`
**Location:** `examples/ethtool/ethtool-app/main.c:47`

```c
volatile int exit_now;
```

This should be `volatile sig_atomic_t` for strictest correctness, but since it's not written from a signal handler in this patch (the signal handler is not modified), the change to `volatile` is an improvement. If a future patch adds a signal handler that writes to this flag, it should be changed to `sig_atomic_t`.

---

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

**Findings:** None

The patch correctly adds signal handlers, a quit flag, and joins worker lcores before cleanup. The pattern is consistent with other patches in this series.

---

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

**Findings:**

### Warning 1: `volatile sig_atomic_t` on `dump_dev_signal` but not `done`
**Location:** `examples/eventdev_pipeline/pipeline_common.h:47,72`

```c
struct __rte_cache_aligned fastpath_data {
    volatile sig_atomic_t done;  // Good
    ...
};

struct config_data {
    ...
    volatile sig_atomic_t dump_dev_signal;  // Good
};
```

Both flags are correctly `volatile sig_atomic_t`. However, note that in `schedule_devices()`, the read-modify pattern on `dump_dev_signal`:

```c
if (cdata.dump_dev_signal) {
    rte_event_dev_dump(0, stdout);
    cdata.dump_dev_signal = 0;
}
```

is safe because `sig_atomic_t` operations are atomic, and this is a single-consumer (main loop) of a single-producer (signal handler) flag. The worst case is a missed dump request if the signal arrives between the read and the write, which is acceptable for a diagnostic feature.

### Info 1: Signal Changed from SIGTSTP to SIGUSR1
**Location:** Commit message and `examples/eventdev_pipeline/main.c:354`

The commit message correctly notes this change. SIGUSR1 is indeed more conventional for user-defined actions in daemon-like processes. However, users who were accustomed to pressing Ctrl-Z to dump state will need to adapt. This is an acceptable behavior change given that SIGTSTP (Ctrl-Z) traditionally suspends a process, not dumps state.

### Warning 2: `SA_RESETHAND` on Quit Signal
**Location:** `examples/eventdev_pipeline/main.c:346`

```c
struct sigaction sa = {
    .sa_handler = signal_quit,
    .sa_flags = SA_RESETHAND,
};
```

The use of `SA_RESETHAND` means that after the first SIGINT/SIGTERM, the disposition reverts to `SIG_DFL` (default, which is terminate). This allows a second Ctrl-C to forcibly kill the process if graceful shutdown is hanging. This is good design.

However, ensure that `fdata` is initialized before the signal handlers are installed. The handler does check `if (fdata != NULL)` before accessing it, which is defensive programming. But if a signal arrives before `fdata` is set, the flag won't be set and graceful shutdown won't occur. Looking at the code flow:

```c
sigaction(SIGINT, &sa, NULL);  // handlers installed
...
err = rte_eal_init(argc, argv);  // fdata still NULL
...
memset(&worker_data, 0, sizeof(worker_data));  // fdata assigned later
```

The global `fdata` is assigned in `worker_init()` which is called later. If SIGINT arrives before that, `fdata->done` won't be set. The process will still exit due to `SA_RESETHAND` on the second signal, but this is a minor race window during startup. Consider initializing `fdata` or using a local `volatile sig_atomic_t done` in `main()` that is always valid.

This is an **Info**-level observation -- the existing code is acceptable given the narrow startup window and the `SA_RESETHAND` fallback.

---

## Summary

### Errors
None.

### Warnings

1. **Patch 11** (`examples/ethtool`): Use of `rte_panic()` in example code is discouraged; examples should demonstrate proper error handling with early return or exit.

2. **Patch 13** (`examples/eventdev_pipeline`): Potential startup race where SIGINT before `fdata` initialization won't set `fdata->done`. Mitigated by `SA_RESETHAND` and null check, but consider using a local flag or ensuring `fdata` is valid before signal handler installation.

### Info

1. **Patch 08** (`examples/ntb`): Behavior change where SIGINT no longer terminates the application but stops forwarding. Correctly documented in commit message.

2. **Patch 11** (`examples/ethtool`): `exit_now` changed to `volatile int` but should be `volatile sig_atomic_t` if a signal handler writes to it in the future (not in current patch).

3. **Patch 13** (`examples/eventdev_pipeline`): Signal changed from SIGTSTP (Ctrl-Z) to SIGUSR1 for dump. Users must adapt, but this is more conventional.

---

## Overall Assessment

This patch series is **well-structured and correctly addresses the signal handler safety issues** identified by GCC analyzer. The pattern of setting a `volatile sig_atomic_t` flag in the handler and performing cleanup in normal execution context is correct and follows best practices.

The few warnings are minor and do not represent correctness bugs. The series is ready for acceptance with the understanding that:

- Example code quality bar for `rte_panic()` usage (patch 11) is acceptable but not ideal.
- The behavior changes (SIGINT in ntb, SIGTSTP-SIGUSR1 in eventdev_pipeline) are intentional and documented.