|SUCCESS| [v1] dts: add ipgre test suite

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-03-29

Commit: (git sha not directly provided in patch, from metadata appears to be related to patch 162761)
Author: Andrew Bailey <abailey at iol.unh.edu>
Subject: [PATCH v1] dts: add ipgre test suite

This patch ports the IP GRE test suite from old DTS to next DTS, covering
GRE tunneling and checksum offload verification.

> diff --git a/dts/api/testpmd/__init__.py b/dts/api/testpmd/__init__.py
> index e9187440bb..33bf5e63de 100644
> --- a/dts/api/testpmd/__init__.py
> +++ b/dts/api/testpmd/__init__.py
> @@ -951,6 +951,29 @@ def set_flow_control(
>                      f"Testpmd failed to set the {flow_ctrl} in port {port}."
>                  )
>  
> +    def set_csum_parse_tunnel(self, port: int, on: bool, verify: bool = True) -> None:
> +        """Set parse tunnel on or of in testpmd for a given port.

Typo: Should be "on or off" not "on or of"

> +
> +        Args:
> +            port: The ID of the requested port
> +            on: set parse tunnel on if `on` is :data:`True`, otherwise off
> +            verify: if :data:`True`, the output of the command is scanned to verify that
> +                parse tunnel was set successfully
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the command
> +                fails to execute.
> +
> +        """
> +        output = self.send_command(f"csum parse-tunnel {"on" if on else "off"} {port}")
> +        if verify and f"Parse tunnel is {'on' if on else'off'}" not in output:

Whitespace issue: Missing space before 'off' in the string literal. The
condition reads "else'off'" which would look for "Parse tunnel is off"
without a space between "is" and "off". Should be "else 'off'".

> +            self._logger.debug(
> +                f"Testpmd failed to set csum parse-tunnel {'on' if on else 'off'} in port {port}"
> +            )
> +            raise InteractiveCommandExecutionError(
> +                f"Testpmd failed to set csum parse-tunnel {'on' if on else 'off'} in port {port}"
> +            )
> +

[ ... ]

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

[ ... ]

> +    def _setup_session(
> +        self, testpmd: TestPmd, expected_flags: list[RtePTypes], packet_list=list[Packet]

Does this default argument create a mutable default? The pattern
"packet_list=list[Packet]" appears to be attempting to specify a type,
but in Python this actually tries to index the list type and doesn't
create an empty list. Should this be "packet_list: list[Packet]" with
a type annotation instead of a default value? This would cause a
TypeError at runtime if called without the argument.

> +    ) -> None:
> +        """Sets the forwarding and verbose mode of each test case interactive shell session."""
> +        testpmd.set_forward_mode(SimpleForwardingModes.rxonly)
> +        testpmd.set_verbose(level=1)
> +        for i in range(0, len(packet_list)):
> +            testpmd.start(verify=True)
> +            self._send_packet_and_verify_flags(
> +                expected_flag=expected_flags[i], packet=packet_list[i], testpmd=testpmd
> +            )

Does this code handle the case where expected_flags and packet_list have
different lengths? If len(expected_flags) is less than len(packet_list),
does this cause an IndexError when accessing expected_flags[i]?

> +
> +    def _send_packet_and_verify_checksum(
> +        self, packet: Packet, good_L4: bool, good_IP: bool, testpmd: TestPmd
> +    ) -> None:
> +        """Send packet and verify verbose output matches expected output."""
> +        testpmd.start()
> +        send_packet_and_capture(packet=packet)
> +        verbose_output = testpmd.extract_verbose_output(testpmd.stop())
> +        is_IP = is_L4 = None
> +        for testpmd_packet in verbose_output:
> +            if testpmd_packet.src_mac == SRC_ID:
> +                is_IP = PacketOffloadFlag.RTE_MBUF_F_RX_IP_CKSUM_GOOD in testpmd_packet.ol_flags
> +                is_L4 = PacketOffloadFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in testpmd_packet.ol_flags

Does this loop handle the case where multiple packets match SRC_ID? The
variables is_IP and is_L4 would be overwritten on each iteration. Is
this the intended behavior, or should the loop break after finding the
first match?

> +        verify(
> +            is_IP is not None and is_L4 is not None,
> +            "Test packet was dropped when it should have been received.",
> +        )

[ ... ]

> +    @func_test
> +    def gre_checksum_offload(self) -> None:

[ ... ]

> +        with TestPmd() as testpmd:
> +            testpmd.set_forward_mode(SimpleForwardingModes.csum)
> +            testpmd.csum_set_hw(
> +                layers=ChecksumOffloadOptions.ip
> +                | ChecksumOffloadOptions.udp
> +                | ChecksumOffloadOptions.outer_ip
> +                | ChecksumOffloadOptions.sctp
> +                | ChecksumOffloadOptions.tcp,
> +                port_id=0,
> +            )
> +            testpmd.set_csum_parse_tunnel(port=0, on=True)
> +            testpmd.set_verbose(1)
> +            testpmd.start_all_ports()
> +            testpmd.start()
> +            for i in range(len(packets)):
> +                self._send_packet_and_verify_checksum(
> +                    packets[i],
> +                    good_l4_ip[i][0],
> +                    good_l4_ip[i][1],
> +                    testpmd,
> +                )

Does this code verify that len(packets) equals len(good_l4_ip) before
indexing? If the lists have different lengths, does this cause an
IndexError?