|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: series.patch

## Summary

This series introduces common sysfs helper functions in EAL and converts existing drivers to use them. The overall approach is sound--consolidating the pattern "build path with snprintf, open file, read value, parse with fscanf" into single EAL calls removes duplicated code, improves error handling, and eliminates several fscanf pitfalls.

---

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

### Errors

**lib/eal/unix/eal_unix_sysfs.c:55**
```c
*strchrnul(buf, '\n') = '\0';
```
If `buf` is zero-length or `buflen` is 0, this writes to an indeterminate location (fgets guarantees NUL termination only if it reads at least one character). Add a check:
```c
if (buf[0] != '\0')
	*strchrnul(buf, '\n') = '\0';
```

**lib/eal/unix/eal_unix_sysfs.c:83-84 (rejection of negative values in rte_sysfs_vparse_uint)**
The skip-whitespace-then-check-for-minus logic is correct but relies on `isspace((unsigned char)*start)` correctly handling the zero terminator. If `buf` contains only whitespace and no newline, `while (isspace(...))` will loop until `*start == '\0'`, then `*start == '-'` will be false. This is safe.

**lib/eal/unix/eal_unix_sysfs.c:92 (rte_sysfs_parse_uint)**
```c
int rte_sysfs_parse_uint(unsigned long *val, const char *format, ...)
```
Calls `rte_sysfs_vparse_uint`, which writes to `*val` only on success. If the caller passes an uninitialized `val`, it remains uninitialized on failure. Document that `val` is unmodified on failure, or zero it on error for defensive programming. Current code is acceptable (caller should not read `val` if return is < 0), but a comment would help.

### Warnings

**app/test/test_eal_fs.c:47**
The test opens a temporary file with `mkstemp`, then closes it and uses `/proc/self/fd/%d` to get the path. On systems where `/proc` is not mounted or fd symlinks are not supported, the test will fail. The test already skips on FreeBSD; consider documenting that `/proc` is required or adding a runtime check.

**lib/eal/include/rte_sysfs.h:44 (rte_sysfs_parse_uint Doxygen)**
```c
 * @param val
 *   Where to store the parsed value, unmodified on failure.
```
"unmodified on failure" is correct but not verified by code inspection--`strtoul` may clobber `val` even if parsing fails. Update the implementation to store to a temporary variable and only write `*val` on success, or adjust the documentation to say "undefined on failure, caller must check return value".

**lib/eal/unix/eal_unix_sysfs.c:40-41 (missing ENOENT logs vs EIO)**
`sysfs_read_line` logs ENOENT at DEBUG but other errors at ERR. This is fine for attributes that may not exist (e.g., numa_node), but if a caller expects the file to always be present, the DEBUG log is too quiet. Consider logging all errors at DEBUG in `sysfs_read_line` and having callers log at ERR if they need louder diagnostics. Current code is acceptable.

### Info

**lib/eal/unix/eal_unix_sysfs.c:84**
```c
if (end == start || *end != '\0' || errno != 0 || *start == '-') {
```
The `errno != 0` check assumes `strtoul` sets errno on all errors. Per POSIX, `strtoul` sets errno to ERANGE on overflow but does not guarantee setting it for invalid input. The `end == start` check catches empty/invalid strings, so this is redundant but harmless. Consider removing `errno != 0` or documenting the assumption.

**lib/eal/include/rte_sysfs.h:34**
Doxygen says "A negative value is rejected rather than wrapping." This is misleading--`strtoul` itself wraps negative values into large unsigned numbers; the manual minus-sign check after whitespace skip is what rejects them. Rephrase: "Rejects negative values (which strtoul would otherwise wrap to large unsigned values)."

---

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

### No issues found.

The conversion is straightforward. The switch from `fscanf("%u")` to base 0 conversion is noted; the kernel does not print these attributes with leading zeros.

---

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

### No issues found.

The helpers used unchecked `sprintf()` into 64-byte buffers and `fscanf()` without error checking. The EAL routines handle this correctly.

---

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

### Warnings

