|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

## Summary

This patch adds NIC information gathering and JSON output to the DTS (DPDK Test Suite) framework. It collects hardware details about network interfaces and writes them to a file for use in results reporting.

---

## Errors

### 1. Error path resource leak - file not closed on exception

**Location:** `dts/framework/test_run.py:354-355`

The file is opened but not guaranteed to be closed if `json.dump()` raises an exception (e.g., serialization error).

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

**Fix:** The `with` statement already ensures the file is closed. However, if `json.dump()` fails, the exception propagates without any cleanup logging or context. Consider wrapping in a try-except if you want to handle serialization failures gracefully:

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

---

### 2. Missing error check - `lshw_result.stdout` not validated before use

**Location:** `dts/framework/testbed_model/linux_session.py:224`

The code checks `lshw_result.stdout == ""` but does not verify that the command actually parsed the PCI address correctly. An empty stdout could mean the grep found nothing, but a non-empty stdout could still be garbage if the command syntax was wrong.

```python
if lshw_result.return_code != 0 or lshw_result.stdout == "":
    raise ConfigurationError(f"Unable to get bus type for port {pci_addr}.")
bus_type = lshw_result.stdout
```

**Issue:** If `lshw_result.stdout` contains unexpected output (e.g., multiple lines, error message), `bus_type` will have an invalid value leading to incorrect `bus_info` construction on line 227.

**Fix:** Add validation that `bus_type` contains expected format (e.g., `"pci"`, `"usb"`):

```python
if lshw_result.return_code != 0 or not lshw_result.stdout.strip():
    raise ConfigurationError(f"Unable to get bus type for port {pci_addr}.")
bus_type = lshw_result.stdout.strip()
if not bus_type:
    raise ConfigurationError(f"Empty bus type returned for port {pci_addr}.")
```

---

### 3. Potential KeyError - `port_to_data[bus_info]` not checked before access

**Location:** `dts/framework/testbed_model/linux_session.py:228`

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

**Issue:** If `bus_info` is not in `port_to_data`, this raises `KeyError`. The subsequent `if nic_port is None` check will never execute.

**Fix:** Use `.get()` to avoid KeyError:

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

---

### 4. Inconsistent error handling - `ethtool` failure logged but not raised

**Location:** `dts/framework/testbed_model/linux_session.py:244-249`

When `ethtool` fails to get NIC speed, an error is logged but the function continues. However, other failures (lshw bus type, missing config) raise `ConfigurationError`. This inconsistency means partial NIC info is returned with `"Unknown"` speed, which may not be the intended behavior.

**Decision needed:** Should missing speed be a fatal error or a warning?

If fatal (consistent with other errors):
```python
if ethtool_result.return_code == 0 and ethtool_result.stdout:
    nic_speed = ethtool_result.stdout
else:
    raise ConfigurationError(f"Unable to get speed for NIC: {pci_addr}")
```

If acceptable (current behavior is fine), change to Warning-level or keep as-is. But the current mix of error and continue is confusing.

---

### 5. Type annotation mismatch - `nic_port` cannot be None after assignment

**Location:** `dts/framework/testbed_model/linux_session.py:228`

```python
nic_port: LshwOutput | None = port_to_data[bus_info]
if nic_port is None:
    raise ConfigurationError(...)
```

**Issue:** The type hint allows `None`, but `port_to_data[bus_info]` will raise `KeyError` if the key is missing, not return `None`. This is dead code after fixing Error #3.

