|WARNING| [v3,3/3] dts: add verify coverage for cryptodev testing

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/3: dts: add directory for test resources

### Errors
None identified.

### Warnings
None identified.

### Info

**Documentation:**
The commit message says these files are "relocated from old DTS to new DTS," but there is no documentation update explaining what these test vector files are, their format, or how they should be used. Consider adding a README or inline comments in the data files explaining the format.

**File Format:**
The `.data` files use a custom format with hex byte arrays. While this appears to be a known format from the old DTS, it would be helpful to document the schema (what `plaintext`, `ciphertext`, `cipher_key`, etc. represent and their constraints).

---

## Patch 2/3: dts: fix cryptodev verify parsing

### Errors

**Regex pattern readability and correctness (Error):**
The new regex patterns in `dts/api/cryptodev/types.py` are cryptic and brittle. Each pattern is of the form `r"\s*(?:\d+\s+){N}(\d+)"` which matches:
- Optional leading whitespace
- N occurrences of a digit group followed by whitespace (non-capturing)
- A final digit group (captured)

This relies on the output format having exactly N whitespace-separated numeric columns before the desired value. If the output format changes (extra columns, different whitespace, or reordering), these regexes will silently capture the wrong column or fail to match.

**Why this matters:** Parsing numeric columns by position without anchoring to column headers or field names is fragile. The comment in `__init__.py` (`parser_options |= re.DOTALL`) suggests the output spans multiple lines, but the new patterns do not account for newlines explicitly. If the output has a header row or variable whitespace, these patterns may break.

**Suggested fix:** If the verify output has a tabular format with headers, parse the header line to determine column positions, then extract values by column. If headers are not present, at minimum add a comment showing an example of the output format these patterns are designed to match, and add a runtime validation that the number of fields matches expectations.

Example of a more robust approach (if headers exist):
```python
# Example output line:
# "lcore_id buffer_size burst_size enqueued dequeued failed_enq failed_deq failed_ops"
# "1       32          32         10000000 10000000 0          0          0"
# Parse by splitting on whitespace and accessing by column index.
fields = line.split()
if len(fields) < 8:
    raise ValueError(f"Expected at least 8 fields, got {len(fields)}")
lcore_id = int(fields[0])
buffer_size = int(fields[1])
# ...
```

Alternatively, if the format is truly fixed and stable, add a comment block showing the exact expected format:
```python
# Expected verify output format (one line per result):
# <lcore_id> <buffer_size> <burst_size> <enqueued> <dequeued> <failed_enq> <failed_deq> <failed_ops>
# Example: "1 32 32 10000000 10000000 0 0 0"
```

---

**`re.DOTALL` usage (Warning):**
In `dts/api/cryptodev/__init__.py`, line 135, `parser_options |= re.DOTALL` is set for the verify test type. `re.DOTALL` makes `.` match newlines, but none of the new regex patterns in `types.py` use `.`. This flag appears to be unnecessary for the current patterns and suggests either (a) the patterns were intended to span multiple lines but do not, or (b) this is leftover from an earlier implementation.

**Suggested fix:** Remove `parser_options |= re.DOTALL` if it is not needed, or document why it is required (e.g., if the `regex` in line 137 uses `.` to match multi-line output).

---

**Missing validation of parsed values (Warning):**
The `VerifyResults` fields (`enqueued`, `dequeued`, `failed_enqueued`, etc.) are parsed as integers with no bounds checking. If the regex accidentally captures a non-numeric field or a value from the wrong column, `TextParser.find_int()` may raise an exception or silently parse incorrect data.

**Suggested fix:** After parsing, validate that critical fields are non-negative and that relationships hold (e.g., `dequeued <= enqueued`). Example:
```python
if result.dequeued > result.enqueued:
    raise ValueError(f"Dequeued ({result.dequeued}) exceeds enqueued ({result.enqueued})")
```

---

### Info

**Commit message:**
The commit message says "did not properly gather the correct values" but does not explain what was wrong with the old patterns or why the new ones are correct. Consider adding an example of the actual verify output format to the commit message for future reference.

---

## Patch 3/3: dts: add verify coverage for cryptodev testing

### Errors

**Resource leak: VirtualDevice lifecycle (Error):**
In `aesni_mb_vdev()` and `openssl_vdev()`, `VirtualDevice` instances are created and passed to `Cryptodev`. There is no explicit cleanup or removal of these virtual devices. If `Cryptodev.run_app()` or the test fails before completion, the virtual device may remain allocated.

