|WARNING| [v5] 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/166260

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds NIC information gathering during DTS (DPDK Test Suite) runs and writes the data to a JSON file. The implementation is for the Linux OS session.

---

## Errors

### 1. Resource leak on error path in `get_nic_info()`

**File:** `dts/framework/test_run.py`

The file opened at line 354 is not closed if `json.dump()` raises an exception (e.g., if `testrun_nic_info` contains non-serializable data).

```python
# BAD - file not closed on json.dump() error
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(testrun_nic_info, file, indent=3)
```

**Fix:** The code already uses a context manager (`with` statement) which ensures the file is closed even on exception. This is actually correct. However, if `get_nic_info()` raises an exception, the file will be created but empty. Consider wrapping the entire block:

```python
# Get NIC info first, then write atomically
testrun_nic_info = self.test_run.ctx.sut_node.main_session.get_nic_info()
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(testrun_nic_info, file, indent=3)
```

Actually, reviewing more carefully: the current code already gets the NIC info before opening the file, so this is correct. **Retracted - no issue.**

### 2. Missing error check on `send_command()` return value

**File:** `dts/framework/testbed_model/linux_session.py`, line 228

The code checks `lshw_result.return_code != 0 and lshw_result.stdout == ""` but the logic is incorrect. If the command fails, you should raise an error. The `and` should likely be `or`:

```python
# BAD - only raises if BOTH conditions are true
if lshw_result.return_code != 0 and lshw_result.stdout == "":
    raise ConfigurationError(f"Unable to get bus type for port {pci_addr}.")
```

**Analysis:** If `return_code != 0` but `stdout` contains data, the error is silently ignored. If `return_code == 0` but `stdout == ""`, the error is also ignored (though this would assign an empty string to `bus_type`).

**Fix:**
```python
# GOOD - raise on any failure condition
if lshw_result.return_code != 0 or lshw_result.stdout == "":
    raise ConfigurationError(f"Unable to get bus type for port {pci_addr}.")
```

### 3. Potential `None` dereference in `get_nic_info()`

**File:** `dts/framework/testbed_model/linux_session.py`, line 231

The code declares `nic_port` as `LshwOutput | None` but then unconditionally accesses `nic_port["configuration"]` on line 234. If `port_data[bus_info]` returns `None` (key not found in dict), this will raise `TypeError: 'NoneType' object is not subscriptable`.

```python
# BAD - nic_port could be None from failed dict lookup
nic_port: LshwOutput | None = port_data[bus_info]
if nic_port is None:
    raise ConfigurationError(f"Port {pci_addr} could not be found on the node.")

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

**Analysis:** Actually, Python dict access raises `KeyError` if the key doesn't exist, not returns `None`. The type annotation `LshwOutput | None` is misleading. The code should use `.get()` to return `None` on missing key:

```python
# GOOD - use .get() to handle missing key
nic_port: LshwOutput | None = port_data.get(bus_info)
if nic_port is None:
    raise ConfigurationError(f"Port {pci_addr} could not be found on the node.")
```

---

## Warnings

### 1. Missing docstring parameter documentation

**File:** `dts/framework/testbed_model/linux_session.py`, line 208

The `get_nic_info()` method docstring does not document the return value structure. Each dictionary key should be documented.

**Suggested addition:**
```python
"""Get NIC information for all configured ports.

Returns:
    A list of dictionaries, one per port, containing:
        - make: NIC vendor name
        - model: NIC product name
        - hardware version: Hardware revision
        - firmware version: Firmware version string
        - deviceBusType: Bus type (e.g., 'pci')
        - deviceId: Device serial number (MAC address)
        - pmd: Driver name
        - speed: Link speed (e.g., '10000Mb/s')

Raises:
    ConfigurationError: If NIC information could not be retrieved.
"""
```

### 2. Inconsistent error handling for missing optional fields

**File:** `dts/framework/testbed_model/linux_session.py`, lines 254-261

The code uses different patterns for handling missing optional fields:
- For `speed`: logs error and sets to `None`, then converts to "Unknown" 
- For `vendor`, `product`, etc.: uses inline conditional with "Unknown"

**Consistency suggestion:**
```python
# Current pattern is acceptable but verbose for speed
if ethtool_result.return_code == 0 and ethtool_result.stdout:
    nic_speed = ethtool_result.stdout
else:
    self._logger.error(f"Unable to get speed for NIC: {pci_addr}")
    nic_speed = None

# Later: nic_speed or "Unknown"

# Could be simplified to match other fields:
nic_speed = (
    ethtool_result.stdout 
    if ethtool_result.return_code == 0 and ethtool_result.stdout
    else "Unknown"
)
if nic_speed == "Unknown":
    self._logger.error(f"Unable to get speed for NIC: {pci_addr}")
```

### 3. Variable `port_data` could be more descriptive

**File:** `dts/framework/testbed_model/linux_session.py`, line 214

The variable name `port_data` doesn't clearly indicate it's a mapping from bus info to port data.

**Suggested rename:**
```python
# More descriptive
businfo_to_port = {
    port.get("businfo"): port for port in self._lshw_net_info if port.get("businfo")
}
```

### 4. Shell command parsing fragility

**File:** `dts/framework/testbed_model/linux_session.py`, line 222

The command `grep '{pci_addr}' | cut -d'@' -f1` assumes a specific lshw output format. If the format changes or the PCI address appears elsewhere in the line, this could extract incorrect data.

**Note:** This is acceptable for a test framework but be aware of the fragility. Consider adding a comment explaining the expected format.

---

## Info

### 1. JSON file location

The JSON file is written to `{SETTINGS.output_dir}/dut_info.json`. Consider whether this should be namespaced per test run (e.g., including a timestamp) if multiple runs could overlap.

### 2. Dictionary key naming convention

The `dut_json` dictionary uses mixed naming conventions:
- `"hardware version"` (space-separated)
- `"firmware version"` (space-separated)  
- `"deviceBusType"` (camelCase)
- `"deviceId"` (camelCase)
- `"pmd"` (lowercase)
- `"speed"` (lowercase)

Consider standardizing to one convention (preferably snake_case for consistency with Python conventions, or all lowercase with underscores).

---

## Summary of Required Fixes

**Must fix:**
1. Change `and` to `or` in the lshw command error check (line 228)
2. Use `.get()` instead of direct dict access for `port_data[bus_info]` (line 231)

**Should fix:**
1. Add detailed docstring for `get_nic_info()` return value structure
2. Consider standardizing dictionary key naming convention