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

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
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

None.

### Warnings

None.

### Info

The patch correctly identifies that FD 0 (stdin) is valid and switches to using -1 as the sentinel value for invalid file descriptors. This is consistent with Unix conventions. The change is localized and correct.

---

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

### Errors

**File descriptor leak on error path**

In `rte_eal_alarm_init()`, when `rte_intr_callback_register()` fails, the function jumps to the `error:` label. The error path now closes `fd` if `fd >= 0`, which is correct. However, the original code did not set `fd = -1` after the successful `kqueue()` call that creates it. If `rte_intr_fd_set()` fails before the `kqueue()` call, `fd` is still -1 (as initialized), so the `if (fd >= 0)` check correctly skips the close. If `rte_intr_fd_set()` succeeds but `rte_intr_callback_register()` fails, `fd` is valid and will be closed. This is correct.

On second inspection, this is actually correct. The initialization `int fd = -1;` ensures that if we jump to `error:` before `kqueue()` is called, the close is skipped. After `kqueue()` succeeds, `fd` holds a valid descriptor, and the error path will close it. No leak.

Actually, re-reading: the line `int fd = -1;` is added. Previously, `fd` was uninitialized. If `rte_intr_instance_alloc()` failed and jumped to `error:`, the old code would have called `rte_intr_instance_free(intr_handle)` without touching `fd` (since it's declared after the first check). But the old code declared `fd` inside the function body after the first check, so it was never in scope at the `error:` label. Let me re-examine the diff.

Looking at the diff again:

```c
@@ -62,7 +65,7 @@ rte_eal_alarm_cleanup(void)
 int
 rte_eal_alarm_init(void)
 {
-	int fd;
+	int fd = -1;
```

The variable is declared at the start of the function. Previously, if `rte_intr_instance_alloc()` failed, the code returned -1 without declaring `fd` (since the declaration was after the check in the original code, or the diff is showing a reordering). The patch adds:

```c
 error:
+	if (fd >= 0)
+		close(fd);
 	rte_intr_instance_free(intr_handle);
 	return -1;
```

So the error path now closes `fd` if it's valid. The initialization to -1 ensures that if we reach the error label before `kqueue()` succeeds (e.g., if `rte_intr_instance_alloc()` fails, or if `rte_intr_type_set()` fails), the close is skipped. This is correct.

Wait, but the original code did not have an `error:` label at all. Let me re-read the original diff context. The diff shows:

```
 	return 0;
 error:
+	if (fd >= 0)
+		close(fd);
 	rte_intr_instance_free(intr_handle);
 	return -1;
 }
```

The `error:` label was already there in the original code. The patch adds the close. The initialization `fd = -1` ensures the close is conditional on `fd` being valid. This is correct.

Actually, no error here. The patch is correct.

### Warnings

None.

### Info

The patch correctly closes the file descriptor created by `kqueue()` on the error path and in the cleanup function. The initialization of `fd` to -1 ensures the error path does not attempt to close an invalid descriptor.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch initializes all file descriptors in a newly allocated `rte_intr_handle` to -1, following Unix convention. This is a good defensive practice. The unit test is updated to verify this behavior. Driver code that redundantly set fd to -1 after allocation is cleaned up. The FreeBSD PCI code is updated to check `>= 0` instead of truthiness. All changes are correct.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch introduces `rte_intr_fd_close()` and `rte_intr_dev_fd_close()` helpers to encapsulate the pattern of closing a file descriptor and setting it to -1. This reduces code duplication and makes the ownership model clearer. The implementation checks that the fd is valid before closing. The patch correctly updates all call sites. The unit test verifies the new helpers. Release notes are updated. This is a clean refactor with no correctness issues.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch adds a warning in `rte_intr_instance_free()` if the interrupt handle still references a valid file descriptor (fd or dev_fd). This is a useful debugging aid to catch resource leaks. The patch also adds explicit `rte_intr_fd_set(..., -1)` calls in drivers where the fd is not owned by the driver (e.g., owned by ibverbs, VFIO layer, or the application). The comments clarify ownership, which is good documentation. The warnings will help catch leaks during development.

The test code in `app/test/test_interrupts.c` sets fds to -1 before freeing the handle to avoid triggering the new warning, which is correct (the test creates the pipes and is responsible for closing them separately).

Drivers that do not own the fd (e.g., mlx4, mlx5, mana, virtio_user) correctly clear the reference before freeing the handle. Drivers that do own the fd (e.g., cnxk_gpio, which creates the fd itself) should have already closed it via `rte_intr_fd_close()` or equivalent before freeing the handle. The patch adds a few such calls to the error paths (e.g., `mlx5_os_interrupt_handler_create` error path, `cnxk_gpio_register_irq_compat` error path).

Actually, looking at `mlx5_os_interrupt_handler_create`:

```c
 err:
+	rte_intr_fd_set(tmp_intr_handle, -1);
 	rte_intr_instance_free(tmp_intr_handle);
 	return NULL;
```

This is on an error path where the fd was set but the callback registration failed. However, the fd itself is not closed here. Looking at the function, the `fd` parameter is passed in, so the caller owns it. The driver does not own the fd, so it should not close it. Setting it to -1 before freeing the handle is correct to avoid the warning. This is consistent with the "fd is not owned by the driver" pattern.

Similarly, `cnxk_gpio_register_irq_compat`:

```c
 out:
+	rte_intr_fd_set(intr_handle, -1);
 	rte_intr_instance_free(intr_handle);
```

The fd is `gpio->gpiochip_fd`, owned by the gpio structure, not by the intr_handle. The driver should not close it here. Clearing the reference is correct.

Wait, actually in `cnxk_gpio_unregister_irq`:

```c
+	/* fd is owned by gpio, only clear reference here. */
+	rte_intr_fd_set(gpio->intr.intr_handle, -1);
 	rte_intr_instance_free(gpio->intr.intr_handle);
```

So the fd is not closed by the intr code, it's managed separately by the gpio lifecycle. Clearing the reference is correct.

All changes in this patch are correct. The warning will catch cases where a driver forgets to close a fd it created, or forgets to clear a reference to a fd it doesn't own.

---

## Summary

All five patches are correct. They fix resource leaks (FD not closed on error in FreeBSD alarm, FD not closed when freeing interrupt handles), introduce better initialization (fd = -1 on allocation), add convenience helpers (`rte_intr_fd_close`), and add leak detection warnings. The code is well-documented with comments explaining fd ownership. Release notes and unit tests are updated appropriately.

Review-Result: CLEAN