|WARNING| [v2,5/5] interrupts: warn on leaked file descriptors

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/5: vdpa/mlx5: fix check on err interrupt FD

### Errors

**Missing fd reset on error path in `mlx5_vdpa_err_event_setup()`**

Line: `rte_intr_fd_set(priv->err_intr_handle, -1);`

The patch adds this reset when `rte_intr_callback_register()` fails, but does not reset the fd that was set via `mlx5_os_set_dev_fd()` earlier in the function. If that function succeeded before the callback registration fails, the fd remains set in the handle. The correct fix requires resetting it consistently, or documenting that `mlx5_os_set_dev_fd()` does not transfer fd ownership to the handle.

Trace the code path:
1. `mlx5_os_set_dev_fd()` is called - sets fd in handle
2. `rte_intr_callback_register()` fails
3. Only the fd set by `rte_intr_fd_set()` is reset to -1
4. The fd from step 1 remains set

**Potential resource leak: efds array not closed in `mlx5_vdpa_err_event_unset()`**

The function now sets the main fd to -1 via `rte_intr_fd_set(priv->err_intr_handle, -1)` after unregistering, and closes `priv->err_chnl` resources. However, if `rte_intr_efds_index_set()` was used to store additional fds in the efds array, those are not closed. The guidelines require all error paths to release resources.

---

## PATCH 2/5: eal/freebsd: fix a FD leak in the alarm subsystem

### Errors

None identified in this patch. The fd leak fix is correct: `close(fd)` is added on the error path before `rte_intr_instance_free()`, and the successful path already had the fd transferred to the handle and closed on cleanup. The initialization `int fd = -1;` correctly handles the check `if (fd >= 0)` on the error path when allocation fails before `kqueue()`.

---

## PATCH 3/5: interrupts: mark file descriptors invalid on allocation

### Errors

None. The patch correctly initializes all fds to -1 in `rte_intr_instance_alloc()` and `rte_intr_event_list_update()`, updates the API documentation, adds unit test coverage, and converts all callers that were previously doing redundant initialization.

The unit test hack comment is acceptable given the test framework's design (UIO type does not require a device FD in real usage, but the test checks handle validity broadly).

---

## PATCH 4/5: interrupts: close interrupt FDs

### Warnings

**`rte_intr_fd_close()` marked experimental in 26.11 release notes but exported as experimental in 26.11 code**

Line in release notes: `Added ``rte_intr_fd_close()`` and ``rte_intr_dev_fd_close()`` helpers`

The release notes entry states these are added, but does not explicitly mention the experimental tag. Since `rte_intr_fd_close()` uses `RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_intr_fd_close, 26.11)`, this should be noted in the release notes as experimental API to set expectations correctly.

Suggested addition to release notes:
```rst
* Added ``rte_intr_fd_close()`` helper (experimental API).
* Added ``rte_intr_dev_fd_close()`` helper (internal API).
```

---

## PATCH 5/5: interrupts: warn on leaked file descriptors

### Warnings

**Inconsistent fd ownership comment style and potential false positive on virtio_user**

Line in `drivers/net/virtio/virtio_user/virtio_user_dev.c`:
```c
/* fd is not owned by the driver, only clear reference here. */
rte_intr_fd_set(eth_dev->intr_handle, -1);
```

This pattern appears in multiple drivers. While documenting non-ownership is good practice, the commit message states "This will likely raise false positives." The warning in `rte_intr_instance_free()` will trigger for all these cases where drivers correctly clear the fd reference without closing because they don't own it.

Two issues:
1. The log message says "leaking" but these documented cases are not leaks
2. The warning fires at NOTICE level for intended behavior

Consider either:
- Suppressing the warning when fd is exactly -1 (drivers that do the right thing won't trigger it)
- Lowering to DEBUG level
- Qualifying the message: "Some interrupt handle still references a FD (may not be a leak if fd not owned): %d"

The current implementation will train users to ignore this warning because of the known false positives in drivers that correctly manage fd lifetimes.

**Missing `rte_intr_fd_set(..., -1)` in `cnxk_gpio_unregister_irq()`**

Line: `rte_intr_instance_free(gpio->intr.intr_handle);`

The function calls `rte_intr_callback_unregister()` which returns 0 on success, then adds the comment "fd is owned by gpio, only clear reference here" and sets fd to -1. However, in the error path when `rte_intr_callback_unregister()` returns non-zero, the function returns early without clearing the fd reference. This will trigger the new warning on the subsequent free in the error unwind path.

Add before `return ret;`:
```c
rte_intr_fd_set(gpio->intr.intr_handle, -1);
```

---

## Summary

The series correctly addresses file descriptor management in the interrupt subsystem. The main correctness issue is in patch 1 (incomplete error path cleanup). Patch 5 introduces a warning that will have false positives by design; this should be documented or the severity reconsidered.

Review-Result: ERROR