|WARNING| [v2] 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/166052

_AI Code Review_

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

# DPDK Patch Review

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

---

## ERRORS

### 1. Resource Leak - File Handle Not Closed on Exception

**Location:** `dts/framework/test_run.py`, lines 378-380

**Issue:** The file is opened but `file.close()` will not execute if `json.dump()` raises an exception, leaking the file descriptor.

**Fix:** Use context manager properly - the `with` statement already handles closing, so remove the explicit `close()` call:

```python
with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
    json.dump(used_nic_info, file, indent=3)
# file is automatically closed here, even on exception
self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/dut_info.json")
```

---

## WARNINGS

### 1. Missing Error Handling for File Write

**Location:** `dts/framework/test_run.py`, lines 378-380

**Issue:** File write operations can fail (disk full, permission denied, directory doesn't exist). No error handling is present.

**Suggested Fix:**

```python
try:
    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")
except OSError as e:
    self.logger.error(f"Failed to write DUT info: {e}")
```

### 2. Inconsistent Error Handling - Logging vs Exception

**Location:** `dts/framework/testbed_model/linux_session.py`, lines 244-248, 256-262

**Issue:** The code logs errors for missing hardware version and speed using `self._logger.error()` but continues execution, while missing bus type or port info raises `ConfigurationError`. This inconsistency makes error handling unpredictable.

**Rationale:** Either these are required fields (raise exception) or they are optional (use warning-level logging). Logging an error but continuing suggests the data is corrupted but the function claims success.

**Suggested Fix:** Use warning-level logging for optional fields:

```python
if hardware_version is None:
    self._logger.warning(f"Unable to get hardware version for NIC: {pci_addr}")

if nic_speed is None:
    self._logger.warning(f"Unable to get speed for NIC: {pci_addr}")
```

### 3. Variable Name Shadowing Loop Variable

**Location:** `dts/framework/testbed_model/linux_session.py`, line 211

**Issue:** The outer loop variable `port` (line 211) is shadowed by the dictionary comprehension variable `port` (line 202). While this is valid Python, it reduces readability.

**Suggested Fix:**

```python
for dut_port in self._config.ports:
    pci_addr = dut_port.pci
    # ... rest of loop
```

---

## INFO

### 1. Missing Release Notes Entry

**Issue:** This patch adds a new user-facing feature (writing `dut_info.json` during test execution) but does not update release notes.

**Suggested Action:** Add an entry to the appropriate release notes file documenting this new feature and the location of the generated file.

### 2. Type Annotation Could Be More Specific

**Location:** `dts/framework/testbed_model/os_session.py`, line 585

**Issue:** Return type `list[dict[str, object]]` uses `object` which is too generic. A TypedDict would provide better type safety.

**Suggested Fix:**

```python
from typing import TypedDict

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

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

### 3. Consider Path Validation

**Location:** `dts/framework/test_run.py`, line 378

**Issue:** The code assumes `SETTINGS.output_dir` exists and is writable. Consider validating or creating the directory.

**Suggested Fix:**

```python
from pathlib import Path

output_path = Path(SETTINGS.output_dir)
output_path.mkdir(parents=True, exist_ok=True)
with open(output_path / "dut_info.json", "w") as file:
    json.dump(used_nic_info, file, indent=3)
```

---

## SUMMARY

**Critical Issues:** 1 Error (file handle leak)  
**Important Issues:** 3 Warnings (missing error handling, inconsistent error logging, variable shadowing)  
**Suggestions:** 3 Info items (release notes, type annotations, path validation)

The most important fix is removing the explicit `file.close()` call in the `with` block, which is both redundant and prevents proper exception handling.