|WARNING| [v1] dts: remove crypto test type

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/167315

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch removes the `crypto` test type from the DTS framework, consolidating crypto tests under the `performance` test category. The change allows crypto tests to run in any topology mode, including `NO_LINK`, which was previously not possible.

---

## Errors

### 1. Resource leak on error path (topology.py)
**File:** `dts/framework/testbed_model/topology.py`  
**Line:** 148-149

```python
if self.type is not LinkTopology.NO_LINK or get_ctx().sut_node.cryptodevs:
    self._prepare_devbind_script()
```

**Issue:** The `get_ctx()` call may access `sut_node.cryptodevs` when `sut_node` is not yet initialized or in an error state. If `get_ctx()` returns a context where `sut_node` is `None`, accessing `.cryptodevs` will raise an `AttributeError` that is not handled.

**Suggested fix:** Add a null check:
```python
ctx = get_ctx()
if self.type is not LinkTopology.NO_LINK or (ctx.sut_node and ctx.sut_node.cryptodevs):
    self._prepare_devbind_script()
```

---

## Warnings

### 1. Inconsistent conditional logic ordering (topology.py)
**File:** `dts/framework/testbed_model/topology.py`  
**Lines:** 148-152

```python
if self.type is not LinkTopology.NO_LINK or get_ctx().sut_node.cryptodevs:
    self._prepare_devbind_script()

if self.type is LinkTopology.NO_LINK:
    return

self._setup_ports("sut")
```

**Issue:** The first condition prepares the devbind script when `type is not NO_LINK OR cryptodevs exist`. The second condition returns early if `type is NO_LINK`. This means when `type is NO_LINK AND cryptodevs exist`, the devbind script is prepared but then the function returns immediately without using it. This appears wasteful and may indicate incomplete logic.

**Suggested fix:** Consider whether the devbind script preparation should be conditional on the subsequent port setup actually happening:
```python
if self.type is LinkTopology.NO_LINK:
    if get_ctx().sut_node.cryptodevs:
        self._prepare_devbind_script()
    return

self._prepare_devbind_script()
self._setup_ports("sut")
self._setup_ports("tg")
```

Or verify that preparing the devbind script without setting up ports is intentional for the crypto-only case.

---

### 2. Empty JSON file written without documentation (test_run.py)
**File:** `dts/framework/test_run.py`  
**Lines:** 363-369

```python
# Only collect NIC info if network ports exist.
testrun_nic_info: list[dict[str, str]] = []
if ctx.topology.sut_ports:
    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)
self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/dut_info.json")
```

**Issue:** When no SUT ports exist, an empty JSON array `[]` is written to `dut_info.json`. The comment says "an empty json file is written as a placeholder" but this behavior is not documented for users who may consume this file. The log message does not indicate that the file is empty/placeholder.

**Suggested fix:** Either document this behavior in user-facing documentation, or adjust the log message to indicate when the file is a placeholder:
```python
if ctx.topology.sut_ports:
    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)
    self.logger.info(f"DUT NIC info written to: {SETTINGS.output_dir}/dut_info.json")
else:
    with open(f"{SETTINGS.output_dir}/dut_info.json", "w") as file:
        json.dump([], file, indent=3)
    self.logger.info(f"No NIC info available; empty placeholder written to: {SETTINGS.output_dir}/dut_info.json")
```

---

### 3. Condition change from `crypto` to `perf` may alter behavior (test_run.py)
**File:** `dts/framework/test_run.py`  
**Line:** 373

```python
if ctx.sut_node.cryptodevs and test_run.config.perf:
```

**Original:**
```python
if ctx.sut_node.cryptodevs and test_run.config.crypto:
```

**Issue:** This changes the condition for instantiating crypto ports from requiring the `crypto` flag to requiring the `perf` flag. If a user previously ran with `--perf` disabled but `--crypto` enabled, their crypto tests would have run. Now, crypto tests only run when `--perf` is enabled. This is a behavioral change that should be documented in release notes or migration documentation.

**Suggested fix:** Ensure the commit message or release notes clearly document that crypto tests now require the `--perf` flag and that the `--crypto` flag has been removed.

---

## Info

### 1. Import placement (topology.py)
**File:** `dts/framework/testbed_model/topology.py`  
**Line:** 146

```python
from framework.context import get_ctx
```

**Observation:** The import is placed inside the `setup()` method rather than at the module level. While this may be intentional to avoid circular imports, it's worth noting that this pattern can make dependencies less obvious.

**Consideration:** If this is to avoid circular imports, a comment explaining this would help future maintainers. If not, consider moving it to module-level imports.

---

## Summary of Findings

- **1 Error** (potential AttributeError from null sut_node access)
- **3 Warnings** (logic inconsistency, undocumented empty JSON behavior, behavior change not documented)
- **1 Info** (non-standard import placement)

The patch is mostly sound, but the error regarding null-safe access to `sut_node.cryptodevs` should be addressed before merging. The warnings are primarily about clarity and documentation rather than correctness bugs.