|WARNING| [9/9] net/mana: use common sysfs routines

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: Add Common Sysfs Value Routines

## Summary
This 9-patch series introduces common sysfs value parsing routines in EAL
and converts existing driver/library code to use them.
It replaces scattered open-coded implementations with a unified API.

---

## Patch 1/9: eal: add common sysfs value routines

### Errors

**Resource leak (Error):**
In `eal_unix_sysfs.c:sysfs_read_line()`, the FILE pointer `f` is leaked
on `vsnprintf()` truncation:

```c
len = vsnprintf(path, sizeof(path), format, ap);
if (len < 0 || len >= (int)sizeof(path)) {
    EAL_LOG(ERR, "sysfs path too long");
    return -1;  /* ERROR: f not opened yet, no leak on this path */
}

f = fopen(path, "r");
if (f == NULL) {
    /* ... */
    return -1;  /* CORRECT: f is NULL, no leak */
}

if (fgets(buf, buflen, f) == NULL) {
    EAL_LOG(ERR, "cannot read %s", path);
    fclose(f);
    return -1;  /* CORRECT: f closed */
}
fclose(f);      /* CORRECT: f closed on success path */
```

This is actually **correct** -- there is no leak.
The early return for path truncation occurs before `fopen()`,
so `f` does not need to be closed.

Wait -- re-reading the code: there is **no error** here.
I was mistaken.
The paths are:
1. vsnprintf fails -> return (no fopen yet, no leak)
2. fopen fails -> return (f is NULL, no leak)
3. fgets fails -> fclose then return (cleaned up)
4. success -> fclose then return (cleaned up)

All paths are correct. No leak.

### Warnings

**Test coverage (Warning):**
`app/test/test_eal_fs.c` adds extensive tests for the new routines,
which is good practice.
However, the test does not exercise `rte_sysfs_write_string()`
error paths (e.g., writing to a read-only file or a file that rejects the value).
This is acceptable for an initial implementation but worth noting.

**Release notes (Warning):**
The patch adds new internal API (`__rte_internal`) but does not update
`doc/guides/rel_notes/release_25_03.rst` (assuming target is 25.03 based on the date).
New internal API exposed to drivers should be documented in release notes
even if not public API.

---

## Patch 2/9: dma/idxd: use common sysfs routines

### Info

The `wq "size"` and `"max_batch_size"` attribute conversion changes from `fscanf("%u")`
to `strtoul(..., 0)` are noted in the commit message.
The kernel does not print these with leading zeros, so the octal parsing difference is academic.

No errors.

---

## Patch 3/9: common/ionic: use common sysfs routines

### Warnings

**Uninitialised variable (Warning):**
In `ionic_common_uio.c`, the original `uio_get_res_*()` functions
initialised their unsigned long variables before the `fopen()` check.
The new code relies on `rte_sysfs_parse_uint()` leaving `size`/`offset`/`addr`
unmodified on failure.

The callers treat `0` as an error value and the comment says
"zero is the error value for all of these".
However, `rte_sysfs_parse_uint()` does **not** guarantee the output is unmodified
on error -- its documentation says "unmodified on failure" but the implementation
in Patch 1 does not initialise `*val` before returning -1 on error.

So if the file does not exist, `rte_sysfs_parse_uint()` returns -1,
the `if` branch is taken, and the function `return 0;` is correct.
Wait, re-reading the code:

```c
static unsigned long
uio_get_res_size(int uio_idx, int res_idx)
{
	unsigned long size;

	/* zero is the error value for all of these */
	if (rte_sysfs_parse_uint(&size, "/sys/class/uio/uio%d/maps/map%d/size",
			uio_idx, res_idx) < 0)
		return 0;

	return size;
}
```

The comment is misleading: the function returns 0 **on error** (file missing or parse failure),
not because the attribute contained zero.
But if `rte_sysfs_parse_uint()` does not modify `size` on error,
then `size` is uninitialised and the `return size;` at the end
would return a random value on the error path... except that cannot happen
because the error case `return 0;` is hit first.

So the code is correct, but fragile.
The original code set `size = 0` before the fopen/fscanf, making it explicit.

Actually, wait. Let me re-read `rte_sysfs_parse_uint()` from Patch 1:

```c
int
rte_sysfs_vparse_uint(unsigned long *val, const char *format, va_list ap)
{
	const char *start;
	char buf[BUFSIZ];
	unsigned long tmp;
	char *end;

	if (sysfs_read_line(buf, sizeof(buf), format, ap) < 0)
		return -1;  /* val NOT modified on this path */

	/* ... parsing ... */
	if (/* parse error */) {
		EAL_LOG(ERR, "cannot parse sysfs value '%s'", buf);
		return -1;  /* val NOT modified on this path */
	}

	*val = tmp;   /* val modified ONLY on success */
	return 0;
}
```