**Why this matters:** Virtual devices consume system resources. If tests are run repeatedly or in parallel, leaked virtual devices can cause resource exhaustion or conflicts.

**Suggested fix:** Ensure `VirtualDevice` has a cleanup mechanism (e.g., `__del__`, context manager, or explicit `remove()` method) and that it is called even on test failure. If `VirtualDevice` is already cleaned up by the test framework or `Cryptodev`, document that assumption. Example:
```python
vdev = VirtualDevice("crypto_aesni_mb0")
try:
    app = Cryptodev(vdevs=[vdev], ...)
    results = app.run_app(num_vfs=0)
    verify(self._verify_output(results), "Failed to verify...")
finally:
    vdev.cleanup()  # if such a method exists
```

---

**Error propagation: `_verify_output` returns `bool` but errors are not detailed (Warning):**
The `_verify_output()` method returns `False` if any of `failed_enqueued`, `failed_dequeued`, or `failed_ops` is greater than zero, but does not log or return which specific result failed or what the values were. When a test fails, the message "Failed to verify test X" does not provide enough information to diagnose the issue.

**Suggested fix:** Log the failing result or include it in the error message. Example:
```python
for i, result in enumerate(results):
    if (result.failed_enqueued > 0 or
        result.failed_dequeued > 0 or
        result.failed_ops > 0):
        self._logger.error(
            f"Result {i} failed: failed_enqueued={result.failed_enqueued}, "
            f"failed_dequeued={result.failed_dequeued}, failed_ops={result.failed_ops}"
        )
        return False
return True
```

---

**Hardcoded constant `TOTAL_OPS = 10_000_000` (Info):**
The `TOTAL_OPS` constant is set to 10 million operations. This may be appropriate for functional verification, but it is not clear if this value was chosen for a specific reason (performance, coverage, timeout constraints). If this is a tunable parameter, consider making it configurable or documenting why 10M is the target.

---

**Test naming: `sha1_hmac_buff_32` test method is generic (Info):**
The test method `sha1_hmac_buff_32()` has the same name as one of the test vector configurations in the data file. The method name should describe what is being tested (e.g., `test_physical_device_aes_cbc_sha1_hmac` or `test_configured_device_verify`). The current name is ambiguous.

**Suggested rename:**
```python
@crypto_test
def test_physical_device_aes_cbc_sha1(self) -> None:
    """Test AES-CBC with SHA1-HMAC on the configured physical crypto device."""
    # ...
```

---

**Documentation file location (Warning):**
The documentation file `tests.TestSuite_cryptodev_verify.rst` is placed at the repository root. DPDK convention is to place test suite documentation under `doc/guides/tools/` or alongside the test code. Verify the correct location per DTS documentation guidelines.

**Suggested fix:** Move `tests.TestSuite_cryptodev_verify.rst` to the appropriate documentation directory (e.g., `doc/guides/tools/dts/test_suites/` or similar).

---

### Info

**Test data files are not validated:**
The test suite assumes `test_aes_cbc.data` and `test_aes_gcm.data` exist in `dts/test_resources/` and have the expected format. If these files are missing, corrupted, or have the wrong schema, the tests will fail with unclear errors.

**Suggested improvement:** Add a `set_up_suite()` check that verifies the required test data files exist and are readable:
```python
def set_up_suite(self) -> None:
    self.device_type = get_device_from_str(
        str(get_ctx().sut_node.crypto_device_type)
    )
    # Verify test data files exist
    resources_path = get_ctx().dpdk_build.remote_dpdk_tree_path.joinpath("dts/test_resources/")
    for filename in [AES_CBC_DATA, AES_GCM_DATA]:
        filepath = resources_path.joinpath(filename)
        if not filepath.exists():
            raise FileNotFoundError(f"Required test data file not found: {filepath}")
```

---

## Summary

**Critical issues:**
1. **Patch 2:** Regex patterns are brittle and rely on positional column matching with no validation. Risk of silent failure if output format changes.
2. **Patch 3:** Potential resource leak for `VirtualDevice` instances if test fails before cleanup.

**Recommendations:**
- Add example output format for verify results to documentation/comments.
- Validate parsed numeric fields and their relationships after parsing.
- Implement explicit cleanup for virtual devices or document existing cleanup mechanism.
- Improve error messages to include failing values for easier debugging.
- Verify documentation file placement per DPDK conventions.