|SUCCESS| [v1,3/3] dts: add verify coverage for cryptodev testing

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163999

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-14

git sha: 20260514172553.191331-1-abailey at iol.unh.edu (patch 1/3)
Author: Andrew Bailey <abailey at iol.unh.edu>
Subject: [PATCH v1 1/3] dts: add directory for test resources

This commit creates a test_resources directory and adds two cryptographic
test vector files relocated from old DTS to support the crypto verify test
suite.

> diff --git a/dts/api/cryptodev/__init__.py b/dts/api/cryptodev/__init__.py
> index a4fafc3713..4e8ce47de1 100644
> --- a/dts/api/cryptodev/__init__.py
> +++ b/dts/api/cryptodev/__init__.py
> @@ -76,7 +76,7 @@ def vector_directory(self) -> PurePath:
>          Returns:
>              The path to the cryptodev vector files.
>          """
> -        return get_ctx().dpdk_build.remote_dpdk_tree_path.joinpath("app/test-crypto-perf/data/")
> +        return get_ctx().dpdk_build.remote_dpdk_tree_path.joinpath("dts/test_resources/")

Does this code properly handle the case where the test_resources directory
does not yet exist on the remote system? Does the cryptodev application
gracefully handle missing vector files, or could this result in misleading
failures when the directory path is invalid?

[ ... ]


git sha: 20260514172553.191331-2-abailey at iol.unh.edu (patch 2/3)
Author: Andrew Bailey <abailey at iol.unh.edu>
Subject: [PATCH v1 2/3] dts: fix cryptodev verify parsing

This commit fixes incorrect regex patterns used to parse dpdk-test-crypto
verify output.

> diff --git a/dts/api/cryptodev/__init__.py b/dts/api/cryptodev/__init__.py
> index 4e8ce47de1..90b847d1fb 100644
> --- a/dts/api/cryptodev/__init__.py
> +++ b/dts/api/cryptodev/__init__.py
> @@ -132,6 +132,7 @@ def run_app(self, num_vfs: int = 1) -> list[CryptodevResults]:
>              case TestType.pmd_cyclecount:
>                  parser = PmdCyclecountResults
>              case TestType.verify:
> +                parser_options |= re.DOTALL
>                  parser = VerifyResults

Does this code risk matching content across multiple records when using
DOTALL mode? Can this cause parser_options to combine flags from
different test types inappropriately if called multiple times?

> diff --git a/dts/api/cryptodev/types.py b/dts/api/cryptodev/types.py
> index df73a86fa4..861d46bf13 100644
> --- a/dts/api/cryptodev/types.py
> +++ b/dts/api/cryptodev/types.py
> @@ -160,26 +160,22 @@ class VerifyResults(CryptodevResults):
>      """A parser for verify test output."""
> 
>      #:
> -    lcore_id: int = field(metadata=TextParser.find_int(r"lcore\s+(?:id.*\n\s+)?(\d+)"))
> +    lcore_id: int = field(metadata=TextParser.find_int(r"\s*(\d+)"))

Does this pattern r"\s*(\d+)" match the intended field reliably? This
regex will match the first integer it finds after optional whitespace,
which seems overly generic. Could this match an unintended number earlier
in the output?

>      #: buffer size ran with app
>      buffer_size: int = field(
> -        metadata=TextParser.find_int(r"Buf(?:.*\n\s+(?:\d+\s+))?(?:fer size:\s+)?(\d+)"),
> +        metadata=TextParser.find_int(r"\s*(?:\d+\s+)(\d+)"),
>      )

Does the pattern r"\s*(?:\d+\s+)(\d+)" correctly isolate the buffer_size
field? Does it skip exactly one preceding integer, or could it match
different fields in different output formats?

>      #: burst size ran with app
>      burst_size: int = field(
> -        metadata=TextParser.find_int(r"Burst(?:.*\n\s+(?:\d+\s+){2})?(?: size:\s+)?(\d+)"),
> +        metadata=TextParser.find_int(r"\s*(?:\d+\s+){2}(\d+)"),
>      )
>      #: number of packets enqueued
> -    enqueued: int = field(metadata=TextParser.find_int(r"Enqueued.*\n\s+(?:\d+\s+){3}(\d+)"))
> +    enqueued: int = field(metadata=TextParser.find_int(r"\s*(?:\d+\s+){3}(\d+)"))
>      #: number of packets dequeued
> -    dequeued: int = field(metadata=TextParser.find_int(r"Dequeued.*\n\s+(?:\d+\s+){4}(\d+)"))
> +    dequeued: int = field(metadata=TextParser.find_int(r"\s*(?:\d+\s+){4}(\d+)"))
>      #: number of packets enqueue failed
> -    failed_enqueued: int = field(
> -        metadata=TextParser.find_int(r"Failed Enq.*\n\s+(?:\d+\s+){5}(\d+)")
> -    )
> +    failed_enqueued: int = field(metadata=TextParser.find_int(r"\s*(?:\d+\s+){5}(\d+)"))
>      #: number of packets dequeue failed
> -    failed_dequeued: int = field(
> -        metadata=TextParser.find_int(r"Failed Deq.*\n\s+(?:\d+\s+){6}(\d+)")
> -    )
> +    failed_dequeued: int = field(metadata=TextParser.find_int(r"\s*(?:\d+\s+){6}(\d+)"))
>      #: total number of failed operations
> -    failed_ops: int = field(metadata=TextParser.find_int(r"Failed Ops.*\n\s+(?:\d+\s+){7}(\d+)"))
> +    failed_ops: int = field(metadata=TextParser.find_int(r"\s*(?:\d+\s+){7}(\d+)"))

Does this sequence of patterns using r"\s*(?:\d+\s+){N}(\d+)" reliably
extract the correct column from tabular output? Are these patterns all
applied to the same input string, and if so, does each pattern start
matching from the beginning? Could overlapping matches cause incorrect
field assignments?


git sha: 20260514172553.191331-3-abailey at iol.unh.edu (patch 3/3)
Author: Andrew Bailey <abailey at iol.unh.edu>
Subject: [PATCH v1 3/3] dts: add verify coverage for cryptodev testing

This commit adds a new test suite for functional testing of cryptodev
using the dpdk-test-crypto application verify mode.

> diff --git a/dts/tests/TestSuite_cryptodev_verify.py b/dts/tests/TestSuite_cryptodev_verify.py
> new file mode 100644
> index 0000000000..45014a4243
> --- /dev/null
> +++ b/dts/tests/TestSuite_cryptodev_verify.py

[ ... ]

> @@ -0,0 +1,160 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2026 University of New Hampshire
> +
> +"""DPDK cryptodev verify test suite.
> +
> +The main goal of this test suite is to utilize the verify mode of dpdk-test-crypto application
> +to ensure functional correctness for various cryptographic operations supported by DPDK
> +cryptodev-pmd.
> +"""
> +
> +from api.capabilities import (
> +    LinkTopology,
> +    requires_link_topology,
> +)
> +from api.cryptodev import Cryptodev
> +from api.cryptodev.config import (
> +    AeadAlgName,
> +    AuthenticationAlgorithm,
> +    AuthenticationOpMode,
> +    CipherAlgorithm,
> +    DeviceType,
> +    EncryptDecryptSwitch,
> +    OperationType,
> +    TestType,
> +    get_device_from_str,
> +)
> +from api.cryptodev.types import (
> +    CryptodevResults,
> +)
> +from api.test import verify
> +from framework.context import get_ctx
> +from framework.test_suite import TestSuite, crypto_test
> +from framework.testbed_model.virtual_device import VirtualDevice
> +
> +TOTAL_OPS = 10_000_000
> +AES_CBC_DATA = "test_aes_cbc.data"
> +AES_GCM_DATA = "test_aes_gcm.data"

[ ... ]

> +    def _verify_output(
> +        self,
> +        results: list[CryptodevResults],
> +    ) -> bool:
> +        for result in results:
> +            if (
> +                getattr(result, "failed_enqueued") > 0
> +                or getattr(result, "failed_dequeued") > 0
> +                or getattr(result, "failed_ops") > 0
> +            ):
> +                return False
> +        return True

Does this function properly handle an empty results list? Can results be
None or contain None elements, and if so, does getattr handle that
gracefully?

> +    @crypto_test
> +    def aesni_mb_vdev(self) -> None:

[ ... ]

> +        app = Cryptodev(
> +            vdevs=[VirtualDevice("crypto_aesni_mb0")],
> +            ptest=TestType.verify,
> +            test_file=AES_CBC_DATA,
> +            test_name="sha1_hmac_buff_32",
> +            devtype=DeviceType.crypto_aesni_mb,
> +            optype=OperationType.cipher_then_auth,
> +            cipher_algo=CipherAlgorithm.aes_cbc,
> +            cipher_op=EncryptDecryptSwitch.encrypt,
> +            cipher_key_sz=32,
> +            cipher_iv_sz=16,
> +            auth_algo=AuthenticationAlgorithm.sha1_hmac,
> +            auth_op=AuthenticationOpMode.generate,
> +            auth_key_sz=64,
> +            digest_sz=12,
> +            burst_sz=32,
> +            buffer_sz=32,
> +            total_ops=TOTAL_OPS,
> +        )
> +
> +        verify(self._verify_output(app.run_app()), "Failed to verify test sha1_hmac_buff_32")

Does the Cryptodev constructor or run_app method leak resources on
failure? Does app.run_app() need explicit cleanup in error paths?

> +    @crypto_test
> +    def openssl_vdev(self) -> None:

[ ... ]

> +        app = Cryptodev(
> +            vdevs=[VirtualDevice("crypto_openssl0")],
> +            ptest=TestType.verify,
> +            test_file=AES_GCM_DATA,
> +            test_name="aes_gcm_buff_32",
> +            devtype=DeviceType.crypto_openssl,
> +            optype=OperationType.aead,
> +            aead_algo=AeadAlgName.aes_gcm,
> +            aead_op=EncryptDecryptSwitch.encrypt,
> +            aead_key_sz=16,
> +            aead_aad_sz=16,
> +            aead_iv_sz=12,
> +            digest_sz=16,
> +            burst_sz=32,
> +            buffer_sz=32,
> +            total_ops=TOTAL_OPS,
> +        )
> +
> +        verify(self._verify_output(app.run_app()), "Failed to verify test aes_gcm_buff_32")

Same question: does app cleanup happen automatically if run_app() fails?

> +    @crypto_test
> +    def sha1_hmac_buff_32(self) -> None:

[ ... ]

> +        app = Cryptodev(
> +            ptest=TestType.verify,
> +            test_file=AES_CBC_DATA,
> +            test_name="sha1_hmac_buff_32",
> +            devtype=self.device_type,
> +            optype=OperationType.cipher_then_auth,
> +            cipher_algo=CipherAlgorithm.aes_cbc,
> +            cipher_op=EncryptDecryptSwitch.encrypt,
> +            cipher_key_sz=32,
> +            cipher_iv_sz=16,
> +            auth_algo=AuthenticationAlgorithm.sha1_hmac,
> +            auth_op=AuthenticationOpMode.generate,
> +            auth_key_sz=64,
> +            digest_sz=20,
> +            burst_sz=32,
> +            buffer_sz=32,
> +            total_ops=TOTAL_OPS,
> +        )
> +
> +        verify(self._verify_output(app.run_app()), "Failed to verify test sha1_hmac_buff_32")

Does self.device_type being None cause an exception here? Does the
Cryptodev constructor validate devtype or can it silently accept None?

[ ... ]