|WARNING| test/pmd_af_packet: cleanups

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch: test/pmd_af_packet: cleanups

### Summary
This patch addresses two Coverity issues related to unchecked `fcntl()` return values and changes some test result codes from SKIPPED to FAILED.

---

## Errors

**1. Incorrect error handling logic for `fcntl(F_GETFL)`**

The patch checks `flags < 0` to detect `fcntl(F_GETFL)` failure.
This is wrong: `fcntl(F_GETFL)` returns the file descriptor flags on success,
which can legitimately be 0 (no flags set).
The correct error check is `flags == -1`.

```c
/* BAD - treats flags == 0 as an error */
if (flags < 0 || fcntl(tap_fd, F_SETFL, flags | O_NONBLOCK) < 0) {

/* GOOD - correct error detection */
if (flags == -1 || fcntl(tap_fd, F_SETFL, flags | O_NONBLOCK) < 0) {
```

Per POSIX, `fcntl()` returns -1 on error and sets `errno`.
The return value for `F_GETFL` is the descriptor flags (a non-negative integer).
Using `< 0` incorrectly rejects the valid case where no flags are set.

**2. Resource leak on `fcntl()` failure**

The new error path returns `TEST_FAILED` without closing `tap_fd`.
The TAP device file descriptor was opened earlier in the function
and must be closed on all error paths to avoid leaking the fd.

```c
/* BAD - leaks tap_fd */
if (flags == -1 || fcntl(tap_fd, F_SETFL, flags | O_NONBLOCK) < 0) {
	printf("fcntl failed: %s\n", strerror(errno));
	return TEST_FAILED;  /* tap_fd is still open */
}

/* GOOD - close tap_fd before returning error */
if (flags == -1 || fcntl(tap_fd, F_SETFL, flags | O_NONBLOCK) < 0) {
	printf("fcntl failed: %s\n", strerror(errno));
	close(tap_fd);
	return TEST_FAILED;
}
```

Review the full function to verify all error paths after `tap_fd` is opened
properly close it before returning.

---

## Warnings

None.

---

## Info

**1. Consider checking for partial transmit**

The patch changes the case where `nb_tx == 0` from SKIPPED to FAILED,
which is reasonable for a loopback test.
However, it does not check whether `nb_tx < allocated` (partial transmit).
In a loopback test, partial transmit may also indicate a problem worth flagging.

```c
/* Current code */
if (nb_tx == 0) {
	printf("No packets transmitted\n");
	return TEST_FAILED;
}

/* Consider also checking partial transmit */
if (nb_tx == 0) {
	printf("No packets transmitted\n");
	return TEST_FAILED;
} else if (nb_tx < allocated) {
	printf("Partial transmit: %u/%u packets sent\n", nb_tx, allocated);
	/* Could return TEST_FAILED or just log a warning */
}
```

This is a suggestion, not a requirement -- the current behavior may be acceptable
if partial transmit is expected in this test scenario.