|WARNING| [v4] 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/166184

_AI Code Review_

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

# DPDK Patch Review

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

---

## Errors

### 1. Resource leak - file not closed on exception

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

The file handle opened with `open()` is not guaranteed to be closed if an exception occurs during `json.dump()`.

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

**Fix:** The `with` statement already ensures the file is closed on exception. However, if `get_nic_info()` raises before the `with` block, the flow is correct. The current code is actually safe - the `with` statement handles cleanup. **Retract this item.**

---

## Warnings

### 1. Missing error handling for `get_nic_info()` call

**File:** `dts/framework/test_run.py` (line ~351)

The call to `get_nic_info()` can raise `ConfigurationError` (per the docstring in `os_session.py`), but there is no try/except block to handle this gracefully in the test run setup phase.

**Suggested fix:**
```python
try:
    used_nic_info: list[dict[str, str]] = 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(used_nic_info, file, indent=3)
    self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/dut_info.json")
except ConfigurationError as e:
    self.logger.warning(f"Failed to collect DUT NIC info: {e}")
```

This prevents test run abort if NIC info gathering fails.

### 2. Variable assigned then immediately overwritten

**File:** `dts/framework/testbed_model/linux_session.py` (line ~262)

```python
nic_speed = command_result.stdout
# ...
else:
    self._logger.error(f"Unable to get speed for NIC: {pci_addr}")
    nic_speed = None  # Overwritten to None, then dict assigns "Unknown"
```

The `nic_speed = None` assignment is immediately consumed by `nic_speed or "Unknown"` in the dict literal. The intermediate `None` assignment adds no value; the error path could directly set the dict value.

**Suggested refactor:**
```python
if command_result.return_code == 0 and command_result.stdout:
    nic_speed = command_result.stdout
else:
    self._logger.error(f"Unable to get speed for NIC: {pci_addr}")
    nic_speed = "Unknown"
```

Or eliminate the variable entirely and use a ternary in the dict.

### 3. Redundant None check after dictionary lookup

**File:** `dts/framework/testbed_model/linux_session.py` (line ~231)

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

Dictionary access with `port_data[bus_info]` will raise `KeyError` if the key is not found, NOT return `None`. The `| None` type annotation and the `is None` check are incorrect for this usage.

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

Or use exception handling:
```python
try:
    nic_port: LshwOutput = port_data[bus_info]
except KeyError:
    raise ConfigurationError(f"Port {pci_addr} could not be found on the node.")
```

### 4. Inconsistent use of `NotRequired` for `LshwConfigurationOutput` fields

**File:** `dts/framework/testbed_model/linux_session.py` (line ~41)

The `LshwConfigurationOutput` class adds a `firmware` field but does not mark it `NotRequired`, yet the code accessing it checks `if "firmware" in config` (line ~258), indicating it may be absent.

Either mark the field `NotRequired[str]` or remove the runtime check if the field is guaranteed present.

**Suggested fix:**
```python
class LshwConfigurationOutput(TypedDict):
    driver: str
    link: str
    firmware: NotRequired[str]  # Mark as optional
```

### 5. Command injection risk in shell command construction

**File:** `dts/framework/testbed_model/linux_session.py` (line ~215, 247)

The `pci_addr` and `nic_port['logicalname']` are interpolated directly into shell commands without sanitization. If these contain shell metacharacters (unlikely but not impossible in misconfigured systems), command injection could occur.

**Suggested fix:** Use parameterized command construction or validate the PCI address format:
```python
# Validate PCI address format
import re
if not re.match(r'^[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}\.[0-9]$', pci_addr):
    raise ConfigurationError(f"Invalid PCI address format: {pci_addr}")
```

DPDK PCI addresses follow a strict format; validating it prevents injection.

---

## Info

### 1. Type annotation could be more precise

**File:** `dts/framework/test_run.py` (line ~351)

The variable `used_nic_info` is annotated as `list[dict[str, str]]`, which matches the return type of `get_nic_info()`. This is correct and no change is needed.

### 2. Consider adding a summary log message

**File:** `dts/framework/test_run.py` (line ~354)

After writing the NIC info, consider logging a summary of how many NICs were detected:
```python
self.logger.info(f"Collected info for {len(used_nic_info)} NIC(s), written to: {SETTINGS.output_dir}/dut_info.json")
```

This aids debugging when NIC count mismatches expectations.

---

## Summary

**Errors:** 0  
**Warnings:** 5  
**Info:** 2

The patch has no critical correctness bugs but has several warnings related to error handling, unnecessary variable assignments, and incorrect dictionary access patterns. The most important issue is the missing exception handling for `get_nic_info()` which could abort test runs unnecessarily.