|WARNING| [v2] fbarray: check file lock downgrade failure

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch Summary
This patch addresses an unchecked return value from `eal_file_lock()` in the error path of `rte_fbarray_destroy()`. When unlinking the fbarray file fails, the code attempts to downgrade from an exclusive lock to a shared lock but currently ignores the result. The patch adds error checking and preserves the original error code.

---

## Review Findings

### Correctness

**No issues found.**

The patch correctly:
- Saves `errno` immediately after the failed `unlink()` to avoid it being overwritten
- Checks the return value of `eal_file_lock()` which was previously unchecked
- Restores `rte_errno` to the primary failure reason (the unlink error) before returning
- Logs the lock downgrade failure using the DPDK logging infrastructure

The error handling flow is sound. The lock downgrade failure is logged but does not override the primary failure (unlink), which is the correct behavior.

---

### C Coding Style

**Info:**

**Explicit comparison added (good):** The patch changes:
```c
eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN);
```
to:
```c
if (eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN) != 0)
```

This follows DPDK style (explicit comparison against 0 for integer return values) and is the recommended pattern. No issue here.

**Consider:** The comparison could also be written as `< 0` if the function documents that it returns negative errno on failure, but `!= 0` is acceptable and commonly used in DPDK for "check if call failed" semantics.

---

### Documentation and Release Notes

**No issues found.**

This is a bug fix in error path handling (checking a previously unchecked return value). It does not:
- Add or change public API
- Require release notes (internal error handling improvement)
- Change documented behavior

The commit message adequately describes the issue and fix. The `Fixes:` and `Cc: stable at dpdk.org` tags are appropriate for a backportable bug fix.

---

### Process and Tags

**No issues found.**

- `Fixes:` tag references the commit that introduced the unchecked call
- `Cc: stable at dpdk.org` appropriately marks this for stable backport
- Signed-off-by present
- Patch is self-contained and compiles independently

---

## Summary

**Errors:** None

**Warnings:** None

**Info:**
- The explicit comparison `!= 0` is correct DPDK style for checking integer return values

**Overall Assessment:** The patch is correct and ready for merge. It fixes a real bug (unchecked return value in an error path) without introducing new issues. The error handling preserves the original failure reason while logging secondary failures, which is the right approach.