|WARNING| [v3] 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/166053

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds NIC information gathering and JSON export functionality to the DTS framework. The changes are primarily additions to the test infrastructure without modifying core DPDK libraries.

---

## Errors

### 1. Resource leak - file not closed on exception path
**File:** `dts/framework/test_run.py`, lines 378-380

The file handle is not closed if `json.dump()` raises an exception (e.g., serialization error).

**Why it matters:** File descriptor leak if the serialization fails.

**Fix:** Use context manager properly or add exception handling:
```python
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(used_nic_info, file, indent=3)
# Remove the redundant file.close() - context manager handles it
self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/dut_info.json")
```

---

## Warnings

### 1. Explicit comparison missing for command result check
**File:** `dts/framework/testbed_model/linux_session.py`, lines 218-219, 238-239

Direct truthiness test on `command_result.stdout` violates DPDK style requiring explicit comparison.

**Fix:**
```python
# Line 218-219
bus_type = (
    command_result.stdout
    if command_result.return_code == 0 and command_result.stdout != ""
    else None
)

# Line 238-239
nic_speed = (
    command_result.stdout
    if command_result.return_code == 0 and command_result.stdout != ""
    else None
)
```

### 2. Missing null check before dictionary access
**File:** `dts/framework/testbed_model/linux_session.py`, line 241

`nic_name` is checked for `None` at line 241, but if it IS `None`, the error logging at line 247 uses `pci_addr` which is assumed to exist but hasn't been validated.

**Why it matters:** If `port.pci` is somehow invalid, the error message construction could fail.

**Fix:** This is minor since `port.pci` comes from config, but for defensive programming:
```python
if nic_speed is None:
    self._logger.error(f"Unable to get speed for NIC: {pci_addr if pci_addr else 'unknown'}")
```

### 3. Error handling inconsistency
**File:** `dts/framework/testbed_model/linux_session.py`, lines 247-251

When `nic_speed` cannot be retrieved, the code logs an error but continues, storing "Unknown" in the JSON. This is inconsistent with earlier error handling (lines 222-227, 229-232, 234-237) which raises `ConfigurationError` for missing data.

**Consider:** Either raise `ConfigurationError` for missing speed (if it's critical), or handle all missing data gracefully with logging. The current mix may indicate speed is optional - if so, document this in a comment.

---

## Info

### 1. Redundant file.close() call
**File:** `dts/framework/test_run.py`, line 379

The `with` statement automatically closes the file when the block exits. The explicit `file.close()` is unnecessary.

**Suggested fix:**
```python
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(used_nic_info, file, indent=3)
self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/dut_info.json")
```

### 2. Consider adding type hints
**File:** `dts/framework/testbed_model/linux_session.py`, line 200

The return type annotation `list[dict[str, object]]` could be more specific. Consider defining a TypedDict for the NIC info structure to improve type safety:

```python
from typing import TypedDict

class NicInfo(TypedDict):
    make: str | None
    model: str | None
    hardware_version: str
    firmware_version: str | None
    deviceBusType: str | None
    deviceId: str | None
    pmd: str | None
    speed: str

def get_nic_info(self) -> list[NicInfo]:
```

### 3. Dictionary key naming inconsistency
**File:** `dts/framework/testbed_model/linux_session.py`, lines 250-257

The dictionary keys mix styles: `"hardware version"` (space-separated) vs `"deviceBusType"` (camelCase) vs `"firmware_version"` (snake_case if it were used consistently). Consider standardizing to one convention.

**Suggested:** Use snake_case consistently:
```python
dut_json = {
    "make": nic_port.get("vendor"),
    "model": nic_port.get("product"),
    "hardware_version": nic_port.get("version") or "Unknown",
    "firmware_version": config.get("firmware"),
    "device_bus_type": bus_type,
    "device_id": nic_port.get("serial"),
    "pmd": config.get("driver"),
    "speed": nic_speed or "Unknown",
}
```

---

## Observations

1. **No tests added:** This patch adds new functionality (`get_nic_info()`) but does not include unit tests. Consider adding test coverage for the JSON output format and error paths.

2. **No documentation update:** The new `dut_info.json` output file is not documented. Consider updating the DTS documentation to describe this output file, its format, and its purpose.

3. **Hardcoded file name:** The output file name `dut_info.json` is hardcoded. Consider making this configurable or documenting it as a fixed output artifact.