**Fix (after applying Error #3 fix):**

```python
nic_port = port_to_data.get(bus_info)
if nic_port is None:
    raise ConfigurationError(f"Port {pci_addr} could not be found on the node.")
# nic_port is now guaranteed to be LshwOutput, no Optional needed below
```

---

## Warnings

### 1. Missing documentation - `get_nic_info()` abstract method has no Raises clause

**Location:** `dts/framework/testbed_model/os_session.py:584-592`

The abstract method docstring documents the return type but the `Raises:` section only mentions `ConfigurationError` generically. The implementation in `LinuxSession` raises `ConfigurationError` for multiple specific reasons, which should be documented in the abstract method.

**Suggested addition:**

```python
def get_nic_info(self) -> list[dict[str, str]]:
    """Get NIC information.

    Returns:
        NIC info as a list of dictionaries, one per configured port.
        Each dictionary contains: make, model, hardware version,
        firmware version, deviceBusType, deviceId, pmd, speed.

    Raises:
        ConfigurationError: If NIC information cannot be retrieved
            (missing bus type, port not found, configuration unavailable,
            or required fields missing).
    """
```

---

### 2. Hardcoded JSON filename and logging message could be parameterized

**Location:** `dts/framework/test_run.py:354-357`

The filename `"dut_info.json"` is hardcoded. If the format or purpose changes, it will need to be updated in multiple places.

**Suggestion:** Define a constant at module level:

```python
DUT_INFO_FILENAME = "dut_info.json"

# Then use:
with open(f"{SETTINGS.output_dir}/{DUT_INFO_FILENAME}", "w") as file:
    json.dump(testrun_nic_info, file, indent=3)
self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/{DUT_INFO_FILENAME}")
```

---

### 3. Dictionary key strings inconsistent casing

**Location:** `dts/framework/testbed_model/linux_session.py:251-258`

The keys use inconsistent casing: `"make"`, `"model"` (lowercase) vs `"deviceBusType"`, `"deviceId"` (camelCase).

**Suggestion:** Use consistent naming convention (all snake_case or all camelCase). If this matches an external API schema, document that requirement. Otherwise, prefer snake_case for consistency with Python conventions:

```python
dut_json = {
    "make": ...,
    "model": ...,
    "hardware_version": ...,
    "firmware_version": ...,
    "device_bus_type": bus_type,
    "device_id": ...,
    "pmd": ...,
    "speed": nic_speed or "Unknown",
}
```

---

### 4. Shell command parsing is fragile

**Location:** `dts/framework/testbed_model/linux_session.py:223-224`

```python
f"sudo lshw -c network -businfo | grep '{pci_addr}' | cut -d'@' -f1"
```

**Issue:** This assumes the output format of `lshw` is stable and that `pci_addr` does not contain shell metacharacters. If `pci_addr` contains characters like `$`, `` ` ``, or `|`, the command could fail or behave unexpectedly.

**Suggestion:** Use proper escaping or parameterization. If `send_command` does not support parameterization, at least validate that `pci_addr` matches expected format (`[0-9a-f:.]+`):

```python
import re
if not re.match(r'^[0-9a-fA-F:.]+$', pci_addr):
    raise ConfigurationError(f"Invalid PCI address format: {pci_addr}")
```

---

### 5. Missing validation - port configuration fields may be empty strings

**Location:** `dts/framework/testbed_model/linux_session.py:251-258`

The code checks `if "driver" in config` but does not check if `config["driver"]` is an empty string. Similarly for other fields. An empty string is truthy in Python, so the check passes but the resulting JSON contains empty values.

**Suggestion:** Add validation for empty strings:

```python
dut_json = {
    "make": nic_port.get("vendor") or "Unknown",
    "model": nic_port.get("product") or "Unknown",
    "hardware version": nic_port.get("version") or "Unknown",
    "firmware version": config.get("firmware") or "Unknown",
    "deviceBusType": bus_type,
    "deviceId": nic_port.get("serial") or "Unknown",
    "pmd": config.get("driver") or "Unknown",
    "speed": nic_speed or "Unknown",
}
```

This also simplifies the logic by removing the `if "key" in dict else` pattern.

---

### 6. `self._lshw_net_info` accessed without checking it's populated

**Location:** `dts/framework/testbed_model/linux_session.py:214`

```python
port_to_data = {
    port.get("businfo"): port for port in self._lshw_net_info if port.get("businfo")
}
```

**Issue:** If `self._lshw_net_info` is not initialized or is empty (due to `lshw` failure earlier in the session), `port_to_data` will be an empty dict and all subsequent lookups will fail with `ConfigurationError("Port ... could not be found")`. This is not wrong per se, but the error message is misleading -- it implies the port doesn't exist when actually the lshw data was never collected.

**Suggestion:** Check that `self._lshw_net_info` is not empty at the start:

```python
if not self._lshw_net_info:
    raise ConfigurationError("NIC information is not available. lshw may have failed.")
```

---

## Info

### 1. Code readability - dictionary comprehension could be clearer

**Location:** `dts/framework/testbed_model/linux_session.py:214-216`

```python
port_to_data = {
    port.get("businfo"): port for port in self._lshw_net_info if port.get("businfo")
}
```

The conditional `if port.get("businfo")` filters out entries where `businfo` is missing or falsy. This is correct but could be more explicit. Consider extracting to a named variable or adding a comment.

```python
# Build lookup table of bus info to port data, skipping entries without businfo
port_to_data = {
    port["businfo"]: port 
    for port in self._lshw_net_info 
    if port.get("businfo")
}
```

---

### 2. Consider using `Path` for file operations

**Location:** `dts/framework/test_run.py:354`

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

**Suggestion:** Use `pathlib.Path` for cross-platform path handling:

```python
from pathlib import Path

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

---

### 3. JSON formatting - indent parameter

**Location:** `dts/framework/test_run.py:355`

`indent=3` is unusual; Python convention is `indent=4` (matching code indentation) or `indent=2` (compact).

**Suggestion:** Use `indent=4` or `indent=2` for consistency with common JSON formatting.

---

## Summary of Findings

**Errors:** 5 (resource leak risk, missing validation, KeyError risk, inconsistent error handling, type annotation mismatch)

**Warnings:** 6 (documentation, hardcoded strings, naming inconsistency, shell command fragility, empty string validation, missing data check)

**Info:** 3 (readability, path handling, JSON formatting)

The most critical issues are the potential `KeyError` in `port_to_data` access (Error #3) and the missing validation of command output (Error #2), both of which can cause the function to fail with confusing error messages or incorrect data.