**drivers/bus/vmbus/linux/vmbus_uio.c:339 (vmbus_uio_sysfs_read error return)**
```c
if (rte_sysfs_parse_uint(val, "%s/%s", dir, name) < 0) {
	VMBUS_LOG(ERR, "can't read %s/%s", dir, name);
	return -EIO;
}
```
The original code returned `-errno` on open failure and `-EIO` on parse failure. The new code always returns `-EIO`. This loses the distinction between ENOENT (file doesn't exist) and other errors, but callers only use the return value in `strerror(-err)` log messages, so the message text change is minor. Acceptable.

---

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

### Errors

**lib/power/power_common.c:93 (power_sysfs_read_u32)**
```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;
	return 0;
}
```
If `tmp` exceeds `UINT32_MAX`, the cast `*val = tmp` silently truncates. On 64-bit systems, `unsigned long` is 64 bits. Add a range check:
```c
if (tmp > UINT32_MAX) {
	POWER_LOG(ERR, "value %lu exceeds uint32_t range", tmp);
	return -1;
}
*val = tmp;
```

**drivers/power/amd_pstate/amd_pstate_cpufreq.c:118 (power_check_turbo)**
The original code had:
```c
FILE *f_nom = NULL, *f_max = NULL;
...
err:
	if (f_max != NULL) fclose(f_max);
	if (f_nom != NULL) fclose(f_nom);
```
The new code removes the NULL checks because those variables no longer exist. However, the original `err:` label was reachable from lines that did not initialize `f_nom` and `f_max`, so the NULL checks prevented double-free. The new code is correct (no FILE pointers to clean up), so this is not a bug. No action needed.

---

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

### No issues found.

Both `get_sysfs_string` (vmbus) and `read_sysfs_string` (platform) are replaced by `rte_sysfs_parse_string`. The newline stripping is equivalent.

---

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

### No issues found.

`mlx5_sys_roce_disable` now reads and writes with EAL routines. The logic is equivalent.

---

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

### No issues found.

The bonding index reads now use `rte_sysfs_parse_uint`. The note about leading-zero octal interpretation is correct; the kernel does not print ifindex with leading zeros.

---

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

### No issues found.

`dev_port` is now read with `rte_sysfs_parse_uint` and cast to `unsigned int`. The kernel does not print it with leading zeros.

---

## Cross-Cutting Concerns

### API Tag Requirements

**lib/eal/include/rte_sysfs.h**
All four functions (`rte_sysfs_parse_uint`, `rte_sysfs_vparse_uint`, `rte_sysfs_parse_int`, `rte_sysfs_parse_string`, `rte_sysfs_write_string`) are marked `__rte_internal`. They are declared in a driver_sdk_headers file (`rte_sysfs.h`), so internal tagging is correct. The corresponding `RTE_EXPORT_INTERNAL_SYMBOL` macros in `eal_unix_sysfs.c` are present.

**lib/power/power_common.h:53 (power_sysfs_read_u32)**
```c
__rte_internal
int power_sysfs_read_u32(uint32_t *val, const char *format, ...)
```
Marked internal and exported with `RTE_EXPORT_INTERNAL_SYMBOL`. Correct.

### Structure and Testing

**app/test/test_eal_fs.c**
The test is updated to cover:
- Missing file (ENOENT)
- Decimal value
- Hex value with `0x` prefix
- Value without trailing newline
- Negative value rejected by unsigned, accepted by signed
- String read with newline stripped
- String write and read-back
- Empty file
- Trailing garbage
- Non-numeric content
- Negative value as unsigned
- Negative value with leading whitespace (to verify the whitespace-skip logic)

This is comprehensive. The test does not verify behavior when `buflen` is 0 or 1 in `rte_sysfs_parse_string`, but that is an edge case.

### Documentation

**lib/eal/include/rte_sysfs.h**
Doxygen is present for all four functions. The descriptions are clear. Minor improvements:
- `rte_sysfs_parse_uint`: Clarify that `val` is unmodified on failure (or document as undefined, depending on the intended contract).
- `rte_sysfs_vparse_uint`: Same as above.
- `rte_sysfs_parse_string`: Clarify that `buf` contents are "indeterminate on failure"--currently it says "indeterminate," which is correct but could be "undefined" for consistency.

---

## Final Summary

### Errors (must fix)

1. **lib/eal/unix/eal_unix_sysfs.c:55**: `*strchrnul(buf, '\n') = '\0'` may write beyond buffer if `buf` is empty. Check `buf[0] != '\0'` first.
2. **lib/power/power_common.c:93**: `power_sysfs_read_u32` does not check that `tmp` fits in `uint32_t`. Add range check or cast with overflow warning.

### Warnings (should fix)

1. **lib/eal/unix/eal_unix_sysfs.c:92**: Document that `val` is unmodified on failure, or zero it on error.
2. **drivers/bus/vmbus/linux/vmbus_uio.c:339**: Error return changed from `-errno` to `-EIO`. Acceptable but note the behavior change in commit message or comments.

### Info (consider)

1. **lib/eal/unix/eal_unix_sysfs.c:84**: The `errno != 0` check in `rte_sysfs_vparse_uint` is redundant. Consider removing or documenting.
2. **lib/eal/include/rte_sysfs.h:34**: Doxygen for `rte_sysfs_parse_uint` could rephrase the negative-value rejection explanation.
3. **app/test/test_eal_fs.c:47**: Test assumes `/proc/self/fd/` is available. Already skipped on FreeBSD; acceptable.