Re: [PATCH v3 1/1] dts: add text parser for testpmd verbose output

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

> diff --git a/dts/framework/parser.py b/dts/framework/parser.py
> index 741dfff821..0b39025a48 100644
> --- a/dts/framework/parser.py
> +++ b/dts/framework/parser.py
> @@ -160,6 +160,36 @@ def _find(text: str) -> Any:
>   
>           return ParserFn(TextParser_fn=_find)
>   
> +    @staticmethod
> +    def find_all(
> +        pattern: str | re.Pattern[str],
> +        flags: re.RegexFlag = re.RegexFlag(0),
> +    ) -> ParserFn:

I'd remove this if it's not used, the rule being let's not introduce 
unused code because it's not going to be maintained. We can always add 
it when needed.

> +        """Makes a parser function that finds all of the regular expression matches in the text.
> +
> +        If there are no matches found in the text than None will be returned, otherwise a list

then None, but maybe a comma would be better (found in the text, None 
will be returned)

> +        containing all matches will be returned. Patterns that contain multiple groups will pack
> +        the matches for each group into a tuple.
> +

> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index 43e9f56517..7d0b5a374c 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -31,7 +31,7 @@
>   from framework.settings import SETTINGS
>   from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList
>   from framework.testbed_model.sut_node import SutNode
> -from framework.utils import StrEnum
> +from framework.utils import REGEX_FOR_MAC_ADDRESS, StrEnum
>   
>   
>   class TestPmdDevice:
> @@ -577,6 +577,377 @@ class TestPmdPortStats(TextParser):
>       tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
>   
>   
> +class OLFlag(Flag):

We should come up with a consistent naming scheme for the various 
offloads. In the capabilities patch, I've introduced 
RxOffloadCapability. I think we can use the full word Offload and we 
should also capture in the name what sort of offload it is. In this 
case, would PacketOffloadFlag be a good name?

> +    """Flag representing the Packet Offload Features Flags in DPDK.
> +
> +    Values in this class are taken from the definitions in the RTE MBUF core library in DPDK.

I like the reference, let's also mention the name of the file 
rte_mbuf_core.h. Maybe we should add more references like these to other 
flags.

> +    """
> +
> +    # RX flags
> +    #:
> +    RTE_MBUF_F_RX_RSS_HASH = auto()
> +
> +    #:

I noticed the flags are not sorted the same way as in rte_mbuf_core.h. I 
think there's value in using the same flag values.

We could also add descriptions to the flag if there are some to be found 
in rte_mbuf_core.h.

> +
> +    # TX flags
> +    #:
> +    RTE_MBUF_F_TX_OUTER_UDP_CKSUM = auto()

Since there is a gap between RX and TX flags, you can just assign the 
actual value here (1 << 41) and the continue using auto().


> +    @classmethod
> +    def from_str_list(cls, arr: list[str]) -> Self:
> +        """Makes an instance from a list containing the flag members.
> +
> +        Args:
> +            arr: A list of strings containing ol_flag values.
> +
> +        Returns:
> +            A new instance of the flag.
> +        """
> +        flag = cls(0)
> +        for name in arr:
> +            if name in cls.__members__:

We could also do if cls[name] in cls. It's basically the same thing, but 
doesn't use the dunder method.

> +                flag |= cls[name]
> +        return flag
> +
> +    @classmethod
> +    def make_parser(cls) -> ParserFn:
> +        """Makes a parser function.
> +
> +        Returns:
> +            ParserFn: A dictionary for the `dataclasses.field` metadata argument containing a
> +                parser function that makes an instance of this flag from text.
> +        """
> +        return TextParser.wrap(
> +            TextParser.wrap(TextParser.find(r"ol_flags: ([^\n]+)"), str.split),
> +            cls.from_str_list,
> +        )
> +
> +
> +class RtePTypes(Flag):
> +    """Flag representing possible packet types in DPDK verbose output."""

Now this docstring doesn't reference from where these come from.

I found these in rte_mbuf_ptype.h, but from what I can tell, they're not 
actual flags, just regular numbers:
#define RTE_PTYPE_L2_ETHER                  0x00000001
#define RTE_PTYPE_L2_ETHER_TIMESYNC         0x00000002

etc., so we're basically converting that to flags. I think this is OK 
and we don't really need to concern ourselves with the actual values, 
just the order.


> +    @classmethod
> +    def from_str_list(cls, arr: list[str]) -> Self:
> +        """Makes an instance from a list containing the flag members.
> +
> +        Args:
> +            arr: A list of strings containing ol_flag values.

ol_flag looks like a copy-paste.

> +
> +        Returns:
> +            A new instance of the flag.
> +        """
> +        flag = cls(0)
> +        for name in arr:
> +            if name in cls.__members__:
> +                flag |= cls[name]
> +        return flag
> +
> +    @classmethod
> +    def make_parser(cls, hw: bool) -> ParserFn:
> +        """Makes a parser function.
> +
> +        Args:
> +            hw: Whether to make a parser for hardware ptypes or software ptypes. If :data:`True`

I think there should be a comma before hardware (on the next line).

> +                hardware ptypes will be collected, otherwise software pytpes will.
> +
> +        Returns:
> +            ParserFn: A dictionary for the `dataclasses.field` metadata argument containing a
> +                parser function that makes an instance of this flag from text.
> +        """
> +        return TextParser.wrap(
> +            TextParser.wrap(TextParser.find(f"{'hw' if hw else 'sw'} ptype: ([^-]+)"), str.split),
> +            cls.from_str_list,
> +        )
> +
> +
> +@dataclass
> +class TestPmdVerbosePacket(TextParser):

> +    ol_flags: OLFlag = field(metadata=OLFlag.make_parser())
> +    #: RSS has of the packet in hex.

typo: hash


>   class TestPmdShell(DPDKShell):

> +    @staticmethod
> +    def extract_verbose_output(output: str) -> list[TestPmdVerbosePacket]:
> +        """Extract the verbose information present in given testpmd output.
> +
> +        This method extracts sections of verbose output that begin with the line
> +        "port X/queue Y: sent/received Z packets" and end with the ol_flags of a packet.
> +
> +        Args:
> +            output: Testpmd output that contains verbose information
> +
> +        Returns:
> +            List of parsed packet information gathered from verbose information in `output`.
> +        """
> +        out: list[TestPmdVerbosePacket] = []
> +        prev_header: str = ""
> +        iter = re.finditer(
> +            r"(?P<HEADER>(?:port \d+/queue \d+: received \d packets)?)\s*"

Looks like sent packets won't be captured by this.


> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> index 6b5d5a805f..9c64cf497f 100644
> --- a/dts/framework/utils.py
> +++ b/dts/framework/utils.py
> @@ -27,6 +27,7 @@
>   from .exception import ConfigurationError
>   
>   REGEX_FOR_PCI_ADDRESS: str = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
> +REGEX_FOR_MAC_ADDRESS: str = r"(?:[\da-fA-F]{2}:){5}[\da-fA-F]{2}"

Is this the only format that testpmd returns?