|WARNING| [v5,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/172128

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-26

# DPDK Patch Review

## ERRORS

### Patch 2/3 - Incorrect Regex Logic

**File:** `dts/api/cryptodev/types.py`

The regex patterns in `VerifyResults` are fundamentally broken. Each field's regex is `r"\s*(?:\d+\s+){N}(\d+)"` which matches:
- Optional whitespace
- Then N occurrences of (digit(s) followed by whitespace)
- Then captures one final digit sequence

This pattern cannot work for parsing a **single line** of space-separated integers because it requires the entire match (including all N prior number groups) to appear contiguously. When applied to multi-line text with `re.DOTALL`, the pattern will match across lines unpredictably, capturing wrong values.

**Example of the problem:**
For `lcore_id: int = field(metadata=TextParser.find_int(r"\s*(\d+)"))`, if the output contains:
```
lcore id  Buffer size  Burst size  Enqueued  Dequeued  Failed Enq  Failed Deq  Failed Ops
0         32           32          10000000  10000000  0           0           0
```

The regex `r"\s*(\d+)"` will match the **first** number it finds, which could be the column header line number or the first field in the data line - not necessarily `lcore_id`.

The patterns for `buffer_size` through `failed_ops` have the same issue: `r"\s*(?:\d+\s+){2}(\d+)"` tries to skip 2 numbers then capture the 3rd, but when applied to the entire output (with `re.DOTALL`), it will match wherever this pattern first occurs, not necessarily on the data line you care about.

**The fundamental error:** These regexes assume they're being applied to a single line containing exactly the data row, or that `re.DOTALL` makes them magically find the right line. Neither is true. `re.DOTALL` only makes `.` match newlines; it doesn't make `\s*(?:\d+\s+){N}` skip to the Nth field of a specific line.

**Required fix:**
1. Anchor the regex to the actual data line structure
2. Match the entire line context to ensure you're capturing from the data row, not headers or other output
3. Use named groups or explicit structure that identifies which number is which

Example correct pattern for a line like `0  32  32  10000000  10000000  0  0  0`:
```python
# Match the entire data line, capture all fields
lcore_id: int = field(metadata=TextParser.find_int(
    r"^\s*(\d+)\s+\d+\s+\d+\s+\d+\s+\d+\s+\d+\s+\d+\s+\d+", re.MULTILINE
))
buffer_size: int = field(metadata=TextParser.find_int(
    r"^\s*\d+\s+(\d+)\s+\d+\s+\d+\s+\d+\s+\d+\s+\d+\s+\d+", re.MULTILINE
))
# ... etc for each field
```

Or better: match the entire line once and extract all fields together, rather than running 8 separate regexes over the same text.

---

## WARNINGS

### Patch 1/3 - Missing Release Notes

**Issue:** The patch introduces a new directory `dts/test_resources/` and moves test data files from `app/test-crypto-perf/data/` to this new location. This is a behavioral change that affects where DTS looks for test data.

**Missing:** No update to release notes documenting this change. Users and CI systems that depend on the old path will break.

**Required:** Add an entry to the current release notes file in `doc/guides/rel_notes/` explaining:
- The new `dts/test_resources/` directory
- That test data files have been relocated
- Migration guidance (if any external tooling referenced the old path)

### Patch 2/3 - Missing Fixes Tag Context

**Issue:** The `Fixes:` tag references commit `8ee2df9da125` but provides no context about what was wrong in that commit or how this fix addresses it.

**Required:** Expand the commit message to explain:
- What the original implementation did wrong (e.g., "regexes matched wrong fields", "parsed header row instead of data")
- What symptom this caused (e.g., "all fields returned 0", "enqueued/dequeued counts were swapped")
- How the new regexes fix it

The Bugzilla ID is present, which is good, but the commit message should be self-contained.

### Patch 3/3 - Test Documentation Too Brief

**File:** `tests.TestSuite_cryptodev_verify.rst`

The RST file contains only a title and automodule directive with no introductory content. For a new test suite, this should include:
- Purpose of the test suite (functional verification vs performance)
- What it tests (cryptodev verify mode, specific algorithms)
- Prerequisites or configuration requirements
- Expected outcomes

**Suggested addition:**
```rst
cryptodev_verify Test Suite
===========================

Functional verification test suite for DPDK cryptodev PMDs using the
``dpdk-test-crypto`` application in verify mode.

This suite validates the correctness of cryptographic operations
(AES-CBC, AES-GCM, SHA1-HMAC, SHA256-HMAC) by comparing computed
outputs against known-good test vectors.

Tests cover both hardware crypto devices and virtual devices
(aesni_mb, openssl).

.. automodule:: tests.TestSuite_cryptodev_verify
   :members:
   :show-inheritance:
```

### Patch 3/3 - Hardcoded Magic Numbers

**File:** `dts/tests/TestSuite_cryptodev_verify.py`

```python
TOTAL_OPS = 10_000_000
```

The constant `TOTAL_OPS` is used in all tests but there's no comment explaining why this specific value. For functional verification (as opposed to throughput testing), this seems arbitrarily large. A comment explaining the rationale would help maintainability:

```python
# Total operations for verify tests - large enough to stress the PMD
# but low enough to complete quickly in CI environments
TOTAL_OPS = 10_000_000
```

Similarly, `buffer_sz=32`, `burst_sz=32`, and `digest_sz` values vary between tests with no explanation. Are these derived from the test data files? From algorithm requirements? Document this.

### Patch 3/3 - Inconsistent Error Messages

In `_verify_output()`:
```python
return False
```
When a test fails, the method returns `False` but the error message in the caller is generic:
```python
verify(self._verify_output(app.run_app(num_vfs=0)), "Failed to verify test sha1_hmac_buff_32")
```

If `failed_enqueued > 0`, the user has no idea which field was non-zero or what the values were. Consider:
```python
def _verify_output(self, results: list[CryptodevResults]) -> tuple[bool, str]:
    """Verify crypto operation results.
    
    Returns:
        Tuple of (success, error_message). error_message is empty string on success.
    """
    for result in results:
        if getattr(result, "failed_enqueued") > 0:
            return False, f"Enqueue failures: {result.failed_enqueued}"
        if getattr(result, "failed_dequeued") > 0:
            return False, f"Dequeue failures: {result.failed_dequeued}"
        if getattr(result, "failed_ops") > 0:
            return False, f"Operation failures: {result.failed_ops}"
    return True, ""

# Usage:
success, err = self._verify_output(app.run_app(num_vfs=0))
verify(success, f"sha1_hmac_buff_32: {err}")
```

---

## INFO

### Patch 1/3 - Data File Format Not Documented

The `.data` files (`test_aes_cbc.data`, `test_aes_gcm.data`) use a custom format (comments with `#`, key-value pairs, hex arrays). There's no documentation of this format in the patch or in a README in `dts/test_resources/`.

Consider adding a `dts/test_resources/README.md` explaining:
- Format syntax
- How to add new test vectors
- Relationship to test suite parameters

This is Info because the format appears to be inherited from the original `app/test-crypto-perf/data/` location, but documenting it would improve maintainability.

### Patch 3/3 - Potential for Parameterization

The three test methods (`aesni_mb_vdev`, `openssl_vdev`, `sha1_hmac_buff_32`) have significant code duplication. They could be parameterized:

```python
@crypto_test
@pytest.mark.parametrize("device,test_name,config", [
    (VirtualDevice("crypto_aesni_mb0"), "sha1_hmac_buff_32", {...}),
    (VirtualDevice("crypto_openssl0"), "aes_gcm_buff_32", {...}),
    (None, "sha1_hmac_buff_32", {...}),  # Uses device_type from suite setup
])
def test_verify(self, device, test_name, config) -> None:
    # Single test method, config drives the parameters
    pass
```

This is purely a maintenance suggestion and not required for correctness.

---

Review-Result: ERROR