So yes, `*val` is **not** modified on error.
But the ionic code does:

```c
unsigned long size;
if (rte_sysfs_parse_uint(&size, ...) < 0)
    return 0;
return size;
```

If the parse fails, the function returns 0 immediately.
The `return size;` statement is only reached when the parse **succeeds**,
at which point `size` has been assigned by `rte_sysfs_parse_uint()`.

So there is **no use of uninitialised variable**.
The code is correct.

No errors.

---

## Patch 4/9: bus/vmbus: use common sysfs routines

### Info

The change from `fscanf("%u")` to base 0 conversion is noted.

No errors.

---

## Patch 5/9: power: use common sysfs routines

### Warnings

**Incorrect type conversion (Warning):**
`power_sysfs_read_u32()` in `power_common.c`:

```c
int
power_sysfs_read_u32(uint32_t *val, const char *format, ...)
{
	unsigned long tmp;
	va_list ap;
	int ret;

	va_start(ap, format);
	ret = rte_sysfs_vparse_uint(&tmp, format, ap);
	va_end(ap);
	if (ret < 0)
		return -1;

	*val = tmp;  /* WARNING: potential truncation */
	return 0;
}
```

On 64-bit systems, `unsigned long` is 64 bits and `uint32_t` is 32 bits.
If a sysfs attribute contains a value > UINT32_MAX,
the assignment `*val = tmp;` silently truncates.

None of the sysfs attributes read by the power library should ever be > 2^32 - 1
(CPU frequencies are in kHz, so even 10 THz would be 10^10 kHz which is < 2^32),
but a truncation check would be safer:

```c
if (tmp > UINT32_MAX) {
    EAL_LOG(ERR, "value %lu exceeds uint32_t range", tmp);
    return -1;
}
*val = (uint32_t)tmp;
```

This is a **Warning** rather than Error because the attributes in question
are kernel-provided CPU frequency values that realistically fit in 32 bits.

---

## Patch 6/9: drivers/bus: remove duplicate sysfs string helpers

### Info

Straightforward replacement of local helpers with the EAL routine.

No errors.

---

## Patch 7/9: common/mlx5: use common sysfs routines

### Info

The `roce_enable` read/write is simplified.
The change from "return -errno" to "return -EIO" on open failure
is noted in the commit message and is acceptable (error code used only for logging).

No errors.

---

## Patch 8/9: net/mlx5: use common sysfs routines

### Warnings

**Type conversion (Warning):**
In `mlx5_ethdev_os.c:mlx5_sysfs_bond_info()`:

```c
unsigned long val;
/* ... */
if (rte_sysfs_parse_uint(&val, "/sys/class/net/%s/master/ifindex", name) != 0) {
    rte_errno = ENODEV;
    return -rte_errno;
}
index = val;
```

`val` is `unsigned long` (64-bit), `index` is `unsigned int` (32-bit).
Network interface indexes are realistically < 2^32,
but a truncation check or cast would make the intent explicit.

This is the same pattern as Patch 5 and is a **Warning** for the same reason.

---

## Patch 9/9: net/mana: use common sysfs routines

### Warnings

**Type conversion (Warning):**
Same pattern: `unsigned long val` assigned to `unsigned int dev_port`.

---

## General Observations

**Release notes:**
None of the patches update `doc/guides/rel_notes/`.
The new API is internal (`__rte_internal`), so it does not require
a "New Features" entry for public API.
However, a brief mention in the "Other" section or in the subsystem
update logs (e.g., "EAL: consolidated sysfs parsing into common routines")
would be helpful for developers who grep for sysfs-related changes.

**Test coverage:**
Patch 1 adds extensive unit tests in `app/test/test_eal_fs.c`,
which is excellent.
The tests cover valid values, invalid values, edge cases (no newline, negative for unsigned),
and both signed and unsigned variants.

**Consistency:**
The series converts all reachable callers of the old `eal_parse_sysfs_value()`,
which is good for avoiding dead code.

---

## Final Checklist

- [x] Each commit compiles independently (assumed based on patch structure)
- [x] Code and docs updated together (no new user-facing docs, internal API)
- [x] Release notes: **Missing** (Warning -- should note EAL internal API addition)
- [x] Tests added: yes (Patch 1)
- [x] Forbidden tokens: none detected
- [x] Style: consistent with DPDK conventions
- [x] Correctness bugs: none found after re-analysis

---

## Recommendation

The series is well-structured and the consolidation of sysfs parsing is a good cleanup.

**Must fix (Warning):**
- Add release notes entry for the new internal API in EAL

**Should consider (Info):**
- Add truncation checks when assigning `unsigned long` -> `uint32_t` / `unsigned int`
  in power/mlx5/mana patches (or document why truncation is safe)
- Add `rte_sysfs_write_string()` error path tests