|WARNING| [v6] dts: report dut/NIC info during DTS run

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch: dts: report dut/NIC info during DTS run

---

## Summary

This patch adds NIC information gathering to the DTS test framework, writing hardware details to a JSON file. The implementation is in the DTS test infrastructure (Python code under `dts/`), not core DPDK C code.

---

## Findings

### Errors

**1. Resource leak on command failure**

In `dts/framework/testbed_model/linux_session.py` line 220-224:

```python
lshw_result = self.send_command(
    f"sudo lshw -c network -businfo | grep '{pci_addr}' | cut -d'@' -f1"
)
if lshw_result.return_code != 0 or lshw_result.stdout == "":
    raise ConfigurationError(f"Unable to get bus type for port {pci_addr}.")
```

When this raises `ConfigurationError`, the file opened at line 354 remains open:

```python
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(testrun_nic_info, file, indent=3)
```

The `get_nic_info()` call at line 351-352 can raise before the `with` block completes, leaving the file handle leaked.

**Fix**: Move error-prone operations outside the `with` block, or wrap the entire sequence in proper exception handling:

```python
try:
    testrun_nic_info = self.test_run.ctx.sut_node.main_session.get_nic_info()
except ConfigurationError as ex:
    self.logger.error(f"Failed to gather NIC info: {ex}")
    # Decide: fail the test run or continue without NIC info
    # For now, re-raise to maintain current behavior
    raise

with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(testrun_nic_info, file, indent=3)
```

---

**2. Dictionary key access without membership check (line 231)**

```python
nic_port: LshwOutput | None = port_to_data[bus_info]
if nic_port is None:
    raise ConfigurationError(f"Port {pci_addr} could not be found on the node.")
```

Dictionary access `port_to_data[bus_info]` raises `KeyError` if `bus_info` is not a key. The code assumes the key exists but then checks if the *value* is None. This conflates two failure modes.

**Fix**: Use `.get()` with a default, or check membership explicitly:

```python
nic_port: LshwOutput | None = port_to_data.get(bus_info)
if nic_port is None:
    raise ConfigurationError(f"Port {pci_addr} with bus_info '{bus_info}' not found.")
```

---

**3. Unchecked dictionary access for `configuration` key (line 233)**

```python
config: LshwConfigurationOutput | None = nic_port["configuration"]
```

If `"configuration"` key is missing, this raises `KeyError`. The type hint says it can be `None`, but the dictionary access does not use `.get()`.

**Fix**:

```python
config: LshwConfigurationOutput | None = nic_port.get("configuration")
```

---

**4. JSON file written without fsync or error handling**

Lines 354-355:

```python
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(testrun_nic_info, file, indent=3)
```

If `json.dump()` raises (e.g., non-serializable data), the file is left incomplete and the error propagates without cleanup. While Python's `with` will close the file, the JSON may be truncated.

**Fix**: Catch JSON serialization errors and log a clear message:

```python
try:
    with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
        json.dump(testrun_nic_info, file, indent=3)
except (OSError, TypeError, ValueError) as ex:
    self.logger.error(f"Failed to write NIC info to JSON: {ex}")
    raise ConfigurationError(f"Could not write dut_info.json: {ex}")
```

---

### Warnings

**1. Implicit boolean test on string (line 221, 244)**

Lines 221 and 244 test `result.stdout == ""`. While explicit string comparison is acceptable, the code would be clearer using `.strip()` to handle whitespace-only output:

```python
if lshw_result.return_code != 0 or not lshw_result.stdout.strip():
    raise ConfigurationError(...)
```

Same for line 244:

```python
if ethtool_result.return_code == 0 and ethtool_result.stdout.strip():
```

---

**2. Inconsistent error handling (line 247)**

When `ethtool` fails, the code logs an error but continues with `nic_speed = None`, later converting it to `"Unknown"` (line 258). This is acceptable, but the mixed handling (raise on `lshw` failure, continue on `ethtool` failure) should be documented.

**Suggestion**: Add a comment explaining why `ethtool` failure is non-fatal:

```python
# Speed is optional metadata; log but don't fail the run
self._logger.error(f"Unable to get speed for NIC: {pci_addr}")
```

---

**3. Redundant None check in ternary (line 258)**

```python
"speed": nic_speed or "Unknown",
```

This is fine, but the explicit `nic_speed = None` on line 247 and the `or` here are redundant with the `if/else` above. Simplify:

```python
"speed": nic_speed if nic_speed else "Unknown",
```

Or just use `nic_speed or "Unknown"` throughout (current code is acceptable).

---

**4. No validation of SETTINGS.output_dir**

Before writing `dut_info.json`, the code assumes `SETTINGS.output_dir` is a valid writable directory. If it doesn't exist or lacks write permissions, the `open()` call will fail.

**Suggestion**: Check directory existence early in the test run setup, or catch and re-raise with a clearer message (covered in Error #4 fix above).

---

**5. Type annotation inconsistency**

Line 231:

```python
nic_port: LshwOutput | None = port_to_data[bus_info]
```

The type hint says `nic_port` can be `None`, but dictionary access cannot return `None` unless `None` is explicitly stored. Use `.get(bus_info)` (see Error #2) to match the annotation.

---

### Info

**1. Consider caching lshw output**

The code calls `send_command("sudo lshw ...")` once per port (line 220). If `_lshw_net_info` already contains this data, parsing it would be faster than shelling out repeatedly.

**Suggestion**: If performance matters, parse `bus_info` from `_lshw_net_info` instead of calling `lshw` again.

---

**2. Shell command with user-supplied PCI address**

Line 220 interpolates `pci_addr` directly into a shell command. While PCI addresses are constrained format (e.g., `0000:01:00.0`), consider validation if this could ever come from untrusted input.

**Current code is likely safe** (PCI addresses from config), but be aware of potential injection risk if input source changes.

---

**3. Python f-string formatting**

Lines 249-260 use a dictionary literal with f-strings for "Unknown" fallbacks. This is fine, but the `.get()` method with a default is more Pythonic:

```python
dut_json = {
    "make": nic_port.get("vendor", "Unknown"),
    "model": nic_port.get("product", "Unknown"),
    ...
}
```

---

## Final Recommendations

1. **Fix dictionary access patterns** (Errors #2, #3) to use `.get()` or explicit membership checks
2. **Add exception handling** around `get_nic_info()` call and JSON write (Errors #1, #4)
3. **Consider explicit string strip checks** for empty output (Warning #1)
4. **Document why ethtool failure is non-fatal** (Warning #2)

The patch adds useful functionality but needs robustness improvements in error paths and dictionary access.