[PATCH 0/6] dts: add testpmd params and statefulness

[PATCH 0/6] dts: add testpmd params and statefulness

[Luca Vizzarro]

Hello!

Sending in some major work relating to Bugzilla Bug 1371. In a few words
I have created a common data structure to handle command line parameters
for shells. I applied this to the current EalParameters class, and made
it so it's reflected across the interactive shell classes for
consistency. Finally, I have implemented all the testpmd parameters
that are publicly documented in a class, and updated the buffer scatter
test suite to use it. The two statefulness patches are very basic and
hopefully the beginning of tracking some state in each testpmd session.

Here are some things I'd like to discuss about these patches:
- the testpmd params defaults. These are a mix between the declared
  defaults on testpmd doc page and some of which we are aware of. I have
  not used all the defaults declared on the testpmd doc page, because I
  have found some inconsistencies when going through testpmd's source
  code.
- the overall structure of the parameter classes. Each of the parameter
  classes are placed in different files, not following a proper
  structure. I'd be keen to restructure everything, and I am open to
  suggestions.
- most of the docstrings relating to the testpmd parameters class are
  effectively taking from testpmd's doc pages. Would this satisfy our
  needs?
- I've tested the docstrings against Juraj's pending API doc generation
  patches and noticed that union types are not correctly represented
  when these are more than two. Not sure if this is a bug or not, but if
  not, any suggestions on how we could solve this?

Looking forward to hearing your replies!

Best regards,
Luca

Luca Vizzarro (6):
  dts: add parameters data structure
  dts: use Params for interactive shells
  dts: add testpmd shell params
  dts: use testpmd params for scatter test suite
  dts: add statefulness to InteractiveShell
  dts: add statefulness to TestPmdShell

 dts/framework/params.py                       | 232 ++++++
 .../remote_session/interactive_shell.py       |  26 +-
 dts/framework/remote_session/testpmd_shell.py | 680 +++++++++++++++++-
 dts/framework/testbed_model/__init__.py       |   2 +-
 dts/framework/testbed_model/node.py           |   4 +-
 dts/framework/testbed_model/os_session.py     |   4 +-
 dts/framework/testbed_model/sut_node.py       | 106 ++-
 dts/tests/TestSuite_pmd_buffer_scatter.py     |  20 +-
 8 files changed, 972 insertions(+), 102 deletions(-)
 create mode 100644 dts/framework/params.py

-- 
2.34.1

[PATCH 1/6] dts: add parameters data structure

[Luca Vizzarro]

This commit introduces a new "params" module, which adds a new way
to manage command line parameters. The provided Params dataclass
is able to read the fields of its child class and produce a string
representation to supply to the command line. Any data structure
that is intended to represent command line parameters can inherit
it. The representation can then manipulated by using the dataclass
field metadata in conjunction with the provided functions:

* value_only, used to supply a value without forming an option/flag
* options_end, used to prefix with an options end delimiter (`--`)
* short, used to define a short parameter name, e.g. `-p`
* long, used to define a long parameter name, e.g. `--parameter`
* multiple, used to turn a list into repeating parameters
* field_mixins, used to manipulate the string representation of the
  value

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 dts/framework/params.py | 232 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 232 insertions(+)
 create mode 100644 dts/framework/params.py

diff --git a/dts/framework/params.py b/dts/framework/params.py
new file mode 100644
index 0000000000..6b48c8353e
--- /dev/null
+++ b/dts/framework/params.py
@@ -0,0 +1,232 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2024 Arm Limited
+
+"""Parameter manipulation module.
+
+This module provides :class:`~Params` which can be used to model any data structure
+that is meant to represent any command parameters.
+"""
+
+from dataclasses import dataclass, field, fields
+from typing import Any, Callable, Literal, Reversible, TypeVar, Iterable
+from enum import Flag
+
+
+T = TypeVar("T")
+#: Type for a Mixin.
+Mixin = Callable[[Any], str]
+#: Type for an option parameter.
+Option = Literal[True, None]
+#: Type for a yes/no option parameter.
+BooleanOption = Literal[True, False, None]
+
+META_VALUE_ONLY = "value_only"
+META_OPTIONS_END = "options_end"
+META_SHORT_NAME = "short_name"
+META_LONG_NAME = "long_name"
+META_MULTIPLE = "multiple"
+META_MIXINS = "mixins"
+
+
+def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
+    """Injects the value of the attribute as-is without flag. Metadata modifier for :func:`dataclasses.field`."""
+    return {**metadata, META_VALUE_ONLY: True}
+
+
+def short(name: str, metadata: dict[str, Any] = {}) -> dict[str, Any]:
+    """Overrides any parameter name with the given short option. Metadata modifier for :func:`dataclasses.field`.
+
+    .. code:: python
+
+        logical_cores: str | None = field(default="1-4", metadata=short("l"))
+
+    will render as ``-l=1-4`` instead of ``--logical-cores=1-4``.
+    """
+    return {**metadata, META_SHORT_NAME: name}
+
+
+def long(name: str, metadata: dict[str, Any] = {}) -> dict[str, Any]:
+    """Overrides the inferred parameter name to the specified one. Metadata modifier for :func:`dataclasses.field`.
+
+    .. code:: python
+
+        x_name: str | None = field(default="y", metadata=long("x"))
+
+    will render as ``--x=y``, but the field is accessed and modified through ``x_name``.
+    """
+    return {**metadata, META_LONG_NAME: name}
+
+
+def options_end(metadata: dict[str, Any] = {}) -> dict[str, Any]:
+    """Precedes the value with an options end delimiter (``--``). Metadata modifier for :func:`dataclasses.field`."""
+    return {**metadata, META_OPTIONS_END: True}
+
+
+def multiple(metadata: dict[str, Any] = {}) -> dict[str, Any]:
+    """Specifies that this parameter is set multiple times. Must be a list. Metadata modifier for :func:`dataclasses.field`.
+
+    .. code:: python
+
+        ports: list[int] | None = field(default_factory=lambda: [0, 1, 2], metadata=multiple(param_name("port")))
+
+    will render as ``--port=0 --port=1 --port=2``. Note that modifiers can be chained like in this example.
+    """
+    return {**metadata, META_MULTIPLE: True}
+
+
+def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:
+    """Takes in a variable number of mixins to manipulate the value's rendering. Metadata modifier for :func:`dataclasses.field`.
+
+    The ``metadata`` keyword argument can be used to chain metadata modifiers together.
+
+    Mixins can be chained together, executed from right to left in the arguments list order.
+
+    Example:
+
+    .. code:: python
+
+        hex_bitmask: int | None = field(default=0b1101, metadata=field_mixins(hex, metadata=param_name("mask")))
+
+    will render as ``--mask=0xd``. The :func:`hex` built-in can be used as a mixin turning a valid integer into a hexadecimal representation.
+    """
+    return {**metadata, META_MIXINS: mixins}
+
+
+def _reduce_mixins(mixins: Reversible[Mixin], value: Any) -> str:
+    for mixin in reversed(mixins):
+        value = mixin(value)
+    return value
+
+
+def str_mixins(*mixins: Mixin):
+    """Decorator which modifies the ``__str__`` method, enabling support for mixins.
+
+    Mixins can be chained together, executed from right to left in the arguments list order.
+
+    Example:
+
+    .. code:: python
+
+        @str_mixins(hex_from_flag_value)
+        class BitMask(enum.Flag):
+            A = auto()
+            B = auto()
+
+    will allow ``BitMask`` to render as a hexadecimal value.
+    """
+
+    def _class_decorator(original_class):
+        original_class.__str__ = lambda self: _reduce_mixins(mixins, self)
+        return original_class
+
+    return _class_decorator
+
+
+def comma_separated(values: Iterable[T]) -> str:
+    """Mixin which renders an iterable in a comma-separated string."""
+    return ",".join([str(value).strip() for value in values if value is not None])
+
+
+def bracketed(value: str) -> str:
+    """Mixin which adds round brackets to the input."""
+    return f"({value})"
+
+
+def str_from_flag_value(flag: Flag) -> str:
+    """Mixin which returns the value from a :class:`enum.Flag` as a string."""
+    return str(flag.value)
+
+
+def hex_from_flag_value(flag: Flag) -> str:
+    """Mixin which turns a :class:`enum.Flag` value into hexadecimal."""
+    return hex(flag.value)
+
+
+def _make_option(param_name: str, short: bool = False, no: bool = False) -> str:
+    param_name = param_name.replace("_", "-")
+    return f"{'-' if short else '--'}{'no-' if no else ''}{param_name}"
+
+
+@dataclass
+class Params:
+    """Helper abstract dataclass that renders its fields into command line arguments.
+
+    The parameter name is taken from the field name by default. The following:
+
+    .. code:: python
+
+        name: str | None = "value"
+
+    is rendered as ``--name=value``.
+    Through :func:`dataclasses.field` the resulting parameter can be manipulated by applying
+    appropriate metadata. This class can be used with the following metadata modifiers:
+
+    * :func:`value_only`
+    * :func:`options_end`
+    * :func:`short`
+    * :func:`long`
+    * :func:`multiple`
+    * :func:`field_mixins`
+
+    To use fields as option switches set the value to ``True`` to enable them. If you
+    use a yes/no option switch you can also set ``False`` which would enable an option
+    prefixed with ``--no-``. Examples:
+
+    .. code:: python
+
+        interactive: Option = True  # renders --interactive
+        numa: BooleanOption = False # renders --no-numa
+
+    Setting ``None`` will disable any option. The :attr:`~Option` type alias is provided for
+    regular option switches, whereas :attr:`~BooleanOption` is offered for yes/no ones.
+
+    An instance of a dataclass inheriting ``Params`` can also be assigned to an attribute, this helps with grouping parameters
+    together. The attribute holding the dataclass will be ignored and the latter will just be rendered as expected.
+    """
+
+    def __str__(self) -> str:
+        arguments: list[str] = []
+
+        for field in fields(self):
+            value = getattr(self, field.name)
+
+            if value is None:
+                continue
+
+            options_end = field.metadata.get(META_OPTIONS_END, False)
+            if options_end:
+                arguments.append("--")
+
+            value_only = field.metadata.get(META_VALUE_ONLY, False)
+            if isinstance(value, Params) or value_only or options_end:
+                arguments.append(str(value))
+                continue
+
+            # take "short_name" metadata, or "long_name" metadata, or infer from field name
+            option_name = field.metadata.get(
+                META_SHORT_NAME, field.metadata.get(META_LONG_NAME, field.name)
+            )
+            is_short = META_SHORT_NAME in field.metadata
+
+            if isinstance(value, bool):
+                arguments.append(_make_option(option_name, short=is_short, no=(not value)))
+                continue
+
+            option = _make_option(option_name, short=is_short)
+            separator = " " if is_short else "="
+            str_mixins = field.metadata.get(META_MIXINS, [])
+            multiple = field.metadata.get(META_MULTIPLE, False)
+
+            values = value if multiple else [value]
+            for entry_value in values:
+                entry_value = _reduce_mixins(str_mixins, entry_value)
+                arguments.append(f"{option}{separator}{entry_value}")
+
+        return " ".join(arguments)
+
+
+@dataclass
+class StrParams(Params):
+    """A drop-in replacement for parameters passed as a string."""
+
+    value: str = field(metadata=value_only())
-- 
2.34.1

[PATCH 2/6] dts: use Params for interactive shells

[Luca Vizzarro]

Make it so that interactive shells accept an implementation of `Params`
for app arguments. Convert EalParameters to use `Params` instead.

String command line parameters can still be supplied by using the
`StrParams` implementation.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 .../remote_session/interactive_shell.py       |   8 +-
 dts/framework/remote_session/testpmd_shell.py |  12 +-
 dts/framework/testbed_model/__init__.py       |   2 +-
 dts/framework/testbed_model/node.py           |   4 +-
 dts/framework/testbed_model/os_session.py     |   4 +-
 dts/framework/testbed_model/sut_node.py       | 106 ++++++++----------
 dts/tests/TestSuite_pmd_buffer_scatter.py     |   3 +-
 7 files changed, 73 insertions(+), 66 deletions(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 5cfe202e15..a2c7b30d9f 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2023 University of New Hampshire
+# Copyright(c) 2024 Arm Limited
 
 """Common functionality for interactive shell handling.
 
@@ -21,6 +22,7 @@
 from paramiko import Channel, SSHClient, channel  # type: ignore[import]
 
 from framework.logger import DTSLogger
+from framework.params import Params
 from framework.settings import SETTINGS
 
 
@@ -40,7 +42,7 @@ class InteractiveShell(ABC):
     _ssh_channel: Channel
     _logger: DTSLogger
     _timeout: float
-    _app_args: str
+    _app_args: Params | None
 
     #: Prompt to expect at the end of output when sending a command.
     #: This is often overridden by subclasses.
@@ -63,7 +65,7 @@ def __init__(
         interactive_session: SSHClient,
         logger: DTSLogger,
         get_privileged_command: Callable[[str], str] | None,
-        app_args: str = "",
+        app_args: Params | None = None,
         timeout: float = SETTINGS.timeout,
     ) -> None:
         """Create an SSH channel during initialization.
@@ -100,7 +102,7 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
             get_privileged_command: A function (but could be any callable) that produces
                 the version of the command with elevated privileges.
         """
-        start_command = f"{self.path} {self._app_args}"
+        start_command = f"{self.path} {self._app_args or ''}"
         if get_privileged_command is not None:
             start_command = get_privileged_command(start_command)
         self.send_command(start_command)
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index cb2ab6bd00..db3abb7600 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -21,6 +21,7 @@
 from typing import Callable, ClassVar
 
 from framework.exception import InteractiveCommandExecutionError
+from framework.params import StrParams
 from framework.settings import SETTINGS
 from framework.utils import StrEnum
 
@@ -118,8 +119,15 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
         Also find the number of pci addresses which were allowed on the command line when the app
         was started.
         """
-        self._app_args += " -i --mask-event intr_lsc"
-        self.number_of_ports = self._app_args.count("-a ")
+        from framework.testbed_model.sut_node import EalParameters
+
+        assert isinstance(self._app_args, EalParameters)
+
+        if isinstance(self._app_args.app_params, StrParams):
+            self._app_args.app_params.value += " -i --mask-event intr_lsc"
+
+        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
+
         super()._start_application(get_privileged_command)
 
     def start(self, verify: bool = True) -> None:
diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py
index 6086512ca2..ef9520df4c 100644
--- a/dts/framework/testbed_model/__init__.py
+++ b/dts/framework/testbed_model/__init__.py
@@ -23,6 +23,6 @@
 from .cpu import LogicalCoreCount, LogicalCoreCountFilter, LogicalCoreList
 from .node import Node
 from .port import Port, PortLink
-from .sut_node import SutNode
+from .sut_node import SutNode, EalParameters
 from .tg_node import TGNode
 from .virtual_device import VirtualDevice
diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
index 74061f6262..ec9512d618 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -2,6 +2,7 @@
 # Copyright(c) 2010-2014 Intel Corporation
 # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
 # Copyright(c) 2022-2023 University of New Hampshire
+# Copyright(c) 2024 Arm Limited
 
 """Common functionality for node management.
 
@@ -24,6 +25,7 @@
 )
 from framework.exception import ConfigurationError
 from framework.logger import DTSLogger, get_dts_logger
+from framework.params import Params
 from framework.settings import SETTINGS
 
 from .cpu import (
@@ -199,7 +201,7 @@ def create_interactive_shell(
         shell_cls: Type[InteractiveShellType],
         timeout: float = SETTINGS.timeout,
         privileged: bool = False,
-        app_args: str = "",
+        app_args: Params | None = None,
     ) -> InteractiveShellType:
         """Factory for interactive session handlers.
 
diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py
index d5bf7e0401..7234c975c8 100644
--- a/dts/framework/testbed_model/os_session.py
+++ b/dts/framework/testbed_model/os_session.py
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2023 PANTHEON.tech s.r.o.
 # Copyright(c) 2023 University of New Hampshire
+# Copyright(c) 2024 Arm Limited
 
 """OS-aware remote session.
 
@@ -29,6 +30,7 @@
 
 from framework.config import Architecture, NodeConfiguration, NodeInfo
 from framework.logger import DTSLogger
+from framework.params import Params
 from framework.remote_session import (
     CommandResult,
     InteractiveRemoteSession,
@@ -134,7 +136,7 @@ def create_interactive_shell(
         shell_cls: Type[InteractiveShellType],
         timeout: float,
         privileged: bool,
-        app_args: str,
+        app_args: Params | None,
     ) -> InteractiveShellType:
         """Factory for interactive session handlers.
 
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 97aa26d419..3f8c3807b3 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -2,6 +2,7 @@
 # Copyright(c) 2010-2014 Intel Corporation
 # Copyright(c) 2023 PANTHEON.tech s.r.o.
 # Copyright(c) 2023 University of New Hampshire
+# Copyright(c) 2024 Arm Limited
 
 """System under test (DPDK + hardware) node.
 
@@ -11,6 +12,7 @@
 """
 
 
+from dataclasses import dataclass, field
 import os
 import tarfile
 import time
@@ -23,6 +25,8 @@
     NodeInfo,
     SutNodeConfiguration,
 )
+from framework import params
+from framework.params import Params, StrParams
 from framework.remote_session import CommandResult
 from framework.settings import SETTINGS
 from framework.utils import MesonArgs
@@ -34,62 +38,51 @@
 from .virtual_device import VirtualDevice
 
 
-class EalParameters(object):
+def _port_to_pci(port: Port) -> str:
+    return port.pci
+
+
+@dataclass(kw_only=True)
+class EalParameters(Params):
     """The environment abstraction layer parameters.
 
     The string representation can be created by converting the instance to a string.
     """
 
-    def __init__(
-        self,
-        lcore_list: LogicalCoreList,
-        memory_channels: int,
-        prefix: str,
-        no_pci: bool,
-        vdevs: list[VirtualDevice],
-        ports: list[Port],
-        other_eal_param: str,
-    ):
-        """Initialize the parameters according to inputs.
-
-        Process the parameters into the format used on the command line.
+    lcore_list: LogicalCoreList = field(metadata=params.short("l"))
+    """The list of logical cores to use."""
 
-        Args:
-            lcore_list: The list of logical cores to use.
-            memory_channels: The number of memory channels to use.
-            prefix: Set the file prefix string with which to start DPDK, e.g.: ``prefix='vf'``.
-            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
-            vdevs: Virtual devices, e.g.::
+    memory_channels: int = field(metadata=params.short("n"))
+    """The number of memory channels to use."""
 
-                vdevs=[
-                    VirtualDevice('net_ring0'),
-                    VirtualDevice('net_ring1')
-                ]
-            ports: The list of ports to allow.
-            other_eal_param: user defined DPDK EAL parameters, e.g.:
-                ``other_eal_param='--single-file-segments'``
-        """
-        self._lcore_list = f"-l {lcore_list}"
-        self._memory_channels = f"-n {memory_channels}"
-        self._prefix = prefix
-        if prefix:
-            self._prefix = f"--file-prefix={prefix}"
-        self._no_pci = "--no-pci" if no_pci else ""
-        self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
-        self._ports = " ".join(f"-a {port.pci}" for port in ports)
-        self._other_eal_param = other_eal_param
-
-    def __str__(self) -> str:
-        """Create the EAL string."""
-        return (
-            f"{self._lcore_list} "
-            f"{self._memory_channels} "
-            f"{self._prefix} "
-            f"{self._no_pci} "
-            f"{self._vdevs} "
-            f"{self._ports} "
-            f"{self._other_eal_param}"
-        )
+    prefix: str = field(metadata=params.long("file-prefix"))
+    """Set the file prefix string with which to start DPDK, e.g.: ``prefix="vf"``."""
+
+    no_pci: params.Option
+    """Switch to disable PCI bus e.g.: ``no_pci=True``."""
+
+    vdevs: list[VirtualDevice] | None = field(
+        default=None, metadata=params.multiple(params.long("vdev"))
+    )
+    """Virtual devices, e.g.::
+
+        vdevs=[
+            VirtualDevice("net_ring0"),
+            VirtualDevice("net_ring1")
+        ]
+    """
+
+    ports: list[Port] | None = field(
+        default=None,
+        metadata=params.field_mixins(_port_to_pci, metadata=params.multiple(params.short("a"))),
+    )
+    """The list of ports to allow."""
+
+    other_eal_param: StrParams | None = None
+    """Any other EAL parameter(s)."""
+
+    app_params: Params | None = field(default=None, metadata=params.options_end())
+    """Parameters to pass to the underlying DPDK app."""
 
 
 class SutNode(Node):
@@ -350,7 +343,7 @@ def create_eal_parameters(
         ascending_cores: bool = True,
         prefix: str = "dpdk",
         append_prefix_timestamp: bool = True,
-        no_pci: bool = False,
+        no_pci: params.Option = None,
         vdevs: list[VirtualDevice] | None = None,
         ports: list[Port] | None = None,
         other_eal_param: str = "",
@@ -393,9 +386,6 @@ def create_eal_parameters(
         if prefix:
             self._dpdk_prefix_list.append(prefix)
 
-        if vdevs is None:
-            vdevs = []
-
         if ports is None:
             ports = self.ports
 
@@ -406,7 +396,7 @@ def create_eal_parameters(
             no_pci=no_pci,
             vdevs=vdevs,
             ports=ports,
-            other_eal_param=other_eal_param,
+            other_eal_param=StrParams(other_eal_param),
         )
 
     def run_dpdk_app(
@@ -442,7 +432,7 @@ def create_interactive_shell(
         shell_cls: Type[InteractiveShellType],
         timeout: float = SETTINGS.timeout,
         privileged: bool = False,
-        app_parameters: str = "",
+        app_parameters: Params | None = None,
         eal_parameters: EalParameters | None = None,
     ) -> InteractiveShellType:
         """Extend the factory for interactive session handlers.
@@ -459,6 +449,7 @@ def create_interactive_shell(
                 reading from the buffer and don't receive any data within the timeout
                 it will throw an error.
             privileged: Whether to run the shell with administrative privileges.
+            app_args: The arguments to be passed to the application.
             eal_parameters: List of EAL parameters to use to launch the app. If this
                 isn't provided or an empty string is passed, it will default to calling
                 :meth:`create_eal_parameters`.
@@ -470,9 +461,10 @@ def create_interactive_shell(
         """
         # We need to append the build directory and add EAL parameters for DPDK apps
         if shell_cls.dpdk_app:
-            if not eal_parameters:
+            if eal_parameters is None:
                 eal_parameters = self.create_eal_parameters()
-            app_parameters = f"{eal_parameters} -- {app_parameters}"
+            eal_parameters.app_params = app_parameters
+            app_parameters = eal_parameters
 
             shell_cls.path = self.main_session.join_remote_path(
                 self.remote_dpdk_build_dir, shell_cls.path
diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 3701c47408..4cdbdc4272 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -22,6 +22,7 @@
 from scapy.packet import Raw  # type: ignore[import]
 from scapy.utils import hexstr  # type: ignore[import]
 
+from framework.params import StrParams
 from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
 from framework.test_suite import TestSuite
 
@@ -103,7 +104,7 @@ def pmd_scatter(self, mbsize: int) -> None:
         """
         testpmd = self.sut_node.create_interactive_shell(
             TestPmdShell,
-            app_parameters=(
+            app_parameters=StrParams(
                 "--mbcache=200 "
                 f"--mbuf-size={mbsize} "
                 "--max-pkt-len=9000 "
-- 
2.34.1

[PATCH 3/6] dts: add testpmd shell params

[Luca Vizzarro]

Implement all the testpmd shell parameters into a data structure.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 dts/framework/remote_session/testpmd_shell.py | 633 +++++++++++++++++-
 1 file changed, 615 insertions(+), 18 deletions(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index db3abb7600..a823dc53be 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2023 University of New Hampshire
 # Copyright(c) 2023 PANTHEON.tech s.r.o.
+# Copyright(c) 2024 Arm Limited
 
 """Testpmd interactive shell.
 
@@ -15,10 +16,25 @@
     testpmd_shell.close()
 """
 
+from dataclasses import dataclass, field
+from enum import auto, Enum, Flag, unique
 import time
-from enum import auto
 from pathlib import PurePath
-from typing import Callable, ClassVar
+from typing import Callable, ClassVar, Literal, NamedTuple
+from framework.params import (
+    BooleanOption,
+    Params,
+    bracketed,
+    comma_separated,
+    Option,
+    field_mixins,
+    hex_from_flag_value,
+    multiple,
+    long,
+    short,
+    str_from_flag_value,
+    str_mixins,
+)
 
 from framework.exception import InteractiveCommandExecutionError
 from framework.params import StrParams
@@ -28,26 +44,79 @@
 from .interactive_shell import InteractiveShell
 
 
-class TestPmdDevice(object):
-    """The data of a device that testpmd can recognize.
+@str_mixins(bracketed, comma_separated)
+class TestPmdPortNUMAConfig(NamedTuple):
+    """DPDK port to NUMA socket association tuple."""
 
-    Attributes:
-        pci_address: The PCI address of the device.
+    port: int
+    socket: int
+
+
+@str_mixins(str_from_flag_value)
+@unique
+class TestPmdFlowDirection(Flag):
+    """Flag indicating the direction of the flow.
+
+    A bi-directional flow can be specified with the pipe:
+
+    >>> TestPmdFlowDirection.RX | TestPmdFlowDirection.TX
+    <TestPmdFlowDirection.TX|RX: 3>
     """
 
-    pci_address: str
+    #:
+    RX = 1 << 0
+    #:
+    TX = 1 << 1
 
-    def __init__(self, pci_address_line: str):
-        """Initialize the device from the testpmd output line string.
 
-        Args:
-            pci_address_line: A line of testpmd output that contains a device.
-        """
-        self.pci_address = pci_address_line.strip().split(": ")[1].strip()
+@str_mixins(bracketed, comma_separated)
+class TestPmdRingNUMAConfig(NamedTuple):
+    """Tuple associating DPDK port, direction of the flow and NUMA socket."""
+
+    port: int
+    direction: TestPmdFlowDirection
+    socket: int
+
+
+@str_mixins(comma_separated)
+class TestPmdEthPeer(NamedTuple):
+    """Tuple associating a MAC address to the specified DPDK port."""
+
+    port_no: int
+    mac_address: str
+
+
+@str_mixins(comma_separated)
+class TestPmdTxIPAddrPair(NamedTuple):
+    """Tuple specifying the source and destination IPs for the packets."""
+
+    source_ip: str
+    dest_ip: str
 
-    def __str__(self) -> str:
-        """The PCI address captures what the device is."""
-        return self.pci_address
+
+@str_mixins(comma_separated)
+class TestPmdTxUDPPortPair(NamedTuple):
+    """Tuple specifying the UDP source and destination ports for the packets.
+
+    If leaving ``dest_port`` unspecified, ``source_port`` will be used for the destination port as well.
+    """
+
+    source_port: int
+    dest_port: int | None = None
+
+
+class TestPmdPortTopology(StrEnum):
+    paired = auto()
+    """In paired mode, the forwarding is between pairs of ports,
+    for example: (0,1), (2,3), (4,5)."""
+    chained = auto()
+    """In chained mode, the forwarding is to the next available port in the port mask,
+    for example: (0,1), (1,2), (2,0).
+
+    The ordering of the ports can be changed using the portlist testpmd runtime function.
+    """
+    loop = auto()
+    """In loop mode, ingress traffic is simply transmitted back on the same interface."""
 
 
 class TestPmdForwardingModes(StrEnum):
@@ -81,6 +150,534 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
+@str_mixins(comma_separated)
+class XYPair(NamedTuple):
+    #:
+    X: int
+    #:
+    Y: int | None = None
+
+
+@str_mixins(hex_from_flag_value)
+@unique
+class TestPmdRXMultiQueueMode(Flag):
+    #:
+    RSS = 1 << 0
+    #:
+    DCB = 1 << 1
+    #:
+    VMDQ = 1 << 2
+
+
+@str_mixins(hex_from_flag_value)
+@unique
+class TestPmdHairpinMode(Flag):
+    TWO_PORTS_LOOP = 1 << 0
+    """Two hairpin ports loop."""
+    TWO_PORTS_PAIRED = 1 << 1
+    """Two hairpin ports paired."""
+    EXPLICIT_TX_FLOW = 1 << 4
+    """Explicit Tx flow rule."""
+    FORCE_RX_QUEUE_MEM_SETTINGS = 1 << 8
+    """Force memory settings of hairpin RX queue."""
+    FORCE_TX_QUEUE_MEM_SETTINGS = 1 << 9
+    """Force memory settings of hairpin TX queue."""
+    RX_QUEUE_USE_LOCKED_DEVICE_MEMORY = 1 << 12
+    """Hairpin RX queues will use locked device memory."""
+    RX_QUEUE_USE_RTE_MEMORY = 1 << 13
+    """Hairpin RX queues will use RTE memory."""
+    TX_QUEUE_USE_LOCKED_DEVICE_MEMORY = 1 << 16
+    """Hairpin TX queues will use locked device memory."""
+    TX_QUEUE_USE_RTE_MEMORY = 1 << 18
+    """Hairpin TX queues will use RTE memory."""
+
+
+class TestPmdEvent(StrEnum):
+    #:
+    unknown = auto()
+    #:
+    queue_state = auto()
+    #:
+    vf_mbox = auto()
+    #:
+    macsec = auto()
+    #:
+    intr_lsc = auto()
+    #:
+    intr_rmv = auto()
+    #:
+    intr_reset = auto()
+    #:
+    dev_probed = auto()
+    #:
+    dev_released = auto()
+    #:
+    flow_aged = auto()
+    #:
+    err_recovering = auto()
+    #:
+    recovery_success = auto()
+    #:
+    recovery_failed = auto()
+    #:
+    all = auto()
+
+
+class TestPmdMempoolAllocationMode(StrEnum):
+    native = auto()
+    """Create and populate mempool using native DPDK memory."""
+    anon = auto()
+    """Create mempool using native DPDK memory, but populate using anonymous memory."""
+    xmem = auto()
+    """Create and populate mempool using externally and anonymously allocated area."""
+    xmemhuge = auto()
+    """Create and populate mempool using externally and anonymously allocated hugepage area."""
+
+
+@dataclass(kw_only=True)
+class TestPmdTXOnlyForwardingMode(Params):
+    __forward_mode: Literal[TestPmdForwardingModes.txonly] = field(
+        default=TestPmdForwardingModes.txonly, init=False, metadata=long("forward-mode")
+    )
+    multi_flow: Option = field(default=None, metadata=long("txonly-multi-flow"))
+    """Generate multiple flows."""
+    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
+    """Set TX segment sizes or total packet length."""
+
+
+@dataclass(kw_only=True)
+class TestPmdFlowGenForwardingMode(Params):
+    __forward_mode: Literal[TestPmdForwardingModes.flowgen] = field(
+        default=TestPmdForwardingModes.flowgen, init=False, metadata=long("forward-mode")
+    )
+    clones: int | None = field(default=None, metadata=long("flowgen-clones"))
+    """Set the number of each packet clones to be sent. Sending clones reduces host CPU load on
+    creating packets and may help in testing extreme speeds or maxing out Tx packet performance.
+    N should be not zero, but less than ‘burst’ parameter.
+    """
+    flows: int | None = field(default=None, metadata=long("flowgen-flows"))
+    """Set the number of flows to be generated, where 1 <= N <= INT32_MAX."""
+    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
+    """Set TX segment sizes or total packet length."""
+
+
+@dataclass(kw_only=True)
+class TestPmdNoisyForwardingMode(Params):
+    __forward_mode: Literal[TestPmdForwardingModes.noisy] = field(
+        default=TestPmdForwardingModes.noisy, init=False, metadata=long("forward-mode")
+    )
+    forward_mode: (
+        Literal[
+            TestPmdForwardingModes.io,
+            TestPmdForwardingModes.mac,
+            TestPmdForwardingModes.macswap,
+            TestPmdForwardingModes.fivetswap,
+        ]
+        | None
+    ) = field(default=TestPmdForwardingModes.io, metadata=long("noisy-forward-mode"))
+    """Set the noisy vnf forwarding mode."""
+    tx_sw_buffer_size: int | None = field(default=None, metadata=long("noisy-tx-sw-buffer-size"))
+    """Set the maximum number of elements of the FIFO queue to be created for buffering packets.
+    The default value is 0.
+    """
+    tx_sw_buffer_flushtime: int | None = field(
+        default=None, metadata=long("noisy-tx-sw-buffer-flushtime")
+    )
+    """Set the time before packets in the FIFO queue are flushed. The default value is 0."""
+    lkup_memory: int | None = field(default=None, metadata=long("noisy-lkup-memory"))
+    """Set the size of the noisy neighbor simulation memory buffer in MB to N. The default value is 0."""
+    lkup_num_reads: int | None = field(default=None, metadata=long("noisy-lkup-num-reads"))
+    """Set the number of reads to be done in noisy neighbor simulation memory buffer to N.
+    The default value is 0.
+    """
+    lkup_num_writes: int | None = field(default=None, metadata=long("noisy-lkup-num-writes"))
+    """Set the number of writes to be done in noisy neighbor simulation memory buffer to N.
+    The default value is 0.
+    """
+    lkup_num_reads_writes: int | None = field(
+        default=None, metadata=long("noisy-lkup-num-reads-writes")
+    )
+    """Set the number of r/w accesses to be done in noisy neighbor simulation memory buffer to N.
+    The default value is 0.
+    """
+
+
+@dataclass(kw_only=True)
+class TestPmdAnonMempoolAllocationMode(Params):
+    __mp_alloc: Literal[TestPmdMempoolAllocationMode.anon] = field(
+        default=TestPmdMempoolAllocationMode.anon, init=False, metadata=long("mp-alloc")
+    )
+    no_iova_contig: Option = None
+    """Enable to create mempool which is not IOVA contiguous."""
+
+
+@dataclass(kw_only=True)
+class TestPmdRXRingParams(Params):
+    descriptors: int | None = field(default=None, metadata=long("rxd"))
+    """Set the number of descriptors in the RX rings to N, where N > 0. The default value is 128."""
+    prefetch_threshold: int | None = field(default=None, metadata=long("rxpt"))
+    """Set the prefetch threshold register of RX rings to N, where N >= 0. The default value is 8."""
+    host_threshold: int | None = field(default=None, metadata=long("rxht"))
+    """Set the host threshold register of RX rings to N, where N >= 0. The default value is 8."""
+    write_back_threshold: int | None = field(default=None, metadata=long("rxwt"))
+    """Set the write-back threshold register of RX rings to N, where N >= 0. The default value is 4."""
+    free_threshold: int | None = field(default=None, metadata=long("rxfreet"))
+    """Set the free threshold of RX descriptors to N, where 0 <= N < value of ``-–rxd``.
+    The default value is 0.
+    """
+
+
+@dataclass
+class TestPmdDisableRSS(Params):
+    """Disable RSS (Receive Side Scaling)."""
+
+    __disable_rss: Literal[True] = field(default=True, init=False, metadata=long("disable-rss"))
+
+
+@dataclass
+class TestPmdSetRSSIPOnly(Params):
+    """Set RSS functions for IPv4/IPv6 only."""
+
+    __rss_ip: Literal[True] = field(default=True, init=False, metadata=long("rss-ip"))
+
+
+@dataclass
+class TestPmdSetRSSUDP(Params):
+    """Set RSS functions for IPv4/IPv6 and UDP."""
+
+    __rss_udp: Literal[True] = field(default=True, init=False, metadata=long("rss-udp"))
+
+
+@dataclass(kw_only=True)
+class TestPmdTXRingParams(Params):
+    descriptors: int | None = field(default=None, metadata=long("txd"))
+    """Set the number of descriptors in the TX rings to N, where N > 0. The default value is 512."""
+    rs_bit_threshold: int | None = field(default=None, metadata=long("txrst"))
+    """Set the transmit RS bit threshold of TX rings to N, where 0 <= N <= value of ``--txd``.
+    The default value is 0.
+    """
+    prefetch_threshold: int | None = field(default=None, metadata=long("txpt"))
+    """Set the prefetch threshold register of TX rings to N, where N >= 0. The default value is 36."""
+    host_threshold: int | None = field(default=None, metadata=long("txht"))
+    """Set the host threshold register of TX rings to N, where N >= 0. The default value is 0."""
+    write_back_threshold: int | None = field(default=None, metadata=long("txwt"))
+    """Set the write-back threshold register of TX rings to N, where N >= 0. The default value is 0."""
+    free_threshold: int | None = field(default=None, metadata=long("txfreet"))
+    """Set the transmit free threshold of TX rings to N, where 0 <= N <= value of ``--txd``.
+    The default value is 0.
+    """
+
+
+@dataclass(slots=True, kw_only=True)
+class TestPmdParameters(Params):
+    """The testpmd shell parameters.
+
+    The string representation can be created by converting the instance to a string.
+    """
+
+    interactive_mode: Option = field(default=True, metadata=short("i"))
+    """Runs testpmd in interactive mode."""
+    auto_start: Option = field(default=None, metadata=short("a"))
+    """Start forwarding on initialization."""
+    tx_first: Option = None
+    """Start forwarding, after sending a burst of packets first."""
+
+    stats_period: int | None = None
+    """Display statistics every ``PERIOD`` seconds, if interactive mode is disabled.
+    The default value is 0, which means that the statistics will not be displayed.
+
+    .. note:: This flag should be used only in non-interactive mode.
+    """
+
+    display_xstats: list[str] | None = field(default=None, metadata=field_mixins(comma_separated))
+    """Display comma-separated list of extended statistics every ``PERIOD`` seconds as specified in
+    ``--stats-period`` or when used with interactive commands that show Rx/Tx statistics
+    (i.e. ‘show port stats’).
+    """
+
+    nb_cores: int | None = 1
+    """Set the number of forwarding cores, where 1 <= N <= “number of cores” or ``RTE_MAX_LCORE``
+    from the configuration file. The default value is 1.
+    """
+    coremask: int | None = field(default=None, metadata=field_mixins(hex))
+    """Set the hexadecimal bitmask of the cores running the packet forwarding test. The main lcore
+    is reserved for command line parsing only and cannot be masked on for packet forwarding.
+    """
+
+    nb_ports: int | None = None
+    """Set the number of forwarding ports, where 1 <= N <= “number of ports” on the board or
+    ``RTE_MAX_ETHPORTS`` from the configuration file. The default value is the number of ports
+    on the board.
+    """
+    port_topology: TestPmdPortTopology | None = TestPmdPortTopology.paired
+    """Set port topology, where mode is paired (the default), chained or loop."""
+    portmask: int | None = field(default=None, metadata=field_mixins(hex))
+    """Set the hexadecimal bitmask of the ports used by the packet forwarding test."""
+    portlist: str | None = None  # TODO: can be ranges 0,1-3
+    """Set the forwarding ports based on the user input used by the packet forwarding test.
+    ‘-‘ denotes a range of ports to set including the two specified port IDs ‘,’ separates
+    multiple port values. Possible examples like –portlist=0,1 or –portlist=0-2 or –portlist=0,1-2 etc
+    """
+
+    numa: BooleanOption = True
+    """Enable/disable NUMA-aware allocation of RX/TX rings and of RX memory buffers (mbufs). Enabled by default."""
+    socket_num: int | None = None
+    """Set the socket from which all memory is allocated in NUMA mode, where 0 <= N < number of sockets on the board."""
+    port_numa_config: list[TestPmdPortNUMAConfig] | None = field(
+        default=None, metadata=field_mixins(comma_separated)
+    )
+    """Specify the socket on which the memory pool to be used by the port will be allocated."""
+    ring_numa_config: list[TestPmdRingNUMAConfig] | None = field(
+        default=None, metadata=field_mixins(comma_separated)
+    )
+    """Specify the socket on which the TX/RX rings for the port will be allocated.
+    Where flag is 1 for RX, 2 for TX, and 3 for RX and TX.
+    """
+
+    # Mbufs
+    total_num_mbufs: int | None = None
+    """Set the number of mbufs to be allocated in the mbuf pools, where N > 1024."""
+    mbuf_size: list[int] | None = field(default=None, metadata=field_mixins(comma_separated))
+    """Set the data size of the mbufs used to N bytes, where N < 65536. The default value is 2048.
+    If multiple mbuf-size values are specified the extra memory pools will be created for
+    allocating mbufs to receive packets with buffer splitting features.
+    """
+    mbcache: int | None = None
+    """Set the cache of mbuf memory pools to N, where 0 <= N <= 512. The default value is 16."""
+
+    max_pkt_len: int | None = None
+    """Set the maximum packet size to N bytes, where N >= 64. The default value is 1518."""
+
+    eth_peers_configfile: PurePath | None = None
+    """Use a configuration file containing the Ethernet addresses of the peer ports."""
+    eth_peer: list[TestPmdEthPeer] | None = field(default=None, metadata=multiple())
+    """Set the MAC address XX:XX:XX:XX:XX:XX of the peer port N, where 0 <= N < RTE_MAX_ETHPORTS."""
+
+    tx_ip: TestPmdTxIPAddrPair | None = TestPmdTxIPAddrPair(
+        source_ip="198.18.0.1", dest_ip="198.18.0.2"
+    )
+    """Set the source and destination IP address used when doing transmit only test.
+    The defaults address values are source 198.18.0.1 and destination 198.18.0.2.
+    These are special purpose addresses reserved for benchmarking (RFC 5735).
+    """
+    tx_udp: TestPmdTxUDPPortPair | None = TestPmdTxUDPPortPair(9)
+    """Set the source and destination UDP port number for transmit test only test.
+    The default port is the port 9 which is defined for the discard protocol (RFC 863)."""
+
+    enable_lro: Option = None
+    """Enable large receive offload."""
+    max_lro_pkt_size: int | None = None
+    """Set the maximum LRO aggregated packet size to N bytes, where N >= 64."""
+
+    disable_crc_strip: Option = None
+    """Disable hardware CRC stripping."""
+    enable_scatter: Option = None
+    """Enable scatter (multi-segment) RX."""
+    enable_hw_vlan: Option = None
+    """Enable hardware VLAN."""
+    enable_hw_vlan_filter: Option = None
+    """Enable hardware VLAN filter."""
+    enable_hw_vlan_strip: Option = None
+    """Enable hardware VLAN strip."""
+    enable_hw_vlan_extend: Option = None
+    """Enable hardware VLAN extend."""
+    enable_hw_qinq_strip: Option = None
+    """Enable hardware QINQ strip."""
+    pkt_drop_enabled: Option = field(default=None, metadata=long("enable-drop-en"))
+    """Enable per-queue packet drop for packets with no descriptors."""
+
+    rss: TestPmdDisableRSS | TestPmdSetRSSIPOnly | TestPmdSetRSSUDP | None = None
+    """RSS option setting.
+
+    The value can be one of:
+    * :class:`TestPmdDisableRSS`, to disable RSS
+    * :class:`TestPmdSetRSSIPOnly`, to set RSS for IPv4/IPv6 only
+    * :class:`TestPmdSetRSSUDP`, to set RSS for IPv4/IPv6 and UDP
+    """
+
+    forward_mode: (
+        Literal[
+            TestPmdForwardingModes.io,
+            TestPmdForwardingModes.mac,
+            TestPmdForwardingModes.macswap,
+            TestPmdForwardingModes.rxonly,
+            TestPmdForwardingModes.csum,
+            TestPmdForwardingModes.icmpecho,
+            TestPmdForwardingModes.ieee1588,
+            TestPmdForwardingModes.fivetswap,
+            TestPmdForwardingModes.shared_rxq,
+            TestPmdForwardingModes.recycle_mbufs,
+        ]
+        | TestPmdFlowGenForwardingMode
+        | TestPmdTXOnlyForwardingMode
+        | TestPmdNoisyForwardingMode
+        | None
+    ) = TestPmdForwardingModes.io
+    """Set the forwarding mode.
+
+    The value can be one of:
+    * :attr:`TestPmdForwardingModes.io` (default)
+    * :attr:`TestPmdForwardingModes.mac`
+    * :attr:`TestPmdForwardingModes.rxonly`
+    * :attr:`TestPmdForwardingModes.csum`
+    * :attr:`TestPmdForwardingModes.icmpecho`
+    * :attr:`TestPmdForwardingModes.ieee1588`
+    * :attr:`TestPmdForwardingModes.fivetswap`
+    * :attr:`TestPmdForwardingModes.shared_rxq`
+    * :attr:`TestPmdForwardingModes.recycle_mbufs`
+    * :class:`FlowGenForwardingMode`
+    * :class:`TXOnlyForwardingMode`
+    * :class:`NoisyForwardingMode`
+    """
+
+    hairpin_mode: TestPmdHairpinMode | None = TestPmdHairpinMode(0)
+    """Set the hairpin port configuration."""
+    hairpin_queues: int | None = field(default=None, metadata=long("hairpinq"))
+    """Set the number of hairpin queues per port to N, where 1 <= N <= 65535. The default value is 0."""
+
+    burst: int | None = None
+    """Set the number of packets per burst to N, where 1 <= N <= 512. The default value is 32.
+    If set to 0, driver default is used if defined.
+    Else, if driver default is not defined, default of 32 is used.
+    """
+
+    # RX data parameters
+    enable_rx_cksum: Option = None
+    """Enable hardware RX checksum offload."""
+    rx_queues: int | None = field(default=None, metadata=long("rxq"))
+    """Set the number of RX queues per port to N, where 1 <= N <= 65535. The default value is 1."""
+    rx_ring: TestPmdRXRingParams | None = None
+    """Set the RX rings parameters."""
+    no_flush_rx: Option = None
+    """Don’t flush the RX streams before starting forwarding. Used mainly with the PCAP PMD."""
+    rx_segments_offsets: XYPair | None = field(default=None, metadata=long("rxoffs"))
+    """Set the offsets of packet segments on receiving if split feature is engaged.
+    Affects only the queues configured with split offloads (currently BUFFER_SPLIT is supported only).
+    """
+    rx_segments_length: XYPair | None = field(default=None, metadata=long("rxpkts"))
+    """Set the length of segments to scatter packets on receiving if split feature is engaged.
+    Affects only the queues configured with split offloads (currently BUFFER_SPLIT is supported only).
+    Optionally the multiple memory pools can be specified with –mbuf-size command line parameter and
+    the mbufs to receive will be allocated sequentially from these extra memory pools.
+    """
+    multi_rx_mempool: Option = None
+    """Enable multiple mbuf pools per Rx queue."""
+    rx_shared_queue: Option | int = field(default=None, metadata=long("rxq-share"))
+    """Create queues in shared Rx queue mode if device supports. Shared Rx queues are grouped per X ports.
+    X defaults to UINT32_MAX, implies all ports join share group 1.
+    Forwarding engine “shared-rxq” should be used for shared Rx queues.
+    This engine does Rx only and update stream statistics accordingly.
+    """
+    rx_offloads: int | None = field(default=0, metadata=field_mixins(hex))
+    """Set the hexadecimal bitmask of RX queue offloads. The default value is 0."""
+    rx_mq_mode: TestPmdRXMultiQueueMode | None = (
+        TestPmdRXMultiQueueMode.DCB | TestPmdRXMultiQueueMode.RSS | TestPmdRXMultiQueueMode.VMDQ
+    )
+    """Set the hexadecimal bitmask of RX multi queue mode which can be enabled."""
+
+    # TX data parameters
+    tx_queues: int | None = field(default=None, metadata=long("txq"))
+    """Set the number of TX queues per port to N, where 1 <= N <= 65535. The default value is 1."""
+    tx_ring: TestPmdTXRingParams | None = None
+    """Set the TX rings params."""
+    tx_offloads: int | None = field(default=0, metadata=field_mixins(hex))
+    """Set the hexadecimal bitmask of TX queue offloads. The default value is 0."""
+
+    eth_link_speed: int | None = None
+    """Set a forced link speed to the ethernet port. E.g. 1000 for 1Gbps."""
+    disable_link_check: Option = None
+    """Disable check on link status when starting/stopping ports."""
+    disable_device_start: Option = None
+    """Do not automatically start all ports.
+    This allows testing configuration of rx and tx queues before device is started for the first time.
+    """
+    no_lsc_interrupt: Option = None
+    """Disable LSC interrupts for all ports, even those supporting it."""
+    no_rmv_interrupt: Option = None
+    """Disable RMV interrupts for all ports, even those supporting it."""
+    bitrate_stats: int | None = None
+    """Set the logical core N to perform bitrate calculation."""
+    latencystats: int | None = None
+    """Set the logical core N to perform latency and jitter calculations."""
+    print_events: list[TestPmdEvent] | None = field(
+        default=None, metadata=multiple(long("print-event"))
+    )
+    """Enable printing the occurrence of the designated events.
+    Using :attr:`TestPmdEvent.ALL` will enable all of them.
+    """
+    mask_events: list[TestPmdEvent] | None = field(
+        default_factory=lambda: [TestPmdEvent.intr_lsc], metadata=multiple(long("mask-event"))
+    )
+    """Disable printing the occurrence of the designated events.
+    Using :attr:`TestPmdEvent.ALL` will disable all of them.
+    """
+
+    flow_isolate_all: Option = None
+    """Providing this parameter requests flow API isolated mode on all ports at initialization time.
+    It ensures all traffic is received through the configured flow rules only (see flow command).
+
+    Ports that do not support this mode are automatically discarded.
+    """
+    disable_flow_flush: Option = None
+    """Disable port flow flush when stopping port.
+    This allows testing keep flow rules or shared flow objects across restart.
+    """
+
+    hot_plug: Option = None
+    """Enable device event monitor mechanism for hotplug."""
+    vxlan_gpe_port: int | None = None
+    """Set the UDP port number of tunnel VXLAN-GPE to N. The default value is 4790."""
+    geneve_parsed_port: int | None = None
+    """Set the UDP port number that is used for parsing the GENEVE protocol to N.
+    HW may be configured with another tunnel Geneve port. The default value is 6081.
+    """
+    lock_all_memory: BooleanOption = field(default=False, metadata=long("mlockall"))
+    """Enable/disable locking all memory. Disabled by default."""
+    mempool_allocation_mode: (
+        Literal[
+            TestPmdMempoolAllocationMode.native,
+            TestPmdMempoolAllocationMode.xmem,
+            TestPmdMempoolAllocationMode.xmemhuge,
+        ]
+        | TestPmdAnonMempoolAllocationMode
+        | None
+    ) = field(default=None, metadata=long("mp-alloc"))
+    """Select mempool allocation mode.
+
+    The value can be one of:
+    * :attr:`TestPmdMempoolAllocationMode.native`
+    * :class:`TestPmdAnonMempoolAllocationMode`
+    * :attr:`TestPmdMempoolAllocationMode.xmem`
+    * :attr:`TestPmdMempoolAllocationMode.xmemhuge`
+    """
+    record_core_cycles: Option = None
+    """Enable measurement of CPU cycles per packet."""
+    record_burst_status: Option = None
+    """Enable display of RX and TX burst stats."""
+
+
+class TestPmdDevice(object):
+    """The data of a device that testpmd can recognize.
+
+    Attributes:
+        pci_address: The PCI address of the device.
+    """
+
+    pci_address: str
+
+    def __init__(self, pci_address_line: str):
+        """Initialize the device from the testpmd output line string.
+
+        Args:
+            pci_address_line: A line of testpmd output that contains a device.
+        """
+        self.pci_address = pci_address_line.strip().split(": ")[1].strip()
+
+    def __str__(self) -> str:
+        """The PCI address captures what the device is."""
+        return self.pci_address
+
+
 class TestPmdShell(InteractiveShell):
     """Testpmd interactive shell.
 
@@ -123,8 +720,8 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
 
         assert isinstance(self._app_args, EalParameters)
 
-        if isinstance(self._app_args.app_params, StrParams):
-            self._app_args.app_params.value += " -i --mask-event intr_lsc"
+        if self._app_args.app_params is None:
+            self._app_args.app_params = TestPmdParameters()
 
         self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
 
-- 
2.34.1

[PATCH 4/6] dts: use testpmd params for scatter test suite

[Luca Vizzarro]

Update the buffer scatter test suite to use TestPmdParameters
instead of the StrParams implementation.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 dts/tests/TestSuite_pmd_buffer_scatter.py | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index 4cdbdc4272..c6d313fc64 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -23,7 +23,11 @@
 from scapy.utils import hexstr  # type: ignore[import]
 
 from framework.params import StrParams
-from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
+from framework.remote_session.testpmd_shell import (
+    TestPmdForwardingModes,
+    TestPmdShell,
+    TestPmdParameters,
+)
 from framework.test_suite import TestSuite
 
 
@@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
         """
         testpmd = self.sut_node.create_interactive_shell(
             TestPmdShell,
-            app_parameters=StrParams(
-                "--mbcache=200 "
-                f"--mbuf-size={mbsize} "
-                "--max-pkt-len=9000 "
-                "--port-topology=paired "
-                "--tx-offloads=0x00008000"
+            app_parameters=TestPmdParameters(
+                forward_mode=TestPmdForwardingModes.mac,
+                mbcache=200,
+                mbuf_size=[mbsize],
+                max_pkt_len=9000,
+                tx_offloads=0x00008000,
             ),
             privileged=True,
         )
-        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
         testpmd.start()
 
         for offset in [-1, 0, 1, 4, 5]:
-- 
2.34.1

[PATCH 5/6] dts: add statefulness to InteractiveShell

[Luca Vizzarro]

The InteractiveShell class can be started in privileged mode, but this
is not saved for reference to the tests developer. Moreover, originally
a command timeout could only be set at initialisation, this can now be
amended and reset back as needed.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 .../remote_session/interactive_shell.py        | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index a2c7b30d9f..5d80061e8d 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -41,8 +41,10 @@ class InteractiveShell(ABC):
     _stdout: channel.ChannelFile
     _ssh_channel: Channel
     _logger: DTSLogger
+    __default_timeout: float
     _timeout: float
     _app_args: Params | None
+    _is_privileged: bool = False
 
     #: Prompt to expect at the end of output when sending a command.
     #: This is often overridden by subclasses.
@@ -88,7 +90,7 @@ def __init__(
         self._ssh_channel.settimeout(timeout)
         self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
         self._logger = logger
-        self._timeout = timeout
+        self._timeout = self.__default_timeout = timeout
         self._app_args = app_args
         self._start_application(get_privileged_command)
 
@@ -105,6 +107,7 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
         start_command = f"{self.path} {self._app_args or ''}"
         if get_privileged_command is not None:
             start_command = get_privileged_command(start_command)
+            self._is_privileged = True
         self.send_command(start_command)
 
     def send_command(self, command: str, prompt: str | None = None) -> str:
@@ -150,3 +153,16 @@ def close(self) -> None:
     def __del__(self) -> None:
         """Make sure the session is properly closed before deleting the object."""
         self.close()
+
+    @property
+    def is_privileged(self) -> bool:
+        """Property specifying if the interactive shell is running in privileged mode."""
+        return self._is_privileged
+
+    def set_timeout(self, timeout: float):
+        """Set the timeout to use with the next commands."""
+        self._timeout = timeout
+
+    def reset_timeout(self):
+        """Reset the timeout to the default setting."""
+        self._timeout = self.__default_timeout
-- 
2.34.1

[PATCH 6/6] dts: add statefulness to TestPmdShell

[Luca Vizzarro]

This commit provides a state container for TestPmdShell. It currently
only indicates whether the packet forwarding has started
or not, and the number of ports which were given to the shell.

This also fixes the behaviour of `wait_link_status_up` to use the
command timeout as inherited from InteractiveShell.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 dts/framework/remote_session/testpmd_shell.py | 41 +++++++++++++------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index a823dc53be..ea1d254f86 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -678,19 +678,27 @@ def __str__(self) -> str:
         return self.pci_address
 
 
+@dataclass(slots=True)
+class TestPmdState:
+    """Session state container."""
+
+    #:
+    packet_forwarding_started: bool = False
+
+    #: The number of ports which were allowed on the command-line when testpmd was started.
+    number_of_ports: int = 0
+
+
 class TestPmdShell(InteractiveShell):
     """Testpmd interactive shell.
 
     The testpmd shell users should never use
     the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
     call specialized methods. If there isn't one that satisfies a need, it should be added.
-
-    Attributes:
-        number_of_ports: The number of ports which were allowed on the command-line when testpmd
-            was started.
     """
 
-    number_of_ports: int
+    #: Current state
+    state: TestPmdState = TestPmdState()
 
     #: The path to the testpmd executable.
     path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")
@@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
         if self._app_args.app_params is None:
             self._app_args.app_params = TestPmdParameters()
 
-        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
+        assert isinstance(self._app_args.app_params, TestPmdParameters)
+
+        if self._app_args.app_params.auto_start:
+            self.state.packet_forwarding_started = True
+
+        if self._app_args.ports is not None:
+            self.state.number_of_ports = len(self._app_args.ports)
 
         super()._start_application(get_privileged_command)
 
@@ -746,12 +760,14 @@ def start(self, verify: bool = True) -> None:
                 self._logger.debug(f"Failed to start packet forwarding: \n{start_cmd_output}")
                 raise InteractiveCommandExecutionError("Testpmd failed to start packet forwarding.")
 
-            for port_id in range(self.number_of_ports):
+            for port_id in range(self.state.number_of_ports):
                 if not self.wait_link_status_up(port_id):
                     raise InteractiveCommandExecutionError(
                         "Not all ports came up after starting packet forwarding in testpmd."
                     )
 
+        self.state.packet_forwarding_started = True
+
     def stop(self, verify: bool = True) -> None:
         """Stop packet forwarding.
 
@@ -773,6 +789,8 @@ def stop(self, verify: bool = True) -> None:
                 self._logger.debug(f"Failed to stop packet forwarding: \n{stop_cmd_output}")
                 raise InteractiveCommandExecutionError("Testpmd failed to stop packet forwarding.")
 
+        self.state.packet_forwarding_started = False
+
     def get_devices(self) -> list[TestPmdDevice]:
         """Get a list of device names that are known to testpmd.
 
@@ -788,19 +806,16 @@ def get_devices(self) -> list[TestPmdDevice]:
                 dev_list.append(TestPmdDevice(line))
         return dev_list
 
-    def wait_link_status_up(self, port_id: int, timeout=SETTINGS.timeout) -> bool:
-        """Wait until the link status on the given port is "up".
+    def wait_link_status_up(self, port_id: int) -> bool:
+        """Wait until the link status on the given port is "up". Times out.
 
         Arguments:
             port_id: Port to check the link status on.
-            timeout: Time to wait for the link to come up. The default value for this
-                argument may be modified using the :option:`--timeout` command-line argument
-                or the :envvar:`DTS_TIMEOUT` environment variable.
 
         Returns:
             Whether the link came up in time or not.
         """
-        time_to_stop = time.time() + timeout
+        time_to_stop = time.time() + self._timeout
         port_info: str = ""
         while time.time() < time_to_stop:
             port_info = self.send_command(f"show port info {port_id}")
-- 
2.34.1

Re: [PATCH 1/6] dts: add parameters data structure

[Jeremy Spewock]

Overall I like the idea of having a structured way of passing
command-line arguments to applications as strings and I think that
this is a well-abstracted approach. I also like that this approach
still supports the ability to pass strings "as-is" and use them as
parameters as well. That opens the door for potentially creating
dataclasses which only detail key-parameters that we assume you will
use, without blocking you from inputting whatever you want.

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
<snip>
> +META_VALUE_ONLY = "value_only"
> +META_OPTIONS_END = "options_end"
> +META_SHORT_NAME = "short_name"
> +META_LONG_NAME = "long_name"
> +META_MULTIPLE = "multiple"
> +META_MIXINS = "mixins"
> +
> +

I might add some kind of block comment here as a separator that
delimits that metadata modifiers start here. Something like what is
written in scapy.py which creates sections for XML-RPC method vs ones
that are run by the docker container. This isn't something strictly
necessary, but it might help break things up and add a little more
explanation.

> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Injects the value of the attribute as-is without flag. Metadata modifier for :func:`dataclasses.field`."""
> +    return {**metadata, META_VALUE_ONLY: True}
> +
> +
<snip>

You could do the same thing here for mixins, but again, I'm not sure
it's really necessary.

> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Takes in a variable number of mixins to manipulate the value's rendering. Metadata modifier for :func:`dataclasses.field`.
> +
> +    The ``metadata`` keyword argument can be used to chain metadata modifiers together.
> +
> +    Mixins can be chained together, executed from right to left in the arguments list order.
> +
> +    Example:
> +
> +    .. code:: python
> +
> +        hex_bitmask: int | None = field(default=0b1101, metadata=field_mixins(hex, metadata=param_name("mask")))
> +
> +    will render as ``--mask=0xd``. The :func:`hex` built-in can be used as a mixin turning a valid integer into a hexadecimal representation.
> +    """
> +    return {**metadata, META_MIXINS: mixins}
<snip>
> 2.34.1
>

Re: [PATCH 2/6] dts: use Params for interactive shells

[Jeremy Spewock]

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Make it so that interactive shells accept an implementation of `Params`
> for app arguments. Convert EalParameters to use `Params` instead.
>
> String command line parameters can still be supplied by using the
> `StrParams` implementation.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
<snip>
> @@ -40,7 +42,7 @@ class InteractiveShell(ABC):
>      _ssh_channel: Channel
>      _logger: DTSLogger
>      _timeout: float
> -    _app_args: str
> +    _app_args: Params | None
>

I'm not sure if allowing None should be the solution for these shells
as opposed to just supplying an empty parameter object. Maybe
something that could be done is the factory method in sut_node allows
it to be None, but when it comes time to make the abstract shell it
creates an empty one if it doesn't exist, or the init method makes
this an optional parameter but creates it if it doesn't exist.

I suppose this logic would have to exist somewhere because the
parameters aren't always required, it's just a question of where we
should put it and if we should just assume that the interactive shell
class just knows how to accept some parameters and put them into the
shell. I would maybe leave this as something that cannot be None and
handle it outside of the shell, but I'm not sure it's something really
required and I don't have a super strong opinion on it.

>      #: Prompt to expect at the end of output when sending a command.
>      #: This is often overridden by subclasses.
<snip>
> @@ -118,8 +119,15 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>          Also find the number of pci addresses which were allowed on the command line when the app
>          was started.
>          """
> -        self._app_args += " -i --mask-event intr_lsc"
> -        self.number_of_ports = self._app_args.count("-a ")
> +        from framework.testbed_model.sut_node import EalParameters
> +
> +        assert isinstance(self._app_args, EalParameters)
> +
> +        if isinstance(self._app_args.app_params, StrParams):
> +            self._app_args.app_params.value += " -i --mask-event intr_lsc"
> +
> +        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0

I we should override the _app_args parameter in the testpmd shell to
always be EalParameters instead of doing this import and assertion.
It's a DPDK app, so we will always need EalParameters anyway, so we
might as well put that as our typehint to start off as what we expect
instead.

The checking of an instance of StrParams also feels a little strange
here, it might be more ideal if we could just add the parameters
without this check. Maybe something we can do, just because these
parameters are meant to be CLI commands anyway and will be rendered as
a string, is replace the StrParams object with a method on the base
Params dataclass that allows you to just add any string as a value
only field. Then, we don't have to bother validating anything about
the app parameters and we don't care what they are, we can just add a
string to them of new parameters.

I think this is something that likely also gets solved when you
replace this with testpmd parameters, but it might be a good way to
make the Params object more versatile in general so that people can
diverge and add their own flags to it if needed.

> +
>          super()._start_application(get_privileged_command)
>
>      def start(self, verify: bool = True) -> None:
 <snip>
> @@ -134,7 +136,7 @@ def create_interactive_shell(
>          shell_cls: Type[InteractiveShellType],
>          timeout: float,
>          privileged: bool,
> -        app_args: str,
> +        app_args: Params | None,

This also falls in line with what I was saying before about where this
logic is handled. This was made to always be a string originally
because by this point it being optional was already handled by the
sut_node.create_interactive_shell() and we should have some kind of
value here (even if that value is an empty parameters dataclass) to
pass into the application. It sort of semantically doesn't really
change much, but at this point it might as well not be None, so we can
simplify this type.

>      ) -> InteractiveShellType:
>          """Factory for interactive session handlers.
>
<snip>
> +@dataclass(kw_only=True)
> +class EalParameters(Params):
>      """The environment abstraction layer parameters.
>
>      The string representation can be created by converting the instance to a string.
>      """
>
> -    def __init__(
> -        self,
> -        lcore_list: LogicalCoreList,
> -        memory_channels: int,
> -        prefix: str,
> -        no_pci: bool,
> -        vdevs: list[VirtualDevice],
> -        ports: list[Port],
> -        other_eal_param: str,
> -    ):
> -        """Initialize the parameters according to inputs.
> -
> -        Process the parameters into the format used on the command line.
> +    lcore_list: LogicalCoreList = field(metadata=params.short("l"))
> +    """The list of logical cores to use."""
>
> -        Args:
> -            lcore_list: The list of logical cores to use.
> -            memory_channels: The number of memory channels to use.
> -            prefix: Set the file prefix string with which to start DPDK, e.g.: ``prefix='vf'``.
> -            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
> -            vdevs: Virtual devices, e.g.::
> +    memory_channels: int = field(metadata=params.short("n"))
> +    """The number of memory channels to use."""
>
> -                vdevs=[
> -                    VirtualDevice('net_ring0'),
> -                    VirtualDevice('net_ring1')
> -                ]
> -            ports: The list of ports to allow.
> -            other_eal_param: user defined DPDK EAL parameters, e.g.:
> -                ``other_eal_param='--single-file-segments'``
> -        """
> -        self._lcore_list = f"-l {lcore_list}"
> -        self._memory_channels = f"-n {memory_channels}"
> -        self._prefix = prefix
> -        if prefix:
> -            self._prefix = f"--file-prefix={prefix}"
> -        self._no_pci = "--no-pci" if no_pci else ""
> -        self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
> -        self._ports = " ".join(f"-a {port.pci}" for port in ports)
> -        self._other_eal_param = other_eal_param
> -
> -    def __str__(self) -> str:
> -        """Create the EAL string."""
> -        return (
> -            f"{self._lcore_list} "
> -            f"{self._memory_channels} "
> -            f"{self._prefix} "
> -            f"{self._no_pci} "
> -            f"{self._vdevs} "
> -            f"{self._ports} "
> -            f"{self._other_eal_param}"
> -        )
> +    prefix: str = field(metadata=params.long("file-prefix"))
> +    """Set the file prefix string with which to start DPDK, e.g.: ``prefix="vf"``."""

Just a small note on docstrings, I believe generally in DTS we use the
google docstring
(https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings)
format where it applies with some small differences. Because these
attributes aren't class vars however, I believe they should be in the
docstring for the class in the `Attributes` section. I generally have
trouble remembering exactly how it should look, but Juraj documented
it in `doc/guides/tools/dts.rst`

> +
> +    no_pci: params.Option
> +    """Switch to disable PCI bus e.g.: ``no_pci=True``."""
<snip>

> --
> 2.34.1
>

Re: [PATCH 3/6] dts: add testpmd shell params

[Jeremy Spewock]

We talked about this in DTS meeting, looking at this some more, we
already use default parameters for Eal and structure those, so we
already have sort of tied ourselves into a situation of if those ever
change (unlikely) we would need to change as well, so maybe this could
be something we use, I'd like to hear more of peoples thoughts on this
and what Juraj thinks when he is back.

Just because this is fairly large and bloats the testpmd file a little
bit, it might be more worth it to move this into a separate file and
import it so this file doesn't get too large. Especially because this
file will likely already grow quite a bit just from the amount of
testpmd commands we are going to have to handle in the future.

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Implement all the testpmd shell parameters into a data structure.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
<snip>
> 2.34.1
>

Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

[Jeremy Spewock]

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
<snip>
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index a2c7b30d9f..5d80061e8d 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
>      _stdout: channel.ChannelFile
>      _ssh_channel: Channel
>      _logger: DTSLogger
> +    __default_timeout: float

Only single underscores are used for other private variables, probably
better to keep that consistent with this one.

>      _timeout: float
>      _app_args: Params | None
> +    _is_privileged: bool = False
<snip>
> 2.34.1
>

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Jeremy Spewock]

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This commit provides a state container for TestPmdShell. It currently
> only indicates whether the packet forwarding has started
> or not, and the number of ports which were given to the shell.
>
> This also fixes the behaviour of `wait_link_status_up` to use the
> command timeout as inherited from InteractiveShell.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
<snip>
> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>          if self._app_args.app_params is None:
>              self._app_args.app_params = TestPmdParameters()
>
> -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
> +        assert isinstance(self._app_args.app_params, TestPmdParameters)
> +

This is tricky because ideally we wouldn't have the assertion here,
but I understand why it is needed because Eal parameters have app args
which can be any instance of params. I'm not sure of the best way to
solve this, because making testpmd parameters extend from eal would
break the general scheme that you have in place, and having an
extension of EalParameters that enforces this app_args is
TestPmdParameters would solve the issues, but might be a little
clunky. Is there a way we can use a generic to get python to just
understand that, in this case, this will always be TestPmdParameters?
If not I might prefer making a private class where this is
TestPmdParameters, just because there aren't really any other
assertions that we use elsewhere and an unexpected exception from this
(even though I don't think that can happen) could cause people some
issues.

It might be the case that an assertion is the easiest way to deal with
it though, what do you think?

> +        if self._app_args.app_params.auto_start:
> +            self.state.packet_forwarding_started = True
> +
> +        if self._app_args.ports is not None:
> +            self.state.number_of_ports = len(self._app_args.ports)
>
>          super()._start_application(get_privileged_command)
>
<snip>
> 2.34.1
>

Re: [PATCH 1/6] dts: add parameters data structure

[Juraj Linkeš]

On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This commit introduces a new "params" module, which adds a new way
> to manage command line parameters. The provided Params dataclass
> is able to read the fields of its child class and produce a string
> representation to supply to the command line. Any data structure
> that is intended to represent command line parameters can inherit
> it. The representation can then manipulated by using the dataclass
> field metadata in conjunction with the provided functions:
>
> * value_only, used to supply a value without forming an option/flag
> * options_end, used to prefix with an options end delimiter (`--`)
> * short, used to define a short parameter name, e.g. `-p`
> * long, used to define a long parameter name, e.g. `--parameter`
> * multiple, used to turn a list into repeating parameters
> * field_mixins, used to manipulate the string representation of the
>   value

We shouldn't list what the patch does, but rather explain the choices
made within. It seems to me the patch is here to give devs an API
instead of them having to construct strings - explaining why this
approach improves the old one should be in the commit message.

>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  dts/framework/params.py | 232 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 232 insertions(+)
>  create mode 100644 dts/framework/params.py
>
> diff --git a/dts/framework/params.py b/dts/framework/params.py
> new file mode 100644
> index 0000000000..6b48c8353e
> --- /dev/null
> +++ b/dts/framework/params.py
> @@ -0,0 +1,232 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 Arm Limited
> +
> +"""Parameter manipulation module.
> +
> +This module provides :class:`~Params` which can be used to model any data structure
> +that is meant to represent any command parameters.
> +"""
> +
> +from dataclasses import dataclass, field, fields
> +from typing import Any, Callable, Literal, Reversible, TypeVar, Iterable
> +from enum import Flag
> +
> +
> +T = TypeVar("T")
> +#: Type for a Mixin.
> +Mixin = Callable[[Any], str]
> +#: Type for an option parameter.
> +Option = Literal[True, None]
> +#: Type for a yes/no option parameter.
> +BooleanOption = Literal[True, False, None]
> +
> +META_VALUE_ONLY = "value_only"
> +META_OPTIONS_END = "options_end"
> +META_SHORT_NAME = "short_name"
> +META_LONG_NAME = "long_name"
> +META_MULTIPLE = "multiple"
> +META_MIXINS = "mixins"

These are only used in the Params class (but not set), so I'd either
move them there or make them private.

> +
> +
> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Injects the value of the attribute as-is without flag. Metadata modifier for :func:`dataclasses.field`."""
> +    return {**metadata, META_VALUE_ONLY: True}

These methods, on the other hand, are used outside this module, so it
makes sense to have them outside Params. It could be better to have
them inside as static methods, as they're closely related. Looking at
how they're used, we should unite the imports:
1. In testpmd_shell, they're imported directly (from framework.params
import long)
2. In sut_node, just the params module is imported (from framework
import params and then accessed via it: metadata=params.short("l"))

If we move these to Params, we could import Params and use them
similarly to 2. Not sure which one is better.

> +
> +
> +def short(name: str, metadata: dict[str, Any] = {}) -> dict[str, Any]:

It seems to me that having the metadata argument doesn't do anything
in these basic functions.

> +    """Overrides any parameter name with the given short option. Metadata modifier for :func:`dataclasses.field`.
> +
> +    .. code:: python
> +
> +        logical_cores: str | None = field(default="1-4", metadata=short("l"))
> +
> +    will render as ``-l=1-4`` instead of ``--logical-cores=1-4``.
> +    """
> +    return {**metadata, META_SHORT_NAME: name}
> +
> +
> +def long(name: str, metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Overrides the inferred parameter name to the specified one. Metadata modifier for :func:`dataclasses.field`.
> +
> +    .. code:: python
> +
> +        x_name: str | None = field(default="y", metadata=long("x"))
> +
> +    will render as ``--x=y``, but the field is accessed and modified through ``x_name``.
> +    """
> +    return {**metadata, META_LONG_NAME: name}
> +
> +
> +def options_end(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Precedes the value with an options end delimiter (``--``). Metadata modifier for :func:`dataclasses.field`."""
> +    return {**metadata, META_OPTIONS_END: True}
> +
> +
> +def multiple(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Specifies that this parameter is set multiple times. Must be a list. Metadata modifier for :func:`dataclasses.field`.
> +
> +    .. code:: python
> +
> +        ports: list[int] | None = field(default_factory=lambda: [0, 1, 2], metadata=multiple(param_name("port")))
> +
> +    will render as ``--port=0 --port=1 --port=2``. Note that modifiers can be chained like in this example.
> +    """
> +    return {**metadata, META_MULTIPLE: True}
> +
> +
> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:

Any reason why mixins are plural? I've only seen this used with one
argument, do you anticipate we'd need to use more than one? We could
make this singular and if we ever need to do two things, we could just
pass a function that does those two things (or calls two different
functions). Also, I'd just rename the mixin the func or something like
that.

The default of an argument should not be mutable, here's a quick
explanation: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

I don't really like the name. The positional arguments are applied to
the value and that should be reflected in the name - I'd like to see
something like convert in the name.

> +    """Takes in a variable number of mixins to manipulate the value's rendering. Metadata modifier for :func:`dataclasses.field`.
> +
> +    The ``metadata`` keyword argument can be used to chain metadata modifiers together.
> +
> +    Mixins can be chained together, executed from right to left in the arguments list order.
> +
> +    Example:
> +
> +    .. code:: python
> +
> +        hex_bitmask: int | None = field(default=0b1101, metadata=field_mixins(hex, metadata=param_name("mask")))
> +
> +    will render as ``--mask=0xd``. The :func:`hex` built-in can be used as a mixin turning a valid integer into a hexadecimal representation.
> +    """
> +    return {**metadata, META_MIXINS: mixins}

metadata | {META_MIXINS: mixins} also creates a new dictionary with
values from both and I think that would be more readable (since it's
mentioned in docs:
https://docs.python.org/3/library/stdtypes.html#mapping-types-dict).

> +
> +
> +def _reduce_mixins(mixins: Reversible[Mixin], value: Any) -> str:
> +    for mixin in reversed(mixins):
> +        value = mixin(value)
> +    return value
> +
> +
> +def str_mixins(*mixins: Mixin):

Again the name, this doesn't strike me as a decorator name. A
decorator modifies the thing it's decorating so it should contain a
verb describing what it's doing.

Maybe we could also do singular mixin here, as I described above. It
may result in cleaner API.

> +    """Decorator which modifies the ``__str__`` method, enabling support for mixins.
> +
> +    Mixins can be chained together, executed from right to left in the arguments list order.
> +
> +    Example:
> +
> +    .. code:: python
> +
> +        @str_mixins(hex_from_flag_value)
> +        class BitMask(enum.Flag):
> +            A = auto()
> +            B = auto()
> +
> +    will allow ``BitMask`` to render as a hexadecimal value.
> +    """
> +
> +    def _class_decorator(original_class):
> +        original_class.__str__ = lambda self: _reduce_mixins(mixins, self)
> +        return original_class
> +
> +    return _class_decorator
> +
> +
> +def comma_separated(values: Iterable[T]) -> str:
> +    """Mixin which renders an iterable in a comma-separated string."""
> +    return ",".join([str(value).strip() for value in values if value is not None])
> +
> +
> +def bracketed(value: str) -> str:
> +    """Mixin which adds round brackets to the input."""
> +    return f"({value})"
> +
> +
> +def str_from_flag_value(flag: Flag) -> str:
> +    """Mixin which returns the value from a :class:`enum.Flag` as a string."""
> +    return str(flag.value)
> +
> +
> +def hex_from_flag_value(flag: Flag) -> str:
> +    """Mixin which turns a :class:`enum.Flag` value into hexadecimal."""
> +    return hex(flag.value)
> +
> +
> +def _make_option(param_name: str, short: bool = False, no: bool = False) -> str:
> +    param_name = param_name.replace("_", "-")
> +    return f"{'-' if short else '--'}{'no-' if no else ''}{param_name}"
> +
> +
> +@dataclass
> +class Params:
> +    """Helper abstract dataclass that renders its fields into command line arguments.

Let's make the abstract class explicitly abstract with
https://docs.python.org/3/library/abc.html#abc.ABC. It won't be a full
abstract class since it won't have any abstract method or properties,
but I think it'll be better this way.

> +
> +    The parameter name is taken from the field name by default. The following:
> +
> +    .. code:: python
> +
> +        name: str | None = "value"
> +
> +    is rendered as ``--name=value``.
> +    Through :func:`dataclasses.field` the resulting parameter can be manipulated by applying
> +    appropriate metadata. This class can be used with the following metadata modifiers:
> +
> +    * :func:`value_only`
> +    * :func:`options_end`
> +    * :func:`short`
> +    * :func:`long`
> +    * :func:`multiple`
> +    * :func:`field_mixins`
> +
> +    To use fields as option switches set the value to ``True`` to enable them. If you

There should be a comma between switches and set.

> +    use a yes/no option switch you can also set ``False`` which would enable an option
> +    prefixed with ``--no-``. Examples:
> +
> +    .. code:: python
> +
> +        interactive: Option = True  # renders --interactive
> +        numa: BooleanOption = False # renders --no-numa

I'm wondering whether these could be simplified, but it's pretty good
this way. I'd change the names though:
Option -> Switch
BooleanOption -> NoSwitch (or YesNoSwitch? NegativeSwitch? Not sure
about this one)

All options (short, long, etc.) are options so that's where it's
confusing. The BooleanOption doesn't really capture what we're doing
with it (prepending no-) so I want a name that captures it.

There could also be some confusion with this being different from the
rest of the options API. This only requires us to set the type, the
rest must be passed in dataclasses.field. It's probably not that big
of a deal though.

> +
> +    Setting ``None`` will disable any option.

I'd reword this to "Setting an option to ``None`` will prevent it from
being rendered." or something similar. Disabling an option is kinda
ambiguous.

The :attr:`~Option` type alias is provided for
> +    regular option switches, whereas :attr:`~BooleanOption` is offered for yes/no ones.
> +
> +    An instance of a dataclass inheriting ``Params`` can also be assigned to an attribute, this helps with grouping parameters
> +    together. The attribute holding the dataclass will be ignored and the latter will just be rendered as expected.
> +    """
> +
> +    def __str__(self) -> str:
> +        arguments: list[str] = []
> +
> +        for field in fields(self):
> +            value = getattr(self, field.name)
> +
> +            if value is None:
> +                continue
> +
> +            options_end = field.metadata.get(META_OPTIONS_END, False)
> +            if options_end:
> +                arguments.append("--")

This is a confusing way to separate the options. The separator itself
is an option (I'd rename it to sep or separator instead of end), so
I'd make a separate field for it. We could pass init=False to field()
in the field definition, but that relies on fields being ordered, so
we'd need to also pass order=True to the dataclass constructor.

> +
> +            value_only = field.metadata.get(META_VALUE_ONLY, False)
> +            if isinstance(value, Params) or value_only or options_end:
> +                arguments.append(str(value))
> +                continue
> +
> +            # take "short_name" metadata, or "long_name" metadata, or infer from field name
> +            option_name = field.metadata.get(
> +                META_SHORT_NAME, field.metadata.get(META_LONG_NAME, field.name)
> +            )
> +            is_short = META_SHORT_NAME in field.metadata
> +
> +            if isinstance(value, bool):
> +                arguments.append(_make_option(option_name, short=is_short, no=(not value)))
> +                continue
> +
> +            option = _make_option(option_name, short=is_short)
> +            separator = " " if is_short else "="
> +            str_mixins = field.metadata.get(META_MIXINS, [])
> +            multiple = field.metadata.get(META_MULTIPLE, False)
> +
> +            values = value if multiple else [value]
> +            for entry_value in values:
> +                entry_value = _reduce_mixins(str_mixins, entry_value)
> +                arguments.append(f"{option}{separator}{entry_value}")
> +
> +        return " ".join(arguments)
> +
> +
> +@dataclass
> +class StrParams(Params):
> +    """A drop-in replacement for parameters passed as a string."""
> +
> +    value: str = field(metadata=value_only())
> --
> 2.34.1
>

Re: [PATCH 2/6] dts: use Params for interactive shells

[Juraj Linkeš]

On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >
> > Make it so that interactive shells accept an implementation of `Params`
> > for app arguments. Convert EalParameters to use `Params` instead.
> >
> > String command line parameters can still be supplied by using the
> > `StrParams` implementation.
> >
> > Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> > Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> <snip>
> > @@ -40,7 +42,7 @@ class InteractiveShell(ABC):
> >      _ssh_channel: Channel
> >      _logger: DTSLogger
> >      _timeout: float
> > -    _app_args: str
> > +    _app_args: Params | None
> >
>
> I'm not sure if allowing None should be the solution for these shells
> as opposed to just supplying an empty parameter object. Maybe
> something that could be done is the factory method in sut_node allows
> it to be None, but when it comes time to make the abstract shell it
> creates an empty one if it doesn't exist, or the init method makes
> this an optional parameter but creates it if it doesn't exist.
>
> I suppose this logic would have to exist somewhere because the
> parameters aren't always required, it's just a question of where we
> should put it and if we should just assume that the interactive shell
> class just knows how to accept some parameters and put them into the
> shell. I would maybe leave this as something that cannot be None and
> handle it outside of the shell, but I'm not sure it's something really
> required and I don't have a super strong opinion on it.
>

I think this is an excellent idea. An empty Params (or StrParams or
EalParams if we want to make Params truly abstract and thus not
instantiable) would work as the default value and it would be an
elegant solution since dev will only pass non-empty Params.

> >      #: Prompt to expect at the end of output when sending a command.
> >      #: This is often overridden by subclasses.
> <snip>
> > @@ -118,8 +119,15 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
> >          Also find the number of pci addresses which were allowed on the command line when the app
> >          was started.
> >          """
> > -        self._app_args += " -i --mask-event intr_lsc"
> > -        self.number_of_ports = self._app_args.count("-a ")
> > +        from framework.testbed_model.sut_node import EalParameters
> > +
> > +        assert isinstance(self._app_args, EalParameters)
> > +
> > +        if isinstance(self._app_args.app_params, StrParams):
> > +            self._app_args.app_params.value += " -i --mask-event intr_lsc"
> > +
> > +        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
>
> I we should override the _app_args parameter in the testpmd shell to
> always be EalParameters instead of doing this import and assertion.
> It's a DPDK app, so we will always need EalParameters anyway, so we
> might as well put that as our typehint to start off as what we expect
> instead.
>
> The checking of an instance of StrParams also feels a little strange
> here, it might be more ideal if we could just add the parameters
> without this check. Maybe something we can do, just because these
> parameters are meant to be CLI commands anyway and will be rendered as
> a string, is replace the StrParams object with a method on the base
> Params dataclass that allows you to just add any string as a value
> only field. Then, we don't have to bother validating anything about
> the app parameters and we don't care what they are, we can just add a
> string to them of new parameters.
>
> I think this is something that likely also gets solved when you
> replace this with testpmd parameters, but it might be a good way to
> make the Params object more versatile in general so that people can
> diverge and add their own flags to it if needed.
>

I'd say this is done because of circular imports. If so, we could move
EalParameters to params.py, that should help. And when we're at it,
either rename it to EalParams or rename the other classes to use the
whole word.

> > +
> >          super()._start_application(get_privileged_command)
> >
> >      def start(self, verify: bool = True) -> None:
>  <snip>
> > @@ -134,7 +136,7 @@ def create_interactive_shell(
> >          shell_cls: Type[InteractiveShellType],
> >          timeout: float,
> >          privileged: bool,
> > -        app_args: str,
> > +        app_args: Params | None,
>
> This also falls in line with what I was saying before about where this
> logic is handled. This was made to always be a string originally
> because by this point it being optional was already handled by the
> sut_node.create_interactive_shell() and we should have some kind of
> value here (even if that value is an empty parameters dataclass) to
> pass into the application. It sort of semantically doesn't really
> change much, but at this point it might as well not be None, so we can
> simplify this type.
>
> >      ) -> InteractiveShellType:
> >          """Factory for interactive session handlers.
> >
> <snip>
> > +@dataclass(kw_only=True)
> > +class EalParameters(Params):
> >      """The environment abstraction layer parameters.
> >
> >      The string representation can be created by converting the instance to a string.
> >      """
> >
> > -    def __init__(
> > -        self,
> > -        lcore_list: LogicalCoreList,
> > -        memory_channels: int,
> > -        prefix: str,
> > -        no_pci: bool,
> > -        vdevs: list[VirtualDevice],
> > -        ports: list[Port],
> > -        other_eal_param: str,
> > -    ):
> > -        """Initialize the parameters according to inputs.
> > -
> > -        Process the parameters into the format used on the command line.
> > +    lcore_list: LogicalCoreList = field(metadata=params.short("l"))
> > +    """The list of logical cores to use."""
> >
> > -        Args:
> > -            lcore_list: The list of logical cores to use.
> > -            memory_channels: The number of memory channels to use.
> > -            prefix: Set the file prefix string with which to start DPDK, e.g.: ``prefix='vf'``.
> > -            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
> > -            vdevs: Virtual devices, e.g.::
> > +    memory_channels: int = field(metadata=params.short("n"))
> > +    """The number of memory channels to use."""
> >
> > -                vdevs=[
> > -                    VirtualDevice('net_ring0'),
> > -                    VirtualDevice('net_ring1')
> > -                ]
> > -            ports: The list of ports to allow.
> > -            other_eal_param: user defined DPDK EAL parameters, e.g.:
> > -                ``other_eal_param='--single-file-segments'``
> > -        """
> > -        self._lcore_list = f"-l {lcore_list}"
> > -        self._memory_channels = f"-n {memory_channels}"
> > -        self._prefix = prefix
> > -        if prefix:
> > -            self._prefix = f"--file-prefix={prefix}"
> > -        self._no_pci = "--no-pci" if no_pci else ""
> > -        self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
> > -        self._ports = " ".join(f"-a {port.pci}" for port in ports)
> > -        self._other_eal_param = other_eal_param
> > -
> > -    def __str__(self) -> str:
> > -        """Create the EAL string."""
> > -        return (
> > -            f"{self._lcore_list} "
> > -            f"{self._memory_channels} "
> > -            f"{self._prefix} "
> > -            f"{self._no_pci} "
> > -            f"{self._vdevs} "
> > -            f"{self._ports} "
> > -            f"{self._other_eal_param}"
> > -        )
> > +    prefix: str = field(metadata=params.long("file-prefix"))
> > +    """Set the file prefix string with which to start DPDK, e.g.: ``prefix="vf"``."""
>
> Just a small note on docstrings, I believe generally in DTS we use the
> google docstring
> (https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings)
> format where it applies with some small differences. Because these
> attributes aren't class vars however, I believe they should be in the
> docstring for the class in the `Attributes` section. I generally have
> trouble remembering exactly how it should look, but Juraj documented
> it in `doc/guides/tools/dts.rst`
>

I noticed this right away. Here's the bullet point that applies:

* The ``dataclass.dataclass`` decorator changes how the attributes are
processed.
  The dataclass attributes which result in instance variables/attributes
  should also be recorded in the ``Attributes:`` section.

> > +
> > +    no_pci: params.Option
> > +    """Switch to disable PCI bus e.g.: ``no_pci=True``."""
> <snip>
>
> > --
> > 2.34.1
> >

Re: [PATCH 1/6] dts: add parameters data structure

[Luca Vizzarro]

Thank you for your review Jeremy!

On 28/03/2024 16:48, Jeremy Spewock wrote:
> I might add some kind of block comment here as a separator that
> delimits that metadata modifiers start here. Something like what is
> written in scapy.py which creates sections for XML-RPC method vs ones
> that are run by the docker container. This isn't something strictly
> necessary, but it might help break things up and add a little more
> explanation.
<snip>
> You could do the same thing here for mixins, but again, I'm not sure
> it's really necessary.

Yes, I agree that using block comments to delimit sections is a good 
idea. I wasn't sure if we had an established way of doing this, and 
looks like we do!

Re: [PATCH 1/6] dts: add parameters data structure

[Luca Vizzarro]

Thank you so much for your review Juraj!

On 09/04/2024 13:10, Juraj Linkeš wrote:
> We shouldn't list what the patch does, but rather explain the choices
> made within. It seems to me the patch is here to give devs an API
> instead of them having to construct strings - explaining why this
> approach improves the old one should be in the commit message.
>

Ack.
>> +META_VALUE_ONLY = "value_only"
>> +META_OPTIONS_END = "options_end"
>> +META_SHORT_NAME = "short_name"
>> +META_LONG_NAME = "long_name"
>> +META_MULTIPLE = "multiple"
>> +META_MIXINS = "mixins"
> 
> These are only used in the Params class (but not set), so I'd either
> move them there or make them private.
>

Ack.

>> +
>> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
>> +    """Injects the value of the attribute as-is without flag. Metadata modifier for :func:`dataclasses.field`."""
>> +    return {**metadata, META_VALUE_ONLY: True}
> 
> These methods, on the other hand, are used outside this module, so it
> makes sense to have them outside Params. It could be better to have
> them inside as static methods, as they're closely related. Looking at
> how they're used, we should unite the imports:
> 1. In testpmd_shell, they're imported directly (from framework.params
> import long)
> 2. In sut_node, just the params module is imported (from framework
> import params and then accessed via it: metadata=params.short("l"))
> 
Having a shorter version may look less verbose. I agree that we can make 
everything a static method of Params, but then having to do Params.short 
etc everytime will make it look more verbose. So what option do we 
prefer? The functions do belong to the params module nonetheless, and 
therefore are meant to be used in conjunction with the Params class.

> If we move these to Params, we could import Params and use them
> similarly to 2. Not sure which one is better.
> 


>> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:
> 
> Any reason why mixins are plural? I've only seen this used with one
> argument, do you anticipate we'd need to use more than one? We could
> make this singular and if we ever need to do two things, we could just
> pass a function that does those two things (or calls two different
> functions). Also, I'd just rename the mixin the func or something like
> that.
> 
> The default of an argument should not be mutable, here's a quick
> explanation: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments


Indeed the reason for which I create dictionaries, as I am treating them 
as read only. I wanted to avoid to bloat the code with lots of `is None` 
checks. But we can sacrifice this optimization for better code.

> I don't really like the name. The positional arguments are applied to
> the value and that should be reflected in the name - I'd like to see
> something like convert in the name. 

What this does is effectively just add the mixins to dataclass.field 
metadata, hence "field"_mixins. This is meant to represent a chain of 
mixins, in my original code this appeared more often. Not so much in 
this post, as I made more optimisations. Which takes me to the plural 
bit, for testpmd specifically this plurality appears only when 
decorating another class using `str_mixins`, see TestPmdRingNUMAConfig. 
So for consistency I kept both to ingest a chain of "mixins", as maybe 
it'll be needed in the future.

Are you suggesting to just make the name as singular? But still take 
multiple function pointers? The term "mixin" specifically just means a 
middleware that manipulates the output value when using __str__. Here we 
are just chaining them for reusability. Do you have any better name in mind?

>> +    return {**metadata, META_MIXINS: mixins}
> 
> metadata | {META_MIXINS: mixins} also creates a new dictionary with
> values from both and I think that would be more readable (since it's
> mentioned in docs:
> https://docs.python.org/3/library/stdtypes.html#mapping-types-dict).

If we were to use `None` as default to the arguments, then this would no 
longer be needed. But great shout, wasn't aware of this feature added in 
3.9.

>> +def _reduce_mixins(mixins: Reversible[Mixin], value: Any) -> str:
>> +    for mixin in reversed(mixins):
>> +        value = mixin(value)
>> +    return value
>> +
>> +
>> +def str_mixins(*mixins: Mixin):
> 
> Again the name, this doesn't strike me as a decorator name. A
> decorator modifies the thing it's decorating so it should contain a
> verb describing what it's doing.
> 
> Maybe we could also do singular mixin here, as I described above. It
> may result in cleaner API.

Great point. Yes making the function name sound like a verb is 
definitely the right way. Thank you for pointing it out.

>> +@dataclass
>> +class Params:
>> +    """Helper abstract dataclass that renders its fields into command line arguments.
> 
> Let's make the abstract class explicitly abstract with
> https://docs.python.org/3/library/abc.html#abc.ABC. It won't be a full
> abstract class since it won't have any abstract method or properties,
> but I think it'll be better this way.

No problem. I'm not sure if applying ABC to a dataclass may complicate 
things. But can definitely do.

>> +    To use fields as option switches set the value to ``True`` to enable them. If you
> 
> There should be a comma between switches and set.

Ack.

>> +    use a yes/no option switch you can also set ``False`` which would enable an option
>> +    prefixed with ``--no-``. Examples:
>> +
>> +    .. code:: python
>> +
>> +        interactive: Option = True  # renders --interactive
>> +        numa: BooleanOption = False # renders --no-numa
> 
> I'm wondering whether these could be simplified, but it's pretty good
> this way. I'd change the names though:
> Option -> Switch
> BooleanOption -> NoSwitch (or YesNoSwitch? NegativeSwitch? Not sure
> about this one)
> 
> All options (short, long, etc.) are options so that's where it's
> confusing. The BooleanOption doesn't really capture what we're doing
> with it (prepending no-) so I want a name that captures it.
> 
> There could also be some confusion with this being different from the
> rest of the options API. This only requires us to set the type, the
> rest must be passed in dataclasses.field. It's probably not that big
> of a deal though.
> 

I am glad you are bringing this up. I am also perplexed on the choice of 
name here. I decided to use whatever libc getopts uses. But your 
suggestion sounds nice. Will use it.

>> +
>> +    Setting ``None`` will disable any option.
> 
> I'd reword this to "Setting an option to ``None`` will prevent it from
> being rendered." or something similar. Disabling an option is kinda
> ambiguous.
> 

Ack.

>> +            options_end = field.metadata.get(META_OPTIONS_END, False)
>> +            if options_end:
>> +                arguments.append("--")
> 
> This is a confusing way to separate the options. The separator itself
> is an option (I'd rename it to sep or separator instead of end), so
> I'd make a separate field for it. We could pass init=False to field()
> in the field definition, but that relies on fields being ordered, so
> we'd need to also pass order=True to the dataclass constructor.

Ack.

Re: [PATCH 3/6] dts: add testpmd shell params

[Juraj Linkeš]

As Jeremy pointed out, going forward, this is likely to become bloated
and moving it to params.py (for example) may be better.

There's a lot of testpmd args here. I commented on the implementation
of some of them. I didn't verify that the actual values match the docs
or, god forbid, tested all of it. :-) Doing that as we start using
them is going to be good enough.

On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Implement all the testpmd shell parameters into a data structure.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 633 +++++++++++++++++-
>  1 file changed, 615 insertions(+), 18 deletions(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index db3abb7600..a823dc53be 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py

<snip>

> +@str_mixins(bracketed, comma_separated)
> +class TestPmdRingNUMAConfig(NamedTuple):
> +    """Tuple associating DPDK port, direction of the flow and NUMA socket."""

Is there any particular order for these various classes?

> +
> +    port: int
> +    direction: TestPmdFlowDirection
> +    socket: int
> +
> +

<snip>

> +@dataclass(kw_only=True)
> +class TestPmdTXOnlyForwardingMode(Params):

The three special forwarding modes should really be moved right after
TestPmdForwardingModes. Do we actually need these three in
TestPmdForwardingModes? Looks like we could just remove those from
TestPmdForwardingModes since they have to be passed separately, not as
that Enum.

> +    __forward_mode: Literal[TestPmdForwardingModes.txonly] = field(
> +        default=TestPmdForwardingModes.txonly, init=False, metadata=long("forward-mode")
> +    )

I guess this is here so that "--forward-mode=txonly" gets rendered,
right? Why the two underscored? Is that because we want to hammer home
the fact that this is init=False, a kind of internal field? I'd like
to make it like the other fields, without any underscores (or maybe
just one underscore), and documented (definitely documented).
If we remove txonly from the Enum, we could just have the string value
here. The Enums are mostly useful to give users the proper range of
values.

> +    multi_flow: Option = field(default=None, metadata=long("txonly-multi-flow"))
> +    """Generate multiple flows."""
> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
> +    """Set TX segment sizes or total packet length."""
> +
> +
> +@dataclass(kw_only=True)
> +class TestPmdFlowGenForwardingMode(Params):
> +    __forward_mode: Literal[TestPmdForwardingModes.flowgen] = field(
> +        default=TestPmdForwardingModes.flowgen, init=False, metadata=long("forward-mode")
> +    )
> +    clones: int | None = field(default=None, metadata=long("flowgen-clones"))
> +    """Set the number of each packet clones to be sent. Sending clones reduces host CPU load on
> +    creating packets and may help in testing extreme speeds or maxing out Tx packet performance.
> +    N should be not zero, but less than ‘burst’ parameter.
> +    """
> +    flows: int | None = field(default=None, metadata=long("flowgen-flows"))
> +    """Set the number of flows to be generated, where 1 <= N <= INT32_MAX."""
> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
> +    """Set TX segment sizes or total packet length."""
> +
> +
> +@dataclass(kw_only=True)
> +class TestPmdNoisyForwardingMode(Params):
> +    __forward_mode: Literal[TestPmdForwardingModes.noisy] = field(
> +        default=TestPmdForwardingModes.noisy, init=False, metadata=long("forward-mode")
> +    )

Are both of __forward_mode and forward_mode needed because we need to
render both?

> +    forward_mode: (
> +        Literal[
> +            TestPmdForwardingModes.io,
> +            TestPmdForwardingModes.mac,
> +            TestPmdForwardingModes.macswap,
> +            TestPmdForwardingModes.fivetswap,
> +        ]
> +        | None

Is there a difference between using union (TestPmdForwardingModes.io |
TestPmdForwardingModes.mac etc.) and Literal?

> +    ) = field(default=TestPmdForwardingModes.io, metadata=long("noisy-forward-mode"))
> +    """Set the noisy vnf forwarding mode."""
> +    tx_sw_buffer_size: int | None = field(default=None, metadata=long("noisy-tx-sw-buffer-size"))
> +    """Set the maximum number of elements of the FIFO queue to be created for buffering packets.
> +    The default value is 0.
> +    """
> +    tx_sw_buffer_flushtime: int | None = field(
> +        default=None, metadata=long("noisy-tx-sw-buffer-flushtime")
> +    )
> +    """Set the time before packets in the FIFO queue are flushed. The default value is 0."""
> +    lkup_memory: int | None = field(default=None, metadata=long("noisy-lkup-memory"))
> +    """Set the size of the noisy neighbor simulation memory buffer in MB to N. The default value is 0."""
> +    lkup_num_reads: int | None = field(default=None, metadata=long("noisy-lkup-num-reads"))
> +    """Set the number of reads to be done in noisy neighbor simulation memory buffer to N.
> +    The default value is 0.
> +    """
> +    lkup_num_writes: int | None = field(default=None, metadata=long("noisy-lkup-num-writes"))
> +    """Set the number of writes to be done in noisy neighbor simulation memory buffer to N.
> +    The default value is 0.
> +    """
> +    lkup_num_reads_writes: int | None = field(
> +        default=None, metadata=long("noisy-lkup-num-reads-writes")
> +    )
> +    """Set the number of r/w accesses to be done in noisy neighbor simulation memory buffer to N.
> +    The default value is 0.
> +    """

<snip>

> +@dataclass
> +class TestPmdDisableRSS(Params):
> +    """Disable RSS (Receive Side Scaling)."""

Let's put the explanation/reminder of what RSS stands for to either
all three classes or none of them.

> +
> +    __disable_rss: Literal[True] = field(default=True, init=False, metadata=long("disable-rss"))
> +
> +
> +@dataclass
> +class TestPmdSetRSSIPOnly(Params):
> +    """Set RSS functions for IPv4/IPv6 only."""
> +
> +    __rss_ip: Literal[True] = field(default=True, init=False, metadata=long("rss-ip"))
> +
> +
> +@dataclass
> +class TestPmdSetRSSUDP(Params):
> +    """Set RSS functions for IPv4/IPv6 and UDP."""
> +
> +    __rss_udp: Literal[True] = field(default=True, init=False, metadata=long("rss-udp"))
> +
> +

<snip>

> +    rss: TestPmdDisableRSS | TestPmdSetRSSIPOnly | TestPmdSetRSSUDP | None = None
> +    """RSS option setting.
> +
> +    The value can be one of:
> +    * :class:`TestPmdDisableRSS`, to disable RSS
> +    * :class:`TestPmdSetRSSIPOnly`, to set RSS for IPv4/IPv6 only
> +    * :class:`TestPmdSetRSSUDP`, to set RSS for IPv4/IPv6 and UDP
> +    """

Have you thought about making an Enum where values would be these
classes? That could simplify things a bit for users if it works.

> +
> +    forward_mode: (
> +        Literal[
> +            TestPmdForwardingModes.io,
> +            TestPmdForwardingModes.mac,
> +            TestPmdForwardingModes.macswap,
> +            TestPmdForwardingModes.rxonly,
> +            TestPmdForwardingModes.csum,
> +            TestPmdForwardingModes.icmpecho,
> +            TestPmdForwardingModes.ieee1588,
> +            TestPmdForwardingModes.fivetswap,
> +            TestPmdForwardingModes.shared_rxq,
> +            TestPmdForwardingModes.recycle_mbufs,
> +        ]

This could result in just TestPmdForwardingModes | the rest if we
remove the compound fw modes from TestPmdForwardingModes. Maybe we
could rename TestPmdForwardingModes to TestPmdSimpleForwardingModes or
something at that point.

> +        | TestPmdFlowGenForwardingMode
> +        | TestPmdTXOnlyForwardingMode
> +        | TestPmdNoisyForwardingMode
> +        | None
> +    ) = TestPmdForwardingModes.io
> +    """Set the forwarding mode.

<snip>

> +    mempool_allocation_mode: (
> +        Literal[
> +            TestPmdMempoolAllocationMode.native,
> +            TestPmdMempoolAllocationMode.xmem,
> +            TestPmdMempoolAllocationMode.xmemhuge,
> +        ]
> +        | TestPmdAnonMempoolAllocationMode
> +        | None

This looks similar to fw modes, maybe the same applies here as well.

> +    ) = field(default=None, metadata=long("mp-alloc"))
> +    """Select mempool allocation mode.
> +
> +    The value can be one of:
> +    * :attr:`TestPmdMempoolAllocationMode.native`
> +    * :class:`TestPmdAnonMempoolAllocationMode`
> +    * :attr:`TestPmdMempoolAllocationMode.xmem`
> +    * :attr:`TestPmdMempoolAllocationMode.xmemhuge`
> +    """

Re: [PATCH 4/6] dts: use testpmd params for scatter test suite

[Juraj Linkeš]

On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Update the buffer scatter test suite to use TestPmdParameters
> instead of the StrParams implementation.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  dts/tests/TestSuite_pmd_buffer_scatter.py | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 4cdbdc4272..c6d313fc64 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -23,7 +23,11 @@
>  from scapy.utils import hexstr  # type: ignore[import]
>
>  from framework.params import StrParams
> -from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
> +from framework.remote_session.testpmd_shell import (
> +    TestPmdForwardingModes,
> +    TestPmdShell,
> +    TestPmdParameters,
> +)
>  from framework.test_suite import TestSuite
>
>
> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
>          """
>          testpmd = self.sut_node.create_interactive_shell(
>              TestPmdShell,
> -            app_parameters=StrParams(
> -                "--mbcache=200 "
> -                f"--mbuf-size={mbsize} "
> -                "--max-pkt-len=9000 "
> -                "--port-topology=paired "
> -                "--tx-offloads=0x00008000"
> +            app_parameters=TestPmdParameters(
> +                forward_mode=TestPmdForwardingModes.mac,
> +                mbcache=200,
> +                mbuf_size=[mbsize],
> +                max_pkt_len=9000,
> +                tx_offloads=0x00008000,
>              ),
>              privileged=True,
>          )
> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)

Jeremy, does this change the test? Instead of configuring the fw mode
after starting testpmd, we're starting testpmd with fw mode
configured.

If not, we should remove the testpmd.set_forward_mode method, as it's
not used anymore.

>          testpmd.start()
>
>          for offset in [-1, 0, 1, 4, 5]:
> --
> 2.34.1
>

Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

[Juraj Linkeš]

I have a general question. What are these changes for? Do you
anticipate us needing this in the future? Wouldn't it be better to add
it only when we need it?

On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> <snip>
> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> > index a2c7b30d9f..5d80061e8d 100644
> > --- a/dts/framework/remote_session/interactive_shell.py
> > +++ b/dts/framework/remote_session/interactive_shell.py
> > @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
> >      _stdout: channel.ChannelFile
> >      _ssh_channel: Channel
> >      _logger: DTSLogger
> > +    __default_timeout: float
>
> Only single underscores are used for other private variables, probably
> better to keep that consistent with this one.
>

I agree, I don't see a reason for the double underscore.

> >      _timeout: float
> >      _app_args: Params | None
> > +    _is_privileged: bool = False
> <snip>
> > 2.34.1
> >

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Juraj Linkeš]

On Thu, Mar 28, 2024 at 5:49 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >
> > This commit provides a state container for TestPmdShell. It currently
> > only indicates whether the packet forwarding has started
> > or not, and the number of ports which were given to the shell.
> >
> > This also fixes the behaviour of `wait_link_status_up` to use the
> > command timeout as inherited from InteractiveShell.
> >
> > Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> > Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> <snip>
> > @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
> >          if self._app_args.app_params is None:
> >              self._app_args.app_params = TestPmdParameters()
> >
> > -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
> > +        assert isinstance(self._app_args.app_params, TestPmdParameters)
> > +
>
> This is tricky because ideally we wouldn't have the assertion here,
> but I understand why it is needed because Eal parameters have app args
> which can be any instance of params. I'm not sure of the best way to
> solve this, because making testpmd parameters extend from eal would
> break the general scheme that you have in place, and having an
> extension of EalParameters that enforces this app_args is
> TestPmdParameters would solve the issues, but might be a little
> clunky. Is there a way we can use a generic to get python to just
> understand that, in this case, this will always be TestPmdParameters?
> If not I might prefer making a private class where this is
> TestPmdParameters, just because there aren't really any other
> assertions that we use elsewhere and an unexpected exception from this
> (even though I don't think that can happen) could cause people some
> issues.
>
> It might be the case that an assertion is the easiest way to deal with
> it though, what do you think?
>

We could change the signature (just the type of app_args) of the init
method - I think we should be able to create a type that's
EalParameters with .app_params being TestPmdParameters or None. The
init method would just call super().

Something like the above is basically necessary with inheritance where
subclasses are all extensions (not just implementations) of the
superclass (having differences in API).

> > +        if self._app_args.app_params.auto_start:
> > +            self.state.packet_forwarding_started = True
> > +
> > +        if self._app_args.ports is not None:
> > +            self.state.number_of_ports = len(self._app_args.ports)
> >
> >          super()._start_application(get_privileged_command)
> >
> <snip>
> > 2.34.1
> >

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Juraj Linkeš]

On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This commit provides a state container for TestPmdShell. It currently
> only indicates whether the packet forwarding has started
> or not, and the number of ports which were given to the shell.
>

A reminder, the commit message should explain why we're doing this
change, not what the change is.

> This also fixes the behaviour of `wait_link_status_up` to use the
> command timeout as inherited from InteractiveShell.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 41 +++++++++++++------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index a823dc53be..ea1d254f86 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -678,19 +678,27 @@ def __str__(self) -> str:
>          return self.pci_address
>
>
> +@dataclass(slots=True)
> +class TestPmdState:
> +    """Session state container."""
> +
> +    #:
> +    packet_forwarding_started: bool = False

The same question as in the previous patch, do you anticipate this
being needed and should we add this only when it's actually used?

> +
> +    #: The number of ports which were allowed on the command-line when testpmd was started.
> +    number_of_ports: int = 0
> +
> +
>  class TestPmdShell(InteractiveShell):
>      """Testpmd interactive shell.
>
>      The testpmd shell users should never use
>      the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
>      call specialized methods. If there isn't one that satisfies a need, it should be added.
> -
> -    Attributes:
> -        number_of_ports: The number of ports which were allowed on the command-line when testpmd
> -            was started.
>      """
>
> -    number_of_ports: int
> +    #: Current state
> +    state: TestPmdState = TestPmdState()

Assigning a value makes this a class variable, shared across all
instances. This should be initialized in __init__().

But do we actually want to do this via composition? We'd need to
access the attributes via .state all the time and I don't really like
that. We could just put them into TestPmdShell directly, initializing
them in __init__().

>
>      #: The path to the testpmd executable.
>      path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")

Re: [PATCH 1/6] dts: add parameters data structure

[Juraj Linkeš]

On Tue, Apr 9, 2024 at 6:28 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Thank you so much for your review Juraj!
>

You're welcome!

> >> +
> >> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> >> +    """Injects the value of the attribute as-is without flag. Metadata modifier for :func:`dataclasses.field`."""
> >> +    return {**metadata, META_VALUE_ONLY: True}
> >
> > These methods, on the other hand, are used outside this module, so it
> > makes sense to have them outside Params. It could be better to have
> > them inside as static methods, as they're closely related. Looking at
> > how they're used, we should unite the imports:
> > 1. In testpmd_shell, they're imported directly (from framework.params
> > import long)
> > 2. In sut_node, just the params module is imported (from framework
> > import params and then accessed via it: metadata=params.short("l"))
> >
> Having a shorter version may look less verbose. I agree that we can make
> everything a static method of Params, but then having to do Params.short
> etc everytime will make it look more verbose. So what option do we
> prefer? The functions do belong to the params module nonetheless, and
> therefore are meant to be used in conjunction with the Params class.
>

When I first saw the code, I liked the usage in sut_node better, e.g.:
prefix: str = field(metadata=params.long("file-prefix")). I think this
is because it's obvious where the function comes from. I'd do the
longer version because I think most people are just going to glance at
the code when they want to know how to properly implement an argument
so the explicit nature could help with understanding how it should be
done.

> > If we move these to Params, we could import Params and use them
> > similarly to 2. Not sure which one is better.
> >
>
>
> >> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:
> >
> > Any reason why mixins are plural? I've only seen this used with one
> > argument, do you anticipate we'd need to use more than one? We could
> > make this singular and if we ever need to do two things, we could just
> > pass a function that does those two things (or calls two different
> > functions). Also, I'd just rename the mixin the func or something like
> > that.
> >
> > The default of an argument should not be mutable, here's a quick
> > explanation: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
>
>
> Indeed the reason for which I create dictionaries, as I am treating them
> as read only. I wanted to avoid to bloat the code with lots of `is None`
> checks. But we can sacrifice this optimization for better code.
>

This would be the only place where we'd do the check, as I don't think
we need the metadata argument in any of the other functions - those
seem to be mutually exclusive, but maybe they're not? In any case,
we'd need to fix this, I don't think treating them as read-only avoids
the problem.

> > I don't really like the name. The positional arguments are applied to
> > the value and that should be reflected in the name - I'd like to see
> > something like convert in the name.
>
> What this does is effectively just add the mixins to dataclass.field
> metadata, hence "field"_mixins. This is meant to represent a chain of
> mixins, in my original code this appeared more often. Not so much in
> this post, as I made more optimisations. Which takes me to the plural
> bit, for testpmd specifically this plurality appears only when
> decorating another class using `str_mixins`, see TestPmdRingNUMAConfig.
> So for consistency I kept both to ingest a chain of "mixins", as maybe
> it'll be needed in the future.
>
> Are you suggesting to just make the name as singular? But still take
> multiple function pointers?

Singular with one function, as that was what I saw being used. The one
function could do multiple things (or call multiple other functions)
if a need arises. The str_mixins could be used this way as well.

I don't know which one is better, maybe keeping the plural is fine.

> The term "mixin" specifically just means a
> middleware that manipulates the output value when using __str__.

Aren't all of the other functions mixins as well, at least in some
sense? They change the option, not the value, but could still be
thought of as mixins in some respect.

> Here we
> are just chaining them for reusability. Do you have any better name in mind?
>

I don't know, so let's brainstorm a bit. Let's start with the usage:
portmask: int | None = field(default=None, metadata=field_mixins(hex))

Here it's not clear at all why it's called field_mixins, at least
compared to the other functions which are not called mixins. I guess
the other functions are predefined option mixins whereas we're
supplying our own value mixins here. I also noticed that there's a bit
of an inconsistency with the naming. The basic functions (long etc.)
don't have the "field_" prefix, but this one does. Maybe a better name
would be custom_mixins? Or value_mixins? Or custom_value? Or maybe
convert_value? I like the last one:
portmask: int | None = field(default=None, metadata=convert_value(hex))
metadata=params.convert_value(_port_to_pci,
metadata=params.multiple(params.short("a"))), # in sut_node

I think this is easier to grasp. I'm thinking about whether we need to
have mixin(s) in the name and I don't think it adds much. If I'm a
developer, I'm looking at these functions and I stumble upon
convert_value, what I'm thinking is "Nice, I can do some conversion on
the values I pass, how do I do that?", then I look at the signature
and find out that I expect, that is I need to pass a function (or
multiple function if I want to). I guess this comes down to the
function name (field_mixins) not conveying what it's doing, rather
what you're passing to it.

So my conclusion from this brainstorming is that a better name would
be convert_value. :-)

Also, unrelated, but the context is lost. Another thing I just noticed
is in the docstring:
The ``metadata`` keyword argument can be used to chain metadata
modifiers together.

We're missing the Args: section in all of the docstrings (where we
could put the above). Also the Returns: section.

> >> +    return {**metadata, META_MIXINS: mixins}
> >
> > metadata | {META_MIXINS: mixins} also creates a new dictionary with
> > values from both and I think that would be more readable (since it's
> > mentioned in docs:
> > https://docs.python.org/3/library/stdtypes.html#mapping-types-dict).
>
> If we were to use `None` as default to the arguments, then this would no
> longer be needed. But great shout, wasn't aware of this feature added in
> 3.9.
>

It wouldn't? We'd still have to merge the dicts when metadata is not None, no?

<snip>

Re: [PATCH 2/6] dts: use Params for interactive shells

[Luca Vizzarro]

On 09/04/2024 15:56, Juraj Linkeš wrote:
> On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>>
>> I'm not sure if allowing None should be the solution for these shells
>> as opposed to just supplying an empty parameter object. Maybe
>> something that could be done is the factory method in sut_node allows
>> it to be None, but when it comes time to make the abstract shell it
>> creates an empty one if it doesn't exist, or the init method makes
>> this an optional parameter but creates it if it doesn't exist.
>>
>> I suppose this logic would have to exist somewhere because the
>> parameters aren't always required, it's just a question of where we
>> should put it and if we should just assume that the interactive shell
>> class just knows how to accept some parameters and put them into the
>> shell. I would maybe leave this as something that cannot be None and
>> handle it outside of the shell, but I'm not sure it's something really
>> required and I don't have a super strong opinion on it.
>>
> 
> I think this is an excellent idea. An empty Params (or StrParams or
> EalParams if we want to make Params truly abstract and thus not
> instantiable) would work as the default value and it would be an
> elegant solution since dev will only pass non-empty Params.
> 

I left it as generic as it could get as I honestly couldn't grasp the 
full picture. I am really keen to ditch everything else and use an empty 
Params object instead for defaults. And as Juraj said, if I am making 
Params a true abstract object, then it'd be picking one of the Params 
subclasses mentioned above.

I guess EalParams could only be used with shells are that sure to be 
DPDK apps, and I presume that's only TestPmdShell for now.

>>> +        from framework.testbed_model.sut_node import EalParameters
>>> +
>>> +        assert isinstance(self._app_args, EalParameters)
>>> +
>>> +        if isinstance(self._app_args.app_params, StrParams):
>>> +            self._app_args.app_params.value += " -i --mask-event intr_lsc"
>>> +
>>> +        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
>>
>> I we should override the _app_args parameter in the testpmd shell to
>> always be EalParameters instead of doing this import and assertion.
>> It's a DPDK app, so we will always need EalParameters anyway, so we
>> might as well put that as our typehint to start off as what we expect
>> instead.
>>
>> The checking of an instance of StrParams also feels a little strange
>> here, it might be more ideal if we could just add the parameters
>> without this check. Maybe something we can do, just because these
>> parameters are meant to be CLI commands anyway and will be rendered as
>> a string, is replace the StrParams object with a method on the base
>> Params dataclass that allows you to just add any string as a value
>> only field. Then, we don't have to bother validating anything about
>> the app parameters and we don't care what they are, we can just add a
>> string to them of new parameters.
>>
>> I think this is something that likely also gets solved when you
>> replace this with testpmd parameters, but it might be a good way to
>> make the Params object more versatile in general so that people can
>> diverge and add their own flags to it if needed.

Jeremy, I agree this is not ideal. Although this was meant only to be 
transitionary for the next commit (as you say it gets resolved with 
TestPmdParams). But I agree with you that we can integrate the StrParams 
facility within Params. This means no longer making Params a true 
abstract class though, which is something I can live with, especially if 
we can make it simpler.

> I'd say this is done because of circular imports. If so, we could move
> EalParameters to params.py, that should help. And when we're at it,
> either rename it to EalParams or rename the other classes to use the
> whole word.

Yeah, the circular imports are the main problem indeed. I considered 
refactoring but noticed it'd take more than just moving EalParam(eter)s 
to params.py. Nonetheless, keen to make a bigger refactoring to make 
this happen.
>>> +
>>>           super()._start_application(get_privileged_command)
>>>
>>>       def start(self, verify: bool = True) -> None:
>>   <snip>
>>> @@ -134,7 +136,7 @@ def create_interactive_shell(
>>>           shell_cls: Type[InteractiveShellType],
>>>           timeout: float,
>>>           privileged: bool,
>>> -        app_args: str,
>>> +        app_args: Params | None,
>>
>> This also falls in line with what I was saying before about where this
>> logic is handled. This was made to always be a string originally
>> because by this point it being optional was already handled by the
>> sut_node.create_interactive_shell() and we should have some kind of
>> value here (even if that value is an empty parameters dataclass) to
>> pass into the application. It sort of semantically doesn't really
>> change much, but at this point it might as well not be None, so we can
>> simplify this type.
Ack.
>>>       ) -> InteractiveShellType:
>>>           """Factory for interactive session handlers.
>>>
<snip>
>>> +@dataclass(kw_only=True)
>>> +class EalParameters(Params):
>>>       """The environment abstraction layer parameters.
>>>
>>>       The string representation can be created by converting the instance to a string.
>>>       """
>>>
>>> -    def __init__(
>>> -        self,
>>> -        lcore_list: LogicalCoreList,
>>> -        memory_channels: int,
>>> -        prefix: str,
>>> -        no_pci: bool,
>>> -        vdevs: list[VirtualDevice],
>>> -        ports: list[Port],
>>> -        other_eal_param: str,
>>> -    ):
>>> -        """Initialize the parameters according to inputs.
>>> -
>>> -        Process the parameters into the format used on the command line.
>>> +    lcore_list: LogicalCoreList = field(metadata=params.short("l"))
>>> +    """The list of logical cores to use."""
>>>
>>> -        Args:
>>> -            lcore_list: The list of logical cores to use.
>>> -            memory_channels: The number of memory channels to use.
>>> -            prefix: Set the file prefix string with which to start DPDK, e.g.: ``prefix='vf'``.
>>> -            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
>>> -            vdevs: Virtual devices, e.g.::
>>> +    memory_channels: int = field(metadata=params.short("n"))
>>> +    """The number of memory channels to use."""
>>>
>>> -                vdevs=[
>>> -                    VirtualDevice('net_ring0'),
>>> -                    VirtualDevice('net_ring1')
>>> -                ]
>>> -            ports: The list of ports to allow.
>>> -            other_eal_param: user defined DPDK EAL parameters, e.g.:
>>> -                ``other_eal_param='--single-file-segments'``
>>> -        """
>>> -        self._lcore_list = f"-l {lcore_list}"
>>> -        self._memory_channels = f"-n {memory_channels}"
>>> -        self._prefix = prefix
>>> -        if prefix:
>>> -            self._prefix = f"--file-prefix={prefix}"
>>> -        self._no_pci = "--no-pci" if no_pci else ""
>>> -        self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
>>> -        self._ports = " ".join(f"-a {port.pci}" for port in ports)
>>> -        self._other_eal_param = other_eal_param
>>> -
>>> -    def __str__(self) -> str:
>>> -        """Create the EAL string."""
>>> -        return (
>>> -            f"{self._lcore_list} "
>>> -            f"{self._memory_channels} "
>>> -            f"{self._prefix} "
>>> -            f"{self._no_pci} "
>>> -            f"{self._vdevs} "
>>> -            f"{self._ports} "
>>> -            f"{self._other_eal_param}"
>>> -        )
>>> +    prefix: str = field(metadata=params.long("file-prefix"))
>>> +    """Set the file prefix string with which to start DPDK, e.g.: ``prefix="vf"``."""
>>
>> Just a small note on docstrings, I believe generally in DTS we use the
>> google docstring
>> (https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings)
>> format where it applies with some small differences. Because these
>> attributes aren't class vars however, I believe they should be in the
>> docstring for the class in the `Attributes` section. I generally have
>> trouble remembering exactly how it should look, but Juraj documented
>> it in `doc/guides/tools/dts.rst`
>>
> 
> I noticed this right away. Here's the bullet point that applies:
> 
> * The ``dataclass.dataclass`` decorator changes how the attributes are
> processed.
>    The dataclass attributes which result in instance variables/attributes
>    should also be recorded in the ``Attributes:`` section.
>

I truly did not even for a second recognise the distinction. But this 
explains a lot. So the idea here is to move every documented field as an 
attribute in the main class docstring?

>>> +
>>> +    no_pci: params.Option
>>> +    """Switch to disable PCI bus e.g.: ``no_pci=True``."""
>> <snip>
>>
>>> --
>>> 2.34.1
>>>

Re: [PATCH 1/6] dts: add parameters data structure

[Luca Vizzarro]

On 10/04/2024 10:15, Juraj Linkeš wrote:
>>>> +
>>>> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
>>>> +    """Injects the value of the attribute as-is without flag. Metadata modifier for :func:`dataclasses.field`."""
>>>> +    return {**metadata, META_VALUE_ONLY: True}
>>>
>>> These methods, on the other hand, are used outside this module, so it
>>> makes sense to have them outside Params. It could be better to have
>>> them inside as static methods, as they're closely related. Looking at
>>> how they're used, we should unite the imports:
>>> 1. In testpmd_shell, they're imported directly (from framework.params
>>> import long)
>>> 2. In sut_node, just the params module is imported (from framework
>>> import params and then accessed via it: metadata=params.short("l"))
>>>
>> Having a shorter version may look less verbose. I agree that we can make
>> everything a static method of Params, but then having to do Params.short
>> etc everytime will make it look more verbose. So what option do we
>> prefer? The functions do belong to the params module nonetheless, and
>> therefore are meant to be used in conjunction with the Params class.
>>
> 
> When I first saw the code, I liked the usage in sut_node better, e.g.:
> prefix: str = field(metadata=params.long("file-prefix")). I think this
> is because it's obvious where the function comes from. I'd do the
> longer version because I think most people are just going to glance at
> the code when they want to know how to properly implement an argument
> so the explicit nature could help with understanding how it should be
> done.

Ack.

>>> If we move these to Params, we could import Params and use them
>>> similarly to 2. Not sure which one is better.
>>>
>>
>>
>>>> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:
>>>
>>> Any reason why mixins are plural? I've only seen this used with one
>>> argument, do you anticipate we'd need to use more than one? We could
>>> make this singular and if we ever need to do two things, we could just
>>> pass a function that does those two things (or calls two different
>>> functions). Also, I'd just rename the mixin the func or something like
>>> that.
>>>
>>> The default of an argument should not be mutable, here's a quick
>>> explanation: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
>>
>>
>> Indeed the reason for which I create dictionaries, as I am treating them
>> as read only. I wanted to avoid to bloat the code with lots of `is None`
>> checks. But we can sacrifice this optimization for better code.
>>
> 
> This would be the only place where we'd do the check, as I don't think
> we need the metadata argument in any of the other functions - those
> seem to be mutually exclusive, but maybe they're not? In any case,
> we'd need to fix this, I don't think treating them as read-only avoids
> the problem.
> 

They are not mutually exclusive. But thinking of it we can spare every 
problem with having to chain "metadata" by letting the user do it 
through the use of the pipe operator.

>> Here we
>> are just chaining them for reusability. Do you have any better name in mind?
>>
> 
> I don't know, so let's brainstorm a bit. Let's start with the usage:
> portmask: int | None = field(default=None, metadata=field_mixins(hex))
> 
> Here it's not clear at all why it's called field_mixins, at least
> compared to the other functions which are not called mixins. I guess
> the other functions are predefined option mixins whereas we're
> supplying our own value mixins here. I also noticed that there's a bit
> of an inconsistency with the naming. The basic functions (long etc.)
> don't have the "field_" prefix, but this one does. Maybe a better name
> would be custom_mixins? Or value_mixins? Or custom_value? Or maybe
> convert_value? I like the last one:
> portmask: int | None = field(default=None, metadata=convert_value(hex))
> metadata=params.convert_value(_port_to_pci,
> metadata=params.multiple(params.short("a"))), # in sut_node
> 
> I think this is easier to grasp. I'm thinking about whether we need to
> have mixin(s) in the name and I don't think it adds much. If I'm a
> developer, I'm looking at these functions and I stumble upon
> convert_value, what I'm thinking is "Nice, I can do some conversion on
> the values I pass, how do I do that?", then I look at the signature
> and find out that I expect, that is I need to pass a function (or
> multiple function if I want to). I guess this comes down to the
> function name (field_mixins) not conveying what it's doing, rather
> what you're passing to it.
> 
> So my conclusion from this brainstorming is that a better name would
> be convert_value. :-)
> 
> Also, unrelated, but the context is lost. Another thing I just noticed
> is in the docstring:
> The ``metadata`` keyword argument can be used to chain metadata
> modifiers together.
> 
> We're missing the Args: section in all of the docstrings (where we
> could put the above). Also the Returns: section.

Sure, we can do convert_value. I am honestly not too fussed about 
naming, and your proposal makes more sense. And as above, we can spare 
the whole metadata problem. Using your example:

   metadata=params.short("a") | params.multiple()

>>>> +    return {**metadata, META_MIXINS: mixins}
>>>
>>> metadata | {META_MIXINS: mixins} also creates a new dictionary with
>>> values from both and I think that would be more readable (since it's
>>> mentioned in docs:
>>> https://docs.python.org/3/library/stdtypes.html#mapping-types-dict).
>>
>> If we were to use `None` as default to the arguments, then this would no
>> longer be needed. But great shout, wasn't aware of this feature added in
>> 3.9.
>>
> 
> It wouldn't? We'd still have to merge the dicts when metadata is not None, no?
> 
> <snip>

Re: [PATCH 2/6] dts: use Params for interactive shells

[Juraj Linkeš]

On Wed, Apr 10, 2024 at 11:34 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 09/04/2024 15:56, Juraj Linkeš wrote:
> > On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
> >>
> >> I'm not sure if allowing None should be the solution for these shells
> >> as opposed to just supplying an empty parameter object. Maybe
> >> something that could be done is the factory method in sut_node allows
> >> it to be None, but when it comes time to make the abstract shell it
> >> creates an empty one if it doesn't exist, or the init method makes
> >> this an optional parameter but creates it if it doesn't exist.
> >>
> >> I suppose this logic would have to exist somewhere because the
> >> parameters aren't always required, it's just a question of where we
> >> should put it and if we should just assume that the interactive shell
> >> class just knows how to accept some parameters and put them into the
> >> shell. I would maybe leave this as something that cannot be None and
> >> handle it outside of the shell, but I'm not sure it's something really
> >> required and I don't have a super strong opinion on it.
> >>
> >
> > I think this is an excellent idea. An empty Params (or StrParams or
> > EalParams if we want to make Params truly abstract and thus not
> > instantiable) would work as the default value and it would be an
> > elegant solution since dev will only pass non-empty Params.
> >
>
> I left it as generic as it could get as I honestly couldn't grasp the
> full picture. I am really keen to ditch everything else and use an empty
> Params object instead for defaults. And as Juraj said, if I am making
> Params a true abstract object, then it'd be picking one of the Params
> subclasses mentioned above.
>
> I guess EalParams could only be used with shells are that sure to be
> DPDK apps, and I presume that's only TestPmdShell for now.
>
> >>> +        from framework.testbed_model.sut_node import EalParameters
> >>> +
> >>> +        assert isinstance(self._app_args, EalParameters)
> >>> +
> >>> +        if isinstance(self._app_args.app_params, StrParams):
> >>> +            self._app_args.app_params.value += " -i --mask-event intr_lsc"
> >>> +
> >>> +        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
> >>
> >> I we should override the _app_args parameter in the testpmd shell to
> >> always be EalParameters instead of doing this import and assertion.
> >> It's a DPDK app, so we will always need EalParameters anyway, so we
> >> might as well put that as our typehint to start off as what we expect
> >> instead.
> >>
> >> The checking of an instance of StrParams also feels a little strange
> >> here, it might be more ideal if we could just add the parameters
> >> without this check. Maybe something we can do, just because these
> >> parameters are meant to be CLI commands anyway and will be rendered as
> >> a string, is replace the StrParams object with a method on the base
> >> Params dataclass that allows you to just add any string as a value
> >> only field. Then, we don't have to bother validating anything about
> >> the app parameters and we don't care what they are, we can just add a
> >> string to them of new parameters.
> >>
> >> I think this is something that likely also gets solved when you
> >> replace this with testpmd parameters, but it might be a good way to
> >> make the Params object more versatile in general so that people can
> >> diverge and add their own flags to it if needed.
>
> Jeremy, I agree this is not ideal. Although this was meant only to be
> transitionary for the next commit (as you say it gets resolved with
> TestPmdParams). But I agree with you that we can integrate the StrParams
> facility within Params. This means no longer making Params a true
> abstract class though, which is something I can live with, especially if
> we can make it simpler.
>

No problem with it not being a true abstract class if it's not going
to have abstract methods/properties. I guess making it instantiable
actually makes sense, since it's always going to be empty (as there
are no fields), which should make the usage mostly error-free.

> > I'd say this is done because of circular imports. If so, we could move
> > EalParameters to params.py, that should help. And when we're at it,
> > either rename it to EalParams or rename the other classes to use the
> > whole word.
>
> Yeah, the circular imports are the main problem indeed. I considered
> refactoring but noticed it'd take more than just moving EalParam(eter)s
> to params.py. Nonetheless, keen to make a bigger refactoring to make
> this happen.

Please do. My thinking was if we made params.py standalone we could
import from it anywhere but I guess it's not that simple. :-) In any
case, refactoring is always welcome - please put moved files into a
separate commit.

> >>> +
> >>>           super()._start_application(get_privileged_command)
> >>>
> >>>       def start(self, verify: bool = True) -> None:
> >>   <snip>
> >>> @@ -134,7 +136,7 @@ def create_interactive_shell(
> >>>           shell_cls: Type[InteractiveShellType],
> >>>           timeout: float,
> >>>           privileged: bool,
> >>> -        app_args: str,
> >>> +        app_args: Params | None,
> >>
> >> This also falls in line with what I was saying before about where this
> >> logic is handled. This was made to always be a string originally
> >> because by this point it being optional was already handled by the
> >> sut_node.create_interactive_shell() and we should have some kind of
> >> value here (even if that value is an empty parameters dataclass) to
> >> pass into the application. It sort of semantically doesn't really
> >> change much, but at this point it might as well not be None, so we can
> >> simplify this type.
> Ack.
> >>>       ) -> InteractiveShellType:
> >>>           """Factory for interactive session handlers.
> >>>
> <snip>
> >>> +@dataclass(kw_only=True)
> >>> +class EalParameters(Params):
> >>>       """The environment abstraction layer parameters.
> >>>
> >>>       The string representation can be created by converting the instance to a string.
> >>>       """
> >>>
> >>> -    def __init__(
> >>> -        self,
> >>> -        lcore_list: LogicalCoreList,
> >>> -        memory_channels: int,
> >>> -        prefix: str,
> >>> -        no_pci: bool,
> >>> -        vdevs: list[VirtualDevice],
> >>> -        ports: list[Port],
> >>> -        other_eal_param: str,
> >>> -    ):
> >>> -        """Initialize the parameters according to inputs.
> >>> -
> >>> -        Process the parameters into the format used on the command line.
> >>> +    lcore_list: LogicalCoreList = field(metadata=params.short("l"))
> >>> +    """The list of logical cores to use."""
> >>>
> >>> -        Args:
> >>> -            lcore_list: The list of logical cores to use.
> >>> -            memory_channels: The number of memory channels to use.
> >>> -            prefix: Set the file prefix string with which to start DPDK, e.g.: ``prefix='vf'``.
> >>> -            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
> >>> -            vdevs: Virtual devices, e.g.::
> >>> +    memory_channels: int = field(metadata=params.short("n"))
> >>> +    """The number of memory channels to use."""
> >>>
> >>> -                vdevs=[
> >>> -                    VirtualDevice('net_ring0'),
> >>> -                    VirtualDevice('net_ring1')
> >>> -                ]
> >>> -            ports: The list of ports to allow.
> >>> -            other_eal_param: user defined DPDK EAL parameters, e.g.:
> >>> -                ``other_eal_param='--single-file-segments'``
> >>> -        """
> >>> -        self._lcore_list = f"-l {lcore_list}"
> >>> -        self._memory_channels = f"-n {memory_channels}"
> >>> -        self._prefix = prefix
> >>> -        if prefix:
> >>> -            self._prefix = f"--file-prefix={prefix}"
> >>> -        self._no_pci = "--no-pci" if no_pci else ""
> >>> -        self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
> >>> -        self._ports = " ".join(f"-a {port.pci}" for port in ports)
> >>> -        self._other_eal_param = other_eal_param
> >>> -
> >>> -    def __str__(self) -> str:
> >>> -        """Create the EAL string."""
> >>> -        return (
> >>> -            f"{self._lcore_list} "
> >>> -            f"{self._memory_channels} "
> >>> -            f"{self._prefix} "
> >>> -            f"{self._no_pci} "
> >>> -            f"{self._vdevs} "
> >>> -            f"{self._ports} "
> >>> -            f"{self._other_eal_param}"
> >>> -        )
> >>> +    prefix: str = field(metadata=params.long("file-prefix"))
> >>> +    """Set the file prefix string with which to start DPDK, e.g.: ``prefix="vf"``."""
> >>
> >> Just a small note on docstrings, I believe generally in DTS we use the
> >> google docstring
> >> (https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings)
> >> format where it applies with some small differences. Because these
> >> attributes aren't class vars however, I believe they should be in the
> >> docstring for the class in the `Attributes` section. I generally have
> >> trouble remembering exactly how it should look, but Juraj documented
> >> it in `doc/guides/tools/dts.rst`
> >>
> >
> > I noticed this right away. Here's the bullet point that applies:
> >
> > * The ``dataclass.dataclass`` decorator changes how the attributes are
> > processed.
> >    The dataclass attributes which result in instance variables/attributes
> >    should also be recorded in the ``Attributes:`` section.
> >
>
> I truly did not even for a second recognise the distinction. But this
> explains a lot. So the idea here is to move every documented field as an
> attribute in the main class docstring?
>

Yes. You can look at the dataclasses in config/_init__.py to get an
idea on what it should look like.

> >>> +
> >>> +    no_pci: params.Option
> >>> +    """Switch to disable PCI bus e.g.: ``no_pci=True``."""
> >> <snip>
> >>
> >>> --
> >>> 2.34.1
> >>>
>

Re: [PATCH 1/6] dts: add parameters data structure

[Juraj Linkeš]

On Wed, Apr 10, 2024 at 11:51 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 10:15, Juraj Linkeš wrote:
> >>>> +
> >>>> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> >>>> +    """Injects the value of the attribute as-is without flag. Metadata modifier for :func:`dataclasses.field`."""
> >>>> +    return {**metadata, META_VALUE_ONLY: True}
> >>>
> >>> These methods, on the other hand, are used outside this module, so it
> >>> makes sense to have them outside Params. It could be better to have
> >>> them inside as static methods, as they're closely related. Looking at
> >>> how they're used, we should unite the imports:
> >>> 1. In testpmd_shell, they're imported directly (from framework.params
> >>> import long)
> >>> 2. In sut_node, just the params module is imported (from framework
> >>> import params and then accessed via it: metadata=params.short("l"))
> >>>
> >> Having a shorter version may look less verbose. I agree that we can make
> >> everything a static method of Params, but then having to do Params.short
> >> etc everytime will make it look more verbose. So what option do we
> >> prefer? The functions do belong to the params module nonetheless, and
> >> therefore are meant to be used in conjunction with the Params class.
> >>
> >
> > When I first saw the code, I liked the usage in sut_node better, e.g.:
> > prefix: str = field(metadata=params.long("file-prefix")). I think this
> > is because it's obvious where the function comes from. I'd do the
> > longer version because I think most people are just going to glance at
> > the code when they want to know how to properly implement an argument
> > so the explicit nature could help with understanding how it should be
> > done.
>
> Ack.
>
> >>> If we move these to Params, we could import Params and use them
> >>> similarly to 2. Not sure which one is better.
> >>>
> >>
> >>
> >>>> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:
> >>>
> >>> Any reason why mixins are plural? I've only seen this used with one
> >>> argument, do you anticipate we'd need to use more than one? We could
> >>> make this singular and if we ever need to do two things, we could just
> >>> pass a function that does those two things (or calls two different
> >>> functions). Also, I'd just rename the mixin the func or something like
> >>> that.
> >>>
> >>> The default of an argument should not be mutable, here's a quick
> >>> explanation: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
> >>
> >>
> >> Indeed the reason for which I create dictionaries, as I am treating them
> >> as read only. I wanted to avoid to bloat the code with lots of `is None`
> >> checks. But we can sacrifice this optimization for better code.
> >>
> >
> > This would be the only place where we'd do the check, as I don't think
> > we need the metadata argument in any of the other functions - those
> > seem to be mutually exclusive, but maybe they're not? In any case,
> > we'd need to fix this, I don't think treating them as read-only avoids
> > the problem.
> >
>
> They are not mutually exclusive. But thinking of it we can spare every
> problem with having to chain "metadata" by letting the user do it
> through the use of the pipe operator.
>

Sounds good.

> >> Here we
> >> are just chaining them for reusability. Do you have any better name in mind?
> >>
> >
> > I don't know, so let's brainstorm a bit. Let's start with the usage:
> > portmask: int | None = field(default=None, metadata=field_mixins(hex))
> >
> > Here it's not clear at all why it's called field_mixins, at least
> > compared to the other functions which are not called mixins. I guess
> > the other functions are predefined option mixins whereas we're
> > supplying our own value mixins here. I also noticed that there's a bit
> > of an inconsistency with the naming. The basic functions (long etc.)
> > don't have the "field_" prefix, but this one does. Maybe a better name
> > would be custom_mixins? Or value_mixins? Or custom_value? Or maybe
> > convert_value? I like the last one:
> > portmask: int | None = field(default=None, metadata=convert_value(hex))
> > metadata=params.convert_value(_port_to_pci,
> > metadata=params.multiple(params.short("a"))), # in sut_node
> >
> > I think this is easier to grasp. I'm thinking about whether we need to
> > have mixin(s) in the name and I don't think it adds much. If I'm a
> > developer, I'm looking at these functions and I stumble upon
> > convert_value, what I'm thinking is "Nice, I can do some conversion on
> > the values I pass, how do I do that?", then I look at the signature
> > and find out that I expect, that is I need to pass a function (or
> > multiple function if I want to). I guess this comes down to the
> > function name (field_mixins) not conveying what it's doing, rather
> > what you're passing to it.
> >
> > So my conclusion from this brainstorming is that a better name would
> > be convert_value. :-)
> >
> > Also, unrelated, but the context is lost. Another thing I just noticed
> > is in the docstring:
> > The ``metadata`` keyword argument can be used to chain metadata
> > modifiers together.
> >
> > We're missing the Args: section in all of the docstrings (where we
> > could put the above). Also the Returns: section.
>
> Sure, we can do convert_value. I am honestly not too fussed about
> naming, and your proposal makes more sense.

Ok. I like to think a lot about these names because I think it would
save a considerable amount of time for future developers.

> And as above, we can spare
> the whole metadata problem. Using your example:
>
>    metadata=params.short("a") | params.multiple()
>

I see what you meant, this is awesome. Intuitive, understandable,
concise and easy to use.

> >>>> +    return {**metadata, META_MIXINS: mixins}
> >>>
> >>> metadata | {META_MIXINS: mixins} also creates a new dictionary with
> >>> values from both and I think that would be more readable (since it's
> >>> mentioned in docs:
> >>> https://docs.python.org/3/library/stdtypes.html#mapping-types-dict).
> >>
> >> If we were to use `None` as default to the arguments, then this would no
> >> longer be needed. But great shout, wasn't aware of this feature added in
> >> 3.9.
> >>
> >
> > It wouldn't? We'd still have to merge the dicts when metadata is not None, no?
> >
> > <snip>
>

Re: [PATCH 3/6] dts: add testpmd shell params

[Luca Vizzarro]

On 09/04/2024 17:37, Juraj Linkeš wrote:
> As Jeremy pointed out, going forward, this is likely to become bloated
> and moving it to params.py (for example) may be better.
> 
> There's a lot of testpmd args here. I commented on the implementation
> of some of them. I didn't verify that the actual values match the docs
> or, god forbid, tested all of it. :-) Doing that as we start using
> them is going to be good enough.

It is indeed a lot of args. I double checked most of them, so it should 
be mostly correct, but unfortunately I am not 100% sure. I did notice 
discrepancies between the docs and the source code of testpmd too. 
Although not ideal, I am inclining to update the definitions whenever a 
newly implemented test case hits a roadblock.

One thing that I don't remember if I mentioned so far, is the "XYPair". 
You see --flag=X,[Y] in the docs, but I am sure to have read somewhere 
this is potentially just a comma-separated multiple value.

> On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>>
>> Implement all the testpmd shell parameters into a data structure.
>>
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> ---
>>   dts/framework/remote_session/testpmd_shell.py | 633 +++++++++++++++++-
>>   1 file changed, 615 insertions(+), 18 deletions(-)
>>
>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
>> index db3abb7600..a823dc53be 100644
>> --- a/dts/framework/remote_session/testpmd_shell.py
>> +++ b/dts/framework/remote_session/testpmd_shell.py
> 
> <snip>
> 
>> +@str_mixins(bracketed, comma_separated)
>> +class TestPmdRingNUMAConfig(NamedTuple):
>> +    """Tuple associating DPDK port, direction of the flow and NUMA socket."""
> 
> Is there any particular order for these various classes?

No, there is no actual order, potential dependencies aside.

>> +
>> +    port: int
>> +    direction: TestPmdFlowDirection
>> +    socket: int
>> +
>> +
> 
> <snip>
> 
>> +@dataclass(kw_only=True)
>> +class TestPmdTXOnlyForwardingMode(Params):
> 
> The three special forwarding modes should really be moved right after
> TestPmdForwardingModes. Do we actually need these three in
> TestPmdForwardingModes? Looks like we could just remove those from
> TestPmdForwardingModes since they have to be passed separately, not as
> that Enum.

Can move and no we don't really need them in TestPmdForwardingModes, 
they can be hardcoded in their own special classes.

>> +    __forward_mode: Literal[TestPmdForwardingModes.txonly] = field(
>> +        default=TestPmdForwardingModes.txonly, init=False, metadata=long("forward-mode")
>> +    )
> 
> I guess this is here so that "--forward-mode=txonly" gets rendered,
> right? Why the two underscored? Is that because we want to hammer home
> the fact that this is init=False, a kind of internal field? I'd like
> to make it like the other fields, without any underscores (or maybe
> just one underscore), and documented (definitely documented).
> If we remove txonly from the Enum, we could just have the string value
> here. The Enums are mostly useful to give users the proper range of
> values.
> 

Correct and correct. A double underscore would ensure no access to this 
field, which is fixed and only there for rendering purposes... (also the 
developer doesn't get a hint from the IDE, at least not on VS code) and 
in the case of TestPmdForwardingModes it would remove a potential 
conflict. It can definitely be documented though.

>> +    multi_flow: Option = field(default=None, metadata=long("txonly-multi-flow"))
>> +    """Generate multiple flows."""
>> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
>> +    """Set TX segment sizes or total packet length."""
>> +
>> +
>> +@dataclass(kw_only=True)
>> +class TestPmdFlowGenForwardingMode(Params):
>> +    __forward_mode: Literal[TestPmdForwardingModes.flowgen] = field(
>> +        default=TestPmdForwardingModes.flowgen, init=False, metadata=long("forward-mode")
>> +    )
>> +    clones: int | None = field(default=None, metadata=long("flowgen-clones"))
>> +    """Set the number of each packet clones to be sent. Sending clones reduces host CPU load on
>> +    creating packets and may help in testing extreme speeds or maxing out Tx packet performance.
>> +    N should be not zero, but less than ‘burst’ parameter.
>> +    """
>> +    flows: int | None = field(default=None, metadata=long("flowgen-flows"))
>> +    """Set the number of flows to be generated, where 1 <= N <= INT32_MAX."""
>> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
>> +    """Set TX segment sizes or total packet length."""
>> +
>> +
>> +@dataclass(kw_only=True)
>> +class TestPmdNoisyForwardingMode(Params):
>> +    __forward_mode: Literal[TestPmdForwardingModes.noisy] = field(
>> +        default=TestPmdForwardingModes.noisy, init=False, metadata=long("forward-mode")
>> +    )
> 
> Are both of __forward_mode and forward_mode needed because we need to
> render both?

Yes, this would render as `--forward-mode=noisy --noisy-forward-mode=io` 
using IO as example.

>> +    forward_mode: (
>> +        Literal[
>> +            TestPmdForwardingModes.io,
>> +            TestPmdForwardingModes.mac,
>> +            TestPmdForwardingModes.macswap,
>> +            TestPmdForwardingModes.fivetswap,
>> +        ]
>> +        | None
> 
> Is there a difference between using union (TestPmdForwardingModes.io |
> TestPmdForwardingModes.mac etc.) and Literal?

TestPmdForwardingModes.io etc are literals and mypy complains:

error: Invalid type: try using Literal[TestPmdForwardingModes.io] 
instead?  [misc]

Therefore they need to be wrapped in Literal[..]

Literal[A, B] is the equivalent of Union[Literal[A], Literal[B]]

So this ultimately renders as Union[Lit[io], Lit[mac], Lit[macswap], 
Lit[fivetswap], None]. So it's really a matter of conciseness, by using 
Literal[A, ..], vs intuitiveness, by using Literal[A] | Literal[..] | ..

Which one would we prefer?

>> +@dataclass
>> +class TestPmdDisableRSS(Params):
>> +    """Disable RSS (Receive Side Scaling)."""
> 
> Let's put the explanation/reminder of what RSS stands for to either
> all three classes or none of them.
> 

Ack.
>> +    rss: TestPmdDisableRSS | TestPmdSetRSSIPOnly | TestPmdSetRSSUDP | None = None
>> +    """RSS option setting.
>> +
>> +    The value can be one of:
>> +    * :class:`TestPmdDisableRSS`, to disable RSS
>> +    * :class:`TestPmdSetRSSIPOnly`, to set RSS for IPv4/IPv6 only
>> +    * :class:`TestPmdSetRSSUDP`, to set RSS for IPv4/IPv6 and UDP
>> +    """
> 
> Have you thought about making an Enum where values would be these
> classes? That could simplify things a bit for users if it works.

It would be lovely to have classes as enum values, and I thought of it 
thinking of other languages like Rust. Not sure this is possible in 
Python. Are you suggesting to pass a class type as a value? In the hope 
that doing:

   TestPmdRSS.Disable()

could work? As this wouldn't. What works instead is:

   TestPmdRSS.Disable.value()

Which is somewhat ugly. Maybe I could modify the behaviour of the enum 
to return the underlying value instead of a reference to the field.

Do you have any better ideas?

>> +
>> +    forward_mode: (
>> +        Literal[
>> +            TestPmdForwardingModes.io,
>> +            TestPmdForwardingModes.mac,
>> +            TestPmdForwardingModes.macswap,
>> +            TestPmdForwardingModes.rxonly,
>> +            TestPmdForwardingModes.csum,
>> +            TestPmdForwardingModes.icmpecho,
>> +            TestPmdForwardingModes.ieee1588,
>> +            TestPmdForwardingModes.fivetswap,
>> +            TestPmdForwardingModes.shared_rxq,
>> +            TestPmdForwardingModes.recycle_mbufs,
>> +        ]
> 
> This could result in just TestPmdForwardingModes | the rest if we
> remove the compound fw modes from TestPmdForwardingModes. Maybe we
> could rename TestPmdForwardingModes to TestPmdSimpleForwardingModes or
> something at that point.

Yes, good idea.

>> +        | TestPmdFlowGenForwardingMode
>> +        | TestPmdTXOnlyForwardingMode
>> +        | TestPmdNoisyForwardingMode
>> +        | None
>> +    ) = TestPmdForwardingModes.io
>> +    """Set the forwarding mode.
> 
> <snip>
> 
>> +    mempool_allocation_mode: (
>> +        Literal[
>> +            TestPmdMempoolAllocationMode.native,
>> +            TestPmdMempoolAllocationMode.xmem,
>> +            TestPmdMempoolAllocationMode.xmemhuge,
>> +        ]
>> +        | TestPmdAnonMempoolAllocationMode
>> +        | None
> 
> This looks similar to fw modes, maybe the same applies here as well.

Ack.

>> +    ) = field(default=None, metadata=long("mp-alloc"))
>> +    """Select mempool allocation mode.
>> +
>> +    The value can be one of:
>> +    * :attr:`TestPmdMempoolAllocationMode.native`
>> +    * :class:`TestPmdAnonMempoolAllocationMode`
>> +    * :attr:`TestPmdMempoolAllocationMode.xmem`
>> +    * :attr:`TestPmdMempoolAllocationMode.xmemhuge`
>> +    """

Re: [PATCH 4/6] dts: use testpmd params for scatter test suite

[Luca Vizzarro]

On 09/04/2024 20:12, Juraj Linkeš wrote:
>> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
>>           """
>>           testpmd = self.sut_node.create_interactive_shell(
>>               TestPmdShell,
>> -            app_parameters=StrParams(
>> -                "--mbcache=200 "
>> -                f"--mbuf-size={mbsize} "
>> -                "--max-pkt-len=9000 "
>> -                "--port-topology=paired "
>> -                "--tx-offloads=0x00008000"
>> +            app_parameters=TestPmdParameters(
>> +                forward_mode=TestPmdForwardingModes.mac,
>> +                mbcache=200,
>> +                mbuf_size=[mbsize],
>> +                max_pkt_len=9000,
>> +                tx_offloads=0x00008000,
>>               ),
>>               privileged=True,
>>           )
>> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> 
> Jeremy, does this change the test? Instead of configuring the fw mode
> after starting testpmd, we're starting testpmd with fw mode
> configured.

I am not Jeremy (please Jeremy still reply), but we discussed this on 
Slack. Reading through the testpmd source code, setting arguments like 
forward-mode in the command line, is the exact equivalent of calling 
`set forward mode` right after start-up. So it is equivalent in theory.

> If not, we should remove the testpmd.set_forward_mode method, as it's
> not used anymore.

Could there be test cases that change the forward mode multiple times in 
the same shell, though? As this could still be needed to cover this.

Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

[Luca Vizzarro]

On 10/04/2024 07:53, Juraj Linkeš wrote:
> I have a general question. What are these changes for? Do you
> anticipate us needing this in the future? Wouldn't it be better to add
> it only when we need it?

It's been sometime since we raised this task internally. This patch and 
the next one arise from some survey done on old DTS test cases.
Unfortunately, I can't pinpoint.

Specifically for this patch though, the timeout bit is useful in 
conjunction with the related change in the next. Instead of giving an 
optional timeout argument to all the commands where we may want to 
change it, aren't we better off with providing a facility to temporarily 
change this for the current scope?

> 
> On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>>
>> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>> <snip>
>>> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
>>> index a2c7b30d9f..5d80061e8d 100644
>>> --- a/dts/framework/remote_session/interactive_shell.py
>>> +++ b/dts/framework/remote_session/interactive_shell.py
>>> @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
>>>       _stdout: channel.ChannelFile
>>>       _ssh_channel: Channel
>>>       _logger: DTSLogger
>>> +    __default_timeout: float
>>
>> Only single underscores are used for other private variables, probably
>> better to keep that consistent with this one.
>>
> 
> I agree, I don't see a reason for the double underscore.

Ack.

> 
>>>       _timeout: float
>>>       _app_args: Params | None
>>> +    _is_privileged: bool = False
>> <snip>
>>> 2.34.1
>>>

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Luca Vizzarro]

On 10/04/2024 08:41, Juraj Linkeš wrote:
>> <snip>
>>> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>>>           if self._app_args.app_params is None:
>>>               self._app_args.app_params = TestPmdParameters()
>>>
>>> -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
>>> +        assert isinstance(self._app_args.app_params, TestPmdParameters)
>>> +
>>
>> This is tricky because ideally we wouldn't have the assertion here,
>> but I understand why it is needed because Eal parameters have app args
>> which can be any instance of params. I'm not sure of the best way to
>> solve this, because making testpmd parameters extend from eal would
>> break the general scheme that you have in place, and having an
>> extension of EalParameters that enforces this app_args is
>> TestPmdParameters would solve the issues, but might be a little
>> clunky. Is there a way we can use a generic to get python to just
>> understand that, in this case, this will always be TestPmdParameters?
>> If not I might prefer making a private class where this is
>> TestPmdParameters, just because there aren't really any other
>> assertions that we use elsewhere and an unexpected exception from this
>> (even though I don't think that can happen) could cause people some
>> issues.
>>
>> It might be the case that an assertion is the easiest way to deal with
>> it though, what do you think?
>>
> 
> We could change the signature (just the type of app_args) of the init
> method - I think we should be able to create a type that's
> EalParameters with .app_params being TestPmdParameters or None. The
> init method would just call super().
> 
> Something like the above is basically necessary with inheritance where
> subclasses are all extensions (not just implementations) of the
> superclass (having differences in API).
> 

I believe this is indeed a tricky one. But, unfortunately, I am not 
understanding the solution that is being proposed. To me, it just feels 
like using a generic factory like:

   self.sut_node.create_interactive_shell(..)

is one of the reasons to bring in the majority of these complexities.

What do you mean by creating this new type that combines EalParams and 
TestPmdParams?

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Luca Vizzarro]

On 10/04/2024 08:50, Juraj Linkeš wrote:
> On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>>
>> This commit provides a state container for TestPmdShell. It currently
>> only indicates whether the packet forwarding has started
>> or not, and the number of ports which were given to the shell.
>>
> 
> A reminder, the commit message should explain why we're doing this
> change, not what the change is.
> 
>> This also fixes the behaviour of `wait_link_status_up` to use the
>> command timeout as inherited from InteractiveShell.
>>
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> ---
>>   dts/framework/remote_session/testpmd_shell.py | 41 +++++++++++++------
>>   1 file changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
>> index a823dc53be..ea1d254f86 100644
>> --- a/dts/framework/remote_session/testpmd_shell.py
>> +++ b/dts/framework/remote_session/testpmd_shell.py
>> @@ -678,19 +678,27 @@ def __str__(self) -> str:
>>           return self.pci_address
>>
>>
>> +@dataclass(slots=True)
>> +class TestPmdState:
>> +    """Session state container."""
>> +
>> +    #:
>> +    packet_forwarding_started: bool = False
> 
> The same question as in the previous patch, do you anticipate this
> being needed and should we add this only when it's actually used?
> 

As answered in the previous patch. We can always drop it and do it as 
needed of course.

>> +
>> +    #: The number of ports which were allowed on the command-line when testpmd was started.
>> +    number_of_ports: int = 0
>> +
>> +
>>   class TestPmdShell(InteractiveShell):
>>       """Testpmd interactive shell.
>>
>>       The testpmd shell users should never use
>>       the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
>>       call specialized methods. If there isn't one that satisfies a need, it should be added.
>> -
>> -    Attributes:
>> -        number_of_ports: The number of ports which were allowed on the command-line when testpmd
>> -            was started.
>>       """
>>
>> -    number_of_ports: int
>> +    #: Current state
>> +    state: TestPmdState = TestPmdState()
> 
> Assigning a value makes this a class variable, shared across all
> instances. This should be initialized in __init__().
> 
> But do we actually want to do this via composition? We'd need to
> access the attributes via .state all the time and I don't really like
> that. We could just put them into TestPmdShell directly, initializing
> them in __init__().

No problem. I separated them in fear of bloating TestPmdShell. But I 
agree on the bother of adding .state

>>
>>       #: The path to the testpmd executable.
>>       path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")

Re: [PATCH 3/6] dts: add testpmd shell params

[Juraj Linkeš]

On Wed, Apr 10, 2024 at 12:49 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 09/04/2024 17:37, Juraj Linkeš wrote:
> > As Jeremy pointed out, going forward, this is likely to become bloated
> > and moving it to params.py (for example) may be better.
> >
> > There's a lot of testpmd args here. I commented on the implementation
> > of some of them. I didn't verify that the actual values match the docs
> > or, god forbid, tested all of it. :-) Doing that as we start using
> > them is going to be good enough.
>
> It is indeed a lot of args. I double checked most of them, so it should
> be mostly correct, but unfortunately I am not 100% sure. I did notice
> discrepancies between the docs and the source code of testpmd too.
> Although not ideal, I am inclining to update the definitions whenever a
> newly implemented test case hits a roadblock.
>
> One thing that I don't remember if I mentioned so far, is the "XYPair".
> You see --flag=X,[Y] in the docs, but I am sure to have read somewhere
> this is potentially just a comma-separated multiple value.
>
> > On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >>
> >> Implement all the testpmd shell parameters into a data structure.
> >>
> >> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> >> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> >> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >> ---
> >>   dts/framework/remote_session/testpmd_shell.py | 633 +++++++++++++++++-
> >>   1 file changed, 615 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> >> index db3abb7600..a823dc53be 100644
> >> --- a/dts/framework/remote_session/testpmd_shell.py
> >> +++ b/dts/framework/remote_session/testpmd_shell.py
> >
> > <snip>
> >
> >> +@str_mixins(bracketed, comma_separated)
> >> +class TestPmdRingNUMAConfig(NamedTuple):
> >> +    """Tuple associating DPDK port, direction of the flow and NUMA socket."""
> >
> > Is there any particular order for these various classes?
>
> No, there is no actual order, potential dependencies aside.
>

Ok, can we order them according to when they appear in the code? Maybe
they already are.

> >> +
> >> +    port: int
> >> +    direction: TestPmdFlowDirection
> >> +    socket: int
> >> +
> >> +
> >
> > <snip>
> >
> >> +@dataclass(kw_only=True)
> >> +class TestPmdTXOnlyForwardingMode(Params):
> >
> > The three special forwarding modes should really be moved right after
> > TestPmdForwardingModes. Do we actually need these three in
> > TestPmdForwardingModes? Looks like we could just remove those from
> > TestPmdForwardingModes since they have to be passed separately, not as
> > that Enum.
>
> Can move and no we don't really need them in TestPmdForwardingModes,
> they can be hardcoded in their own special classes.
>
> >> +    __forward_mode: Literal[TestPmdForwardingModes.txonly] = field(
> >> +        default=TestPmdForwardingModes.txonly, init=False, metadata=long("forward-mode")
> >> +    )
> >
> > I guess this is here so that "--forward-mode=txonly" gets rendered,
> > right? Why the two underscored? Is that because we want to hammer home
> > the fact that this is init=False, a kind of internal field? I'd like
> > to make it like the other fields, without any underscores (or maybe
> > just one underscore), and documented (definitely documented).
> > If we remove txonly from the Enum, we could just have the string value
> > here. The Enums are mostly useful to give users the proper range of
> > values.
> >
>
> Correct and correct. A double underscore would ensure no access to this
> field, which is fixed and only there for rendering purposes... (also the
> developer doesn't get a hint from the IDE, at least not on VS code) and
> in the case of TestPmdForwardingModes it would remove a potential
> conflict. It can definitely be documented though.
>

Ok, can we do a single underscore? I don't really see a reason for two
underscores.

> >> +    multi_flow: Option = field(default=None, metadata=long("txonly-multi-flow"))
> >> +    """Generate multiple flows."""
> >> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
> >> +    """Set TX segment sizes or total packet length."""
> >> +
> >> +
> >> +@dataclass(kw_only=True)
> >> +class TestPmdFlowGenForwardingMode(Params):
> >> +    __forward_mode: Literal[TestPmdForwardingModes.flowgen] = field(
> >> +        default=TestPmdForwardingModes.flowgen, init=False, metadata=long("forward-mode")
> >> +    )
> >> +    clones: int | None = field(default=None, metadata=long("flowgen-clones"))
> >> +    """Set the number of each packet clones to be sent. Sending clones reduces host CPU load on
> >> +    creating packets and may help in testing extreme speeds or maxing out Tx packet performance.
> >> +    N should be not zero, but less than ‘burst’ parameter.
> >> +    """
> >> +    flows: int | None = field(default=None, metadata=long("flowgen-flows"))
> >> +    """Set the number of flows to be generated, where 1 <= N <= INT32_MAX."""
> >> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
> >> +    """Set TX segment sizes or total packet length."""
> >> +
> >> +
> >> +@dataclass(kw_only=True)
> >> +class TestPmdNoisyForwardingMode(Params):
> >> +    __forward_mode: Literal[TestPmdForwardingModes.noisy] = field(
> >> +        default=TestPmdForwardingModes.noisy, init=False, metadata=long("forward-mode")
> >> +    )
> >
> > Are both of __forward_mode and forward_mode needed because we need to
> > render both?
>
> Yes, this would render as `--forward-mode=noisy --noisy-forward-mode=io`
> using IO as example.
>
> >> +    forward_mode: (
> >> +        Literal[
> >> +            TestPmdForwardingModes.io,
> >> +            TestPmdForwardingModes.mac,
> >> +            TestPmdForwardingModes.macswap,
> >> +            TestPmdForwardingModes.fivetswap,
> >> +        ]
> >> +        | None
> >
> > Is there a difference between using union (TestPmdForwardingModes.io |
> > TestPmdForwardingModes.mac etc.) and Literal?
>
> TestPmdForwardingModes.io etc are literals and mypy complains:
>
> error: Invalid type: try using Literal[TestPmdForwardingModes.io]
> instead?  [misc]
>
> Therefore they need to be wrapped in Literal[..]
>
> Literal[A, B] is the equivalent of Union[Literal[A], Literal[B]]
>
> So this ultimately renders as Union[Lit[io], Lit[mac], Lit[macswap],
> Lit[fivetswap], None]. So it's really a matter of conciseness, by using
> Literal[A, ..], vs intuitiveness, by using Literal[A] | Literal[..] | ..
>
> Which one would we prefer?
>

Thanks, for the explanation, the way it's now is the most
straightforward, do I'd keep that.

> >> +@dataclass
> >> +class TestPmdDisableRSS(Params):
> >> +    """Disable RSS (Receive Side Scaling)."""
> >
> > Let's put the explanation/reminder of what RSS stands for to either
> > all three classes or none of them.
> >
>
> Ack.
> >> +    rss: TestPmdDisableRSS | TestPmdSetRSSIPOnly | TestPmdSetRSSUDP | None = None
> >> +    """RSS option setting.
> >> +
> >> +    The value can be one of:
> >> +    * :class:`TestPmdDisableRSS`, to disable RSS
> >> +    * :class:`TestPmdSetRSSIPOnly`, to set RSS for IPv4/IPv6 only
> >> +    * :class:`TestPmdSetRSSUDP`, to set RSS for IPv4/IPv6 and UDP
> >> +    """
> >
> > Have you thought about making an Enum where values would be these
> > classes? That could simplify things a bit for users if it works.
>
> It would be lovely to have classes as enum values, and I thought of it
> thinking of other languages like Rust. Not sure this is possible in
> Python. Are you suggesting to pass a class type as a value? In the hope
> that doing:
>
>    TestPmdRSS.Disable()
>
> could work? As this wouldn't. What works instead is:
>
>    TestPmdRSS.Disable.value()
>
> Which is somewhat ugly. Maybe I could modify the behaviour of the enum
> to return the underlying value instead of a reference to the field.
>
> Do you have any better ideas?
>

Not sure if it's better, but I was just thinking:
class RSSEnum(Enum):
    Disable: TestPmdDisableRSS()
    IPOnly: TestPmdSetRSSIPOnly()
    UDP: TestPmdSetRSSIPOnly()

with
rss: RSSEnum | None = None

In this case, the value of the field would be RSSEnum.Disable, but I
don't think that would work, as you mentioned.

Having these three neatly in one object would make it obvious that
these are the rss options, so I think it's worth exploring this a bit
more, but I don't have a solution.

> >> +
> >> +    forward_mode: (
> >> +        Literal[
> >> +            TestPmdForwardingModes.io,
> >> +            TestPmdForwardingModes.mac,
> >> +            TestPmdForwardingModes.macswap,
> >> +            TestPmdForwardingModes.rxonly,
> >> +            TestPmdForwardingModes.csum,
> >> +            TestPmdForwardingModes.icmpecho,
> >> +            TestPmdForwardingModes.ieee1588,
> >> +            TestPmdForwardingModes.fivetswap,
> >> +            TestPmdForwardingModes.shared_rxq,
> >> +            TestPmdForwardingModes.recycle_mbufs,
> >> +        ]
> >
> > This could result in just TestPmdForwardingModes | the rest if we
> > remove the compound fw modes from TestPmdForwardingModes. Maybe we
> > could rename TestPmdForwardingModes to TestPmdSimpleForwardingModes or
> > something at that point.
>
> Yes, good idea.
>
> >> +        | TestPmdFlowGenForwardingMode
> >> +        | TestPmdTXOnlyForwardingMode
> >> +        | TestPmdNoisyForwardingMode
> >> +        | None
> >> +    ) = TestPmdForwardingModes.io
> >> +    """Set the forwarding mode.
> >
> > <snip>
> >
> >> +    mempool_allocation_mode: (
> >> +        Literal[
> >> +            TestPmdMempoolAllocationMode.native,
> >> +            TestPmdMempoolAllocationMode.xmem,
> >> +            TestPmdMempoolAllocationMode.xmemhuge,
> >> +        ]
> >> +        | TestPmdAnonMempoolAllocationMode
> >> +        | None
> >
> > This looks similar to fw modes, maybe the same applies here as well.
>
> Ack.
>
> >> +    ) = field(default=None, metadata=long("mp-alloc"))
> >> +    """Select mempool allocation mode.
> >> +
> >> +    The value can be one of:
> >> +    * :attr:`TestPmdMempoolAllocationMode.native`
> >> +    * :class:`TestPmdAnonMempoolAllocationMode`
> >> +    * :attr:`TestPmdMempoolAllocationMode.xmem`
> >> +    * :attr:`TestPmdMempoolAllocationMode.xmemhuge`
> >> +    """
>

Re: [PATCH 4/6] dts: use testpmd params for scatter test suite

[Juraj Linkeš]

On Wed, Apr 10, 2024 at 12:53 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 09/04/2024 20:12, Juraj Linkeš wrote:
> >> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
> >>           """
> >>           testpmd = self.sut_node.create_interactive_shell(
> >>               TestPmdShell,
> >> -            app_parameters=StrParams(
> >> -                "--mbcache=200 "
> >> -                f"--mbuf-size={mbsize} "
> >> -                "--max-pkt-len=9000 "
> >> -                "--port-topology=paired "
> >> -                "--tx-offloads=0x00008000"
> >> +            app_parameters=TestPmdParameters(
> >> +                forward_mode=TestPmdForwardingModes.mac,
> >> +                mbcache=200,
> >> +                mbuf_size=[mbsize],
> >> +                max_pkt_len=9000,
> >> +                tx_offloads=0x00008000,
> >>               ),
> >>               privileged=True,
> >>           )
> >> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> >
> > Jeremy, does this change the test? Instead of configuring the fw mode
> > after starting testpmd, we're starting testpmd with fw mode
> > configured.
>
> I am not Jeremy (please Jeremy still reply), but we discussed this on
> Slack. Reading through the testpmd source code, setting arguments like
> forward-mode in the command line, is the exact equivalent of calling
> `set forward mode` right after start-up. So it is equivalent in theory.
>
> > If not, we should remove the testpmd.set_forward_mode method, as it's
> > not used anymore.
>
> Could there be test cases that change the forward mode multiple times in
> the same shell, though? As this could still be needed to cover this.

Yes, but we don't have such a test now. It's good practice to remove
unused code. We can still bring it back anytime, it'll be in git
history.

Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

[Juraj Linkeš]

On Wed, Apr 10, 2024 at 1:27 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 07:53, Juraj Linkeš wrote:
> > I have a general question. What are these changes for? Do you
> > anticipate us needing this in the future? Wouldn't it be better to add
> > it only when we need it?
>
> It's been sometime since we raised this task internally. This patch and
> the next one arise from some survey done on old DTS test cases.
> Unfortunately, I can't pinpoint.
>
> Specifically for this patch though, the timeout bit is useful in
> conjunction with the related change in the next. Instead of giving an
> optional timeout argument to all the commands where we may want to
> change it, aren't we better off with providing a facility to temporarily
> change this for the current scope?
>

This is a good question. If the scope is just one command, then no. If
it's more than one, then maybe yes. I don't know which is better.

We should also consider that this would introduce a difference in API
between the interactive and non-interactive sessions. Do we want to do
this there as well?

Also, maybe set_timeout should be a property or we could just make
_timeout public.
And is_privileged should just be privileged, as it's a property (which
shouldn't contain a verb; if it was a method it would be a good name).

> >
> > On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
> >>
> >> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >> <snip>
> >>> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> >>> index a2c7b30d9f..5d80061e8d 100644
> >>> --- a/dts/framework/remote_session/interactive_shell.py
> >>> +++ b/dts/framework/remote_session/interactive_shell.py
> >>> @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
> >>>       _stdout: channel.ChannelFile
> >>>       _ssh_channel: Channel
> >>>       _logger: DTSLogger
> >>> +    __default_timeout: float
> >>
> >> Only single underscores are used for other private variables, probably
> >> better to keep that consistent with this one.
> >>
> >
> > I agree, I don't see a reason for the double underscore.
>
> Ack.
>
> >
> >>>       _timeout: float
> >>>       _app_args: Params | None
> >>> +    _is_privileged: bool = False
> >> <snip>
> >>> 2.34.1
> >>>
>

Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

[Luca Vizzarro]

On 10/04/2024 14:35, Juraj Linkeš wrote:
> We should also consider that this would introduce a difference in API
> between the interactive and non-interactive sessions. Do we want to do
> this there as well?

Could definitely add it there as well. You are referring to 
RemoteSession I presume, right?

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Juraj Linkeš]

I overlooked this reply initially.

On Wed, Apr 10, 2024 at 1:35 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 08:41, Juraj Linkeš wrote:
> >> <snip>
> >>> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
> >>>           if self._app_args.app_params is None:
> >>>               self._app_args.app_params = TestPmdParameters()
> >>>
> >>> -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
> >>> +        assert isinstance(self._app_args.app_params, TestPmdParameters)
> >>> +
> >>
> >> This is tricky because ideally we wouldn't have the assertion here,
> >> but I understand why it is needed because Eal parameters have app args
> >> which can be any instance of params. I'm not sure of the best way to
> >> solve this, because making testpmd parameters extend from eal would
> >> break the general scheme that you have in place, and having an
> >> extension of EalParameters that enforces this app_args is
> >> TestPmdParameters would solve the issues, but might be a little
> >> clunky. Is there a way we can use a generic to get python to just
> >> understand that, in this case, this will always be TestPmdParameters?
> >> If not I might prefer making a private class where this is
> >> TestPmdParameters, just because there aren't really any other
> >> assertions that we use elsewhere and an unexpected exception from this
> >> (even though I don't think that can happen) could cause people some
> >> issues.
> >>
> >> It might be the case that an assertion is the easiest way to deal with
> >> it though, what do you think?
> >>
> >
> > We could change the signature (just the type of app_args) of the init
> > method - I think we should be able to create a type that's
> > EalParameters with .app_params being TestPmdParameters or None. The
> > init method would just call super().
> >
> > Something like the above is basically necessary with inheritance where
> > subclasses are all extensions (not just implementations) of the
> > superclass (having differences in API).
> >
>
> I believe this is indeed a tricky one. But, unfortunately, I am not
> understanding the solution that is being proposed. To me, it just feels
> like using a generic factory like:
>
>    self.sut_node.create_interactive_shell(..)
>
> is one of the reasons to bring in the majority of these complexities.
>

I've been thinking about these interactive shell constructors for some
time and I think the factory pattern is not well suitable for this.
Factories work well with classes with the same API (i.e.
implementations of abstract classes that don't add anything extra),
but are much less useful when dealing with classes with different
behaviors, such as the interactive shells. We see this here, different
apps are going to require different args and that alone kinda breaks
the factory pattern. I think we'll need to either ditch these
factories and instead just have methods that return the proper shell
(and the methods would only exist in classes where they belong, e.g.
testpmd only makes sense on an SUT). Or we could overload each factory
(the support has only been added in 3.11 with @typing.overload, but is
also available in typing_extensions, so we would be able to use it
with the extra dependency) where different signatures would return
different objects. In both cases the caller won't have to import the
class and the method signature is going to be clearer.

We have this pattern with sut/tg nodes. I decided to move away from
the node factory because it didn't add much and in fact the code was
only clunkier. The interactive shell is not quite the same, as the
shells are not standalone in the same way the nodes are (the shells
are tied to nodes). Let me know what you think about all this - both
Luca and Jeremy.

> What do you mean by creating this new type that combines EalParams and
> TestPmdParams?

Let me illustrate this on the TestPmdShell __init__() method I had in mind:

def __init__(self, interactive_session: SSHClient,
        logger: DTSLogger,
        get_privileged_command: Callable[[str], str] | None,
        app_args: EalTestPmdParams | None = None,
        timeout: float = SETTINGS.timeout,
    ) -> None:
    super().__init__(interactive_session, logger, get_privileged_command)
    self.state = TestPmdState()

Where EalTestPmdParams would be something that enforces that
app_args.app_params is of the TestPmdParameters type.

But thinking more about this, we're probably better off switching the
params composition. Instead of TestPmdParameters being part of
EalParameters, we do it the other way around. This way the type of
app_args could just be TestPmdParameters and the types should work.
Or we pass the args separately, but that would likely require ditching
the factories and replacing them with methods (or overloading them).

And hopefully the imports won't be impossible to solve. :-)

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Luca Vizzarro]

On 11/04/2024 11:30, Juraj Linkeš wrote:
> I've been thinking about these interactive shell constructors for some
> time and I think the factory pattern is not well suitable for this.
> Factories work well with classes with the same API (i.e.
> implementations of abstract classes that don't add anything extra),
> but are much less useful when dealing with classes with different
> behaviors, such as the interactive shells. We see this here, different
> apps are going to require different args and that alone kinda breaks
> the factory pattern. I think we'll need to either ditch these
> factories and instead just have methods that return the proper shell
> (and the methods would only exist in classes where they belong, e.g.
> testpmd only makes sense on an SUT). Or we could overload each factory
> (the support has only been added in 3.11 with @typing.overload, but is
> also available in typing_extensions, so we would be able to use it
> with the extra dependency) where different signatures would return
> different objects. In both cases the caller won't have to import the
> class and the method signature is going to be clearer.
> 
> We have this pattern with sut/tg nodes. I decided to move away from
> the node factory because it didn't add much and in fact the code was
> only clunkier. The interactive shell is not quite the same, as the
> shells are not standalone in the same way the nodes are (the shells
> are tied to nodes). Let me know what you think about all this - both
> Luca and Jeremy.

When writing this series, I went down the path of creating a 
`create_testpmd_shell` method at some point as a solution to these 
problems. Realising after that it may be too big of a change, and 
possibly best left to a discussion exactly like this one.

Generics used at this level may be a bit too much, especially for 
Python, as support is not *that* great. I am of the opinion that having 
a dedicated wrapper is easier for the developer and the user. Generics 
are not needed to this level anyways, as we have a limited selection of 
shells that are actually going to be used.

We can also swap the wrapping process to simplify things, instead of:

   shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)

do:

   shell = TestPmdShell(self.sut_node, ..)

Let the Shell class ingest the node, and not the other way round.

The current approach appears to me to be top-down instead of bottom-up. 
We take the most abstracted part and we work our way down. But all we 
want is concreteness to the end user (developer).

> Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> 
> def __init__(self, interactive_session: SSHClient,
>          logger: DTSLogger,
>          get_privileged_command: Callable[[str], str] | None,
>          app_args: EalTestPmdParams | None = None,
>          timeout: float = SETTINGS.timeout,
>      ) -> None:
>      super().__init__(interactive_session, logger, get_privileged_command)
>      self.state = TestPmdState()
> 
> Where EalTestPmdParams would be something that enforces that
> app_args.app_params is of the TestPmdParameters type.
> 
> But thinking more about this, we're probably better off switching the
> params composition. Instead of TestPmdParameters being part of
> EalParameters, we do it the other way around. This way the type of
> app_args could just be TestPmdParameters and the types should work.
> Or we pass the args separately, but that would likely require ditching
> the factories and replacing them with methods (or overloading them).
> 
> And hopefully the imports won't be impossible to solve. :-)

It is what I feared, and I think it may become even more convoluted. As 
you said, ditching the factories will simplify things and make it more 
straightforward. So, we wouldn't find ourselves in problems like these.

I don't have a strong preference in approach between:
* overloading node methods
* dedicated node methods
* let the shells ingest nodes instead

But if I were to give priority, I'd take it from last to first. Letting 
shells ingest nodes will decouple the situation adding an extra step of 
simplification. I may not see the full picture though. The two are 
reasonable but, having a dedicated node method will stop the requirement 
to import the shell we need, and it's pretty much equivalent... but 
overloading also is very new to Python, so I may prefer to stick to more 
established.

Letting TestPmdParams take EalParams, instead of the other way around, 
would naturally follow the bottom-up approach too. Allowing Params to 
arbitrarily append string arguments – as proposed, would also allow 
users to use a plain (EalParams + string). So sounds like a good 
approach overall.

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Juraj Linkeš]

On Thu, Apr 11, 2024 at 1:47 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 11/04/2024 11:30, Juraj Linkeš wrote:
> > I've been thinking about these interactive shell constructors for some
> > time and I think the factory pattern is not well suitable for this.
> > Factories work well with classes with the same API (i.e.
> > implementations of abstract classes that don't add anything extra),
> > but are much less useful when dealing with classes with different
> > behaviors, such as the interactive shells. We see this here, different
> > apps are going to require different args and that alone kinda breaks
> > the factory pattern. I think we'll need to either ditch these
> > factories and instead just have methods that return the proper shell
> > (and the methods would only exist in classes where they belong, e.g.
> > testpmd only makes sense on an SUT). Or we could overload each factory
> > (the support has only been added in 3.11 with @typing.overload, but is
> > also available in typing_extensions, so we would be able to use it
> > with the extra dependency) where different signatures would return
> > different objects. In both cases the caller won't have to import the
> > class and the method signature is going to be clearer.
> >
> > We have this pattern with sut/tg nodes. I decided to move away from
> > the node factory because it didn't add much and in fact the code was
> > only clunkier. The interactive shell is not quite the same, as the
> > shells are not standalone in the same way the nodes are (the shells
> > are tied to nodes). Let me know what you think about all this - both
> > Luca and Jeremy.
>
> When writing this series, I went down the path of creating a
> `create_testpmd_shell` method at some point as a solution to these
> problems. Realising after that it may be too big of a change, and
> possibly best left to a discussion exactly like this one.
>

The changes we discuss below don't seem that big. What do you think,
do we just add another patch to the series?

> Generics used at this level may be a bit too much, especially for
> Python, as support is not *that* great. I am of the opinion that having
> a dedicated wrapper is easier for the developer and the user. Generics
> are not needed to this level anyways, as we have a limited selection of
> shells that are actually going to be used.
>
> We can also swap the wrapping process to simplify things, instead of:
>
>    shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)
>
> do:
>
>    shell = TestPmdShell(self.sut_node, ..)
>
> Let the Shell class ingest the node, and not the other way round.
>

I thought about this a bit as well, it's a good approach. The current
design is top-down, as you say, in that "I have a node and I do things
with the node, including starting testpmd on the node". But it could
also be "I have a node, but I also have other non-node resources at my
disposal and it's up to me how I utilize those". If we can make the
imports work then this is likely the best option.

> The current approach appears to me to be top-down instead of bottom-up.
> We take the most abstracted part and we work our way down. But all we
> want is concreteness to the end user (developer).
>
> > Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> >
> > def __init__(self, interactive_session: SSHClient,
> >          logger: DTSLogger,
> >          get_privileged_command: Callable[[str], str] | None,
> >          app_args: EalTestPmdParams | None = None,
> >          timeout: float = SETTINGS.timeout,
> >      ) -> None:
> >      super().__init__(interactive_session, logger, get_privileged_command)
> >      self.state = TestPmdState()
> >
> > Where EalTestPmdParams would be something that enforces that
> > app_args.app_params is of the TestPmdParameters type.
> >
> > But thinking more about this, we're probably better off switching the
> > params composition. Instead of TestPmdParameters being part of
> > EalParameters, we do it the other way around. This way the type of
> > app_args could just be TestPmdParameters and the types should work.
> > Or we pass the args separately, but that would likely require ditching
> > the factories and replacing them with methods (or overloading them).
> >
> > And hopefully the imports won't be impossible to solve. :-)
>
> It is what I feared, and I think it may become even more convoluted. As
> you said, ditching the factories will simplify things and make it more
> straightforward. So, we wouldn't find ourselves in problems like these.
>
> I don't have a strong preference in approach between:
> * overloading node methods
> * dedicated node methods
> * let the shells ingest nodes instead
>
> But if I were to give priority, I'd take it from last to first. Letting
> shells ingest nodes will decouple the situation adding an extra step of
> simplification.

+1 for simplification.

> I may not see the full picture though. The two are
> reasonable but, having a dedicated node method will stop the requirement
> to import the shell we need, and it's pretty much equivalent... but
> overloading also is very new to Python, so I may prefer to stick to more
> established.
>

Let's try shells ingesting nodes if the imports work out then. If not,
we can fall back to dedicated node methods.

> Letting TestPmdParams take EalParams, instead of the other way around,
> would naturally follow the bottom-up approach too. Allowing Params to
> arbitrarily append string arguments – as proposed, would also allow
> users to use a plain (EalParams + string). So sounds like a good
> approach overall.

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Luca Vizzarro]

On 11/04/2024 13:13, Juraj Linkeš wrote:
> The changes we discuss below don't seem that big. What do you think,
> do we just add another patch to the series?

Sure thing, I can take this and add it to v2.

> I thought about this a bit as well, it's a good approach. The current
> design is top-down, as you say, in that "I have a node and I do things
> with the node, including starting testpmd on the node". But it could
> also be "I have a node, but I also have other non-node resources at my
> disposal and it's up to me how I utilize those". If we can make the
> imports work then this is likely the best option.
> 
> <snip>
> 
> +1 for simplification.
>
> <snip> >
> Let's try shells ingesting nodes if the imports work out then. If not,
> we can fall back to dedicated node methods.

Sounds good!

Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

[Juraj Linkeš]

On Wed, Apr 10, 2024 at 4:07 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 14:35, Juraj Linkeš wrote:
> > We should also consider that this would introduce a difference in API
> > between the interactive and non-interactive sessions. Do we want to do
> > this there as well?
>
> Could definitely add it there as well. You are referring to
> RemoteSession I presume, right?

Yes.

Re: [PATCH 4/6] dts: use testpmd params for scatter test suite

[Jeremy Spewock]

On Wed, Apr 10, 2024 at 9:19 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Wed, Apr 10, 2024 at 12:53 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> >
> > On 09/04/2024 20:12, Juraj Linkeš wrote:
> > >> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
> > >>           """
> > >>           testpmd = self.sut_node.create_interactive_shell(
> > >>               TestPmdShell,
> > >> -            app_parameters=StrParams(
> > >> -                "--mbcache=200 "
> > >> -                f"--mbuf-size={mbsize} "
> > >> -                "--max-pkt-len=9000 "
> > >> -                "--port-topology=paired "
> > >> -                "--tx-offloads=0x00008000"
> > >> +            app_parameters=TestPmdParameters(
> > >> +                forward_mode=TestPmdForwardingModes.mac,
> > >> +                mbcache=200,
> > >> +                mbuf_size=[mbsize],
> > >> +                max_pkt_len=9000,
> > >> +                tx_offloads=0x00008000,
> > >>               ),
> > >>               privileged=True,
> > >>           )
> > >> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > >
> > > Jeremy, does this change the test? Instead of configuring the fw mode
> > > after starting testpmd, we're starting testpmd with fw mode
> > > configured.

To my knowledge, as Luca mentions below, this does not functionally
change anything about the test, scatter should just need the MAC
forwarding mode to be set at some point before forwarding starts, it
doesn't technically matter when. One thing to note that this does
change, however, is that we lose the verification step that the method
within testpmd provides. I'm not sure off the top of my head if
testpmd just completely fails to start if the forwarding mode flag is
set and it fails to change modes or if it still starts and then just
goes back to default (io) which would make the test operate in an
invalid state without anyway of knowing.

As another note however, I've never seen a mode change fail and I
don't know what could even make it fail, so this would be a rare thing
anyway, but still just something to consider.


> >
> > I am not Jeremy (please Jeremy still reply), but we discussed this on
> > Slack. Reading through the testpmd source code, setting arguments like
> > forward-mode in the command line, is the exact equivalent of calling
> > `set forward mode` right after start-up. So it is equivalent in theory.
> >
> > > If not, we should remove the testpmd.set_forward_mode method, as it's
> > > not used anymore.
> >
> > Could there be test cases that change the forward mode multiple times in
> > the same shell, though? As this could still be needed to cover this.
>
> Yes, but we don't have such a test now. It's good practice to remove
> unused code. We can still bring it back anytime, it'll be in git
> history.

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Jeremy Spewock]

Apologies for being so late on the discussion, but just a few of my
thoughts:

* I think using something like overloading even though it is new to
python is completely fine because this new python version is a
dependency of  the DTS runner. The DTS runner can have bleeding-edge
requirements because we manage that through a container to make things
easier.

On Thu, Apr 11, 2024 at 8:13 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Thu, Apr 11, 2024 at 1:47 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> >
> > On 11/04/2024 11:30, Juraj Linkeš wrote:
> > > I've been thinking about these interactive shell constructors for some
> > > time and I think the factory pattern is not well suitable for this.
> > > Factories work well with classes with the same API (i.e.
> > > implementations of abstract classes that don't add anything extra),
> > > but are much less useful when dealing with classes with different
> > > behaviors, such as the interactive shells. We see this here, different
> > > apps are going to require different args and that alone kinda breaks
> > > the factory pattern. I think we'll need to either ditch these
> > > factories and instead just have methods that return the proper shell
> > > (and the methods would only exist in classes where they belong, e.g.
> > > testpmd only makes sense on an SUT). Or we could overload each factory
> > > (the support has only been added in 3.11 with @typing.overload, but is
> > > also available in typing_extensions, so we would be able to use it
> > > with the extra dependency) where different signatures would return
> > > different objects. In both cases the caller won't have to import the
> > > class and the method signature is going to be clearer.
> > >
> > > We have this pattern with sut/tg nodes. I decided to move away from
> > > the node factory because it didn't add much and in fact the code was
> > > only clunkier. The interactive shell is not quite the same, as the
> > > shells are not standalone in the same way the nodes are (the shells
> > > are tied to nodes). Let me know what you think about all this - both
> > > Luca and Jeremy.
> >
> > When writing this series, I went down the path of creating a
> > `create_testpmd_shell` method at some point as a solution to these
> > problems. Realising after that it may be too big of a change, and
> > possibly best left to a discussion exactly like this one.
> >
>
> The changes we discuss below don't seem that big. What do you think,
> do we just add another patch to the series?
>
> > Generics used at this level may be a bit too much, especially for
> > Python, as support is not *that* great. I am of the opinion that having
> > a dedicated wrapper is easier for the developer and the user. Generics
> > are not needed to this level anyways, as we have a limited selection of
> > shells that are actually going to be used.
> >
> > We can also swap the wrapping process to simplify things, instead of:
> >
> >    shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)
> >
> > do:
> >
> >    shell = TestPmdShell(self.sut_node, ..)
> >
> > Let the Shell class ingest the node, and not the other way round.
> >
>
> I thought about this a bit as well, it's a good approach. The current
> design is top-down, as you say, in that "I have a node and I do things
> with the node, including starting testpmd on the node". But it could
> also be "I have a node, but I also have other non-node resources at my
> disposal and it's up to me how I utilize those". If we can make the
> imports work then this is likely the best option.

It might be me slightly stuck in the old ways of doing things, but I
might slightly favor the overloading methods approach. This is really
because, at least in my mind, the SUT node is somewhat of a central
API for the developer to use during testing, so having a method on
that API for creating a shell for you to use on the node makes sense
to me. It creates more of a "one stop shop" kind of idea where
developers have to do less reading about how to do things and can just
look at the methods of the SUT node to get what they would need.

That being said, I think in any other framework the passing of the
node into the shell would easily make more sense and I'm not opposed
to going that route either. In general, I agree that not using a
factory with a generic will make things much easier in the future.

>
> > The current approach appears to me to be top-down instead of bottom-up.
> > We take the most abstracted part and we work our way down. But all we
> > want is concreteness to the end user (developer).
> >
> > > Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> > >
> > > def __init__(self, interactive_session: SSHClient,
> > >          logger: DTSLogger,
> > >          get_privileged_command: Callable[[str], str] | None,
> > >          app_args: EalTestPmdParams | None = None,
> > >          timeout: float = SETTINGS.timeout,
> > >      ) -> None:
> > >      super().__init__(interactive_session, logger, get_privileged_command)
> > >      self.state = TestPmdState()
> > >
> > > Where EalTestPmdParams would be something that enforces that
> > > app_args.app_params is of the TestPmdParameters type.
> > >
> > > But thinking more about this, we're probably better off switching the
> > > params composition. Instead of TestPmdParameters being part of
> > > EalParameters, we do it the other way around. This way the type of
> > > app_args could just be TestPmdParameters and the types should work.
> > > Or we pass the args separately, but that would likely require ditching
> > > the factories and replacing them with methods (or overloading them).
> > >
> > > And hopefully the imports won't be impossible to solve. :-)
> >
> > It is what I feared, and I think it may become even more convoluted. As
> > you said, ditching the factories will simplify things and make it more
> > straightforward. So, we wouldn't find ourselves in problems like these.
> >
> > I don't have a strong preference in approach between:
> > * overloading node methods
> > * dedicated node methods
> > * let the shells ingest nodes instead
> >
> > But if I were to give priority, I'd take it from last to first. Letting
> > shells ingest nodes will decouple the situation adding an extra step of
> > simplification.
>
> +1 for simplification.
>
> > I may not see the full picture though. The two are
> > reasonable but, having a dedicated node method will stop the requirement
> > to import the shell we need, and it's pretty much equivalent... but
> > overloading also is very new to Python, so I may prefer to stick to more
> > established.
> >
>
> Let's try shells ingesting nodes if the imports work out then. If not,
> we can fall back to dedicated node methods.
>
> > Letting TestPmdParams take EalParams, instead of the other way around,
> > would naturally follow the bottom-up approach too. Allowing Params to
> > arbitrarily append string arguments – as proposed, would also allow
> > users to use a plain (EalParams + string). So sounds like a good
> > approach overall.

This I like a lot. We don't want to force EalParams to have
TestpmdParams nested inside of them because other DPDK apps might need
them too and this fixes the issue of always having to assert what type
of inner params you have.

Re: [PATCH 4/6] dts: use testpmd params for scatter test suite

[Juraj Linkeš]

On Fri, Apr 26, 2024 at 8:06 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Wed, Apr 10, 2024 at 9:19 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
> >
> > On Wed, Apr 10, 2024 at 12:53 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> > >
> > > On 09/04/2024 20:12, Juraj Linkeš wrote:
> > > >> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
> > > >>           """
> > > >>           testpmd = self.sut_node.create_interactive_shell(
> > > >>               TestPmdShell,
> > > >> -            app_parameters=StrParams(
> > > >> -                "--mbcache=200 "
> > > >> -                f"--mbuf-size={mbsize} "
> > > >> -                "--max-pkt-len=9000 "
> > > >> -                "--port-topology=paired "
> > > >> -                "--tx-offloads=0x00008000"
> > > >> +            app_parameters=TestPmdParameters(
> > > >> +                forward_mode=TestPmdForwardingModes.mac,
> > > >> +                mbcache=200,
> > > >> +                mbuf_size=[mbsize],
> > > >> +                max_pkt_len=9000,
> > > >> +                tx_offloads=0x00008000,
> > > >>               ),
> > > >>               privileged=True,
> > > >>           )
> > > >> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > > >
> > > > Jeremy, does this change the test? Instead of configuring the fw mode
> > > > after starting testpmd, we're starting testpmd with fw mode
> > > > configured.
>
> To my knowledge, as Luca mentions below, this does not functionally
> change anything about the test, scatter should just need the MAC
> forwarding mode to be set at some point before forwarding starts, it
> doesn't technically matter when. One thing to note that this does
> change, however, is that we lose the verification step that the method
> within testpmd provides. I'm not sure off the top of my head if
> testpmd just completely fails to start if the forwarding mode flag is
> set and it fails to change modes or if it still starts and then just
> goes back to default (io) which would make the test operate in an
> invalid state without anyway of knowing.
>
> As another note however, I've never seen a mode change fail and I
> don't know what could even make it fail, so this would be a rare thing
> anyway, but still just something to consider.
>

Ok, thanks. This is fine then. If we see a problem with this when
testpmd starts we can just raise a bug against testpmd (as it should
either start with mac forwarding or error).

>
> > >
> > > I am not Jeremy (please Jeremy still reply), but we discussed this on
> > > Slack. Reading through the testpmd source code, setting arguments like
> > > forward-mode in the command line, is the exact equivalent of calling
> > > `set forward mode` right after start-up. So it is equivalent in theory.
> > >
> > > > If not, we should remove the testpmd.set_forward_mode method, as it's
> > > > not used anymore.
> > >
> > > Could there be test cases that change the forward mode multiple times in
> > > the same shell, though? As this could still be needed to cover this.
> >
> > Yes, but we don't have such a test now. It's good practice to remove
> > unused code. We can still bring it back anytime, it'll be in git
> > history.

Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Juraj Linkeš]

On Fri, Apr 26, 2024 at 8:06 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> Apologies for being so late on the discussion, but just a few of my
> thoughts:
>
> * I think using something like overloading even though it is new to
> python is completely fine because this new python version is a
> dependency of  the DTS runner. The DTS runner can have bleeding-edge
> requirements because we manage that through a container to make things
> easier.
>
> On Thu, Apr 11, 2024 at 8:13 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
> >
> > On Thu, Apr 11, 2024 at 1:47 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> > >
> > > On 11/04/2024 11:30, Juraj Linkeš wrote:
> > > > I've been thinking about these interactive shell constructors for some
> > > > time and I think the factory pattern is not well suitable for this.
> > > > Factories work well with classes with the same API (i.e.
> > > > implementations of abstract classes that don't add anything extra),
> > > > but are much less useful when dealing with classes with different
> > > > behaviors, such as the interactive shells. We see this here, different
> > > > apps are going to require different args and that alone kinda breaks
> > > > the factory pattern. I think we'll need to either ditch these
> > > > factories and instead just have methods that return the proper shell
> > > > (and the methods would only exist in classes where they belong, e.g.
> > > > testpmd only makes sense on an SUT). Or we could overload each factory
> > > > (the support has only been added in 3.11 with @typing.overload, but is
> > > > also available in typing_extensions, so we would be able to use it
> > > > with the extra dependency) where different signatures would return
> > > > different objects. In both cases the caller won't have to import the
> > > > class and the method signature is going to be clearer.
> > > >
> > > > We have this pattern with sut/tg nodes. I decided to move away from
> > > > the node factory because it didn't add much and in fact the code was
> > > > only clunkier. The interactive shell is not quite the same, as the
> > > > shells are not standalone in the same way the nodes are (the shells
> > > > are tied to nodes). Let me know what you think about all this - both
> > > > Luca and Jeremy.
> > >
> > > When writing this series, I went down the path of creating a
> > > `create_testpmd_shell` method at some point as a solution to these
> > > problems. Realising after that it may be too big of a change, and
> > > possibly best left to a discussion exactly like this one.
> > >
> >
> > The changes we discuss below don't seem that big. What do you think,
> > do we just add another patch to the series?
> >
> > > Generics used at this level may be a bit too much, especially for
> > > Python, as support is not *that* great. I am of the opinion that having
> > > a dedicated wrapper is easier for the developer and the user. Generics
> > > are not needed to this level anyways, as we have a limited selection of
> > > shells that are actually going to be used.
> > >
> > > We can also swap the wrapping process to simplify things, instead of:
> > >
> > >    shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)
> > >
> > > do:
> > >
> > >    shell = TestPmdShell(self.sut_node, ..)
> > >
> > > Let the Shell class ingest the node, and not the other way round.
> > >
> >
> > I thought about this a bit as well, it's a good approach. The current
> > design is top-down, as you say, in that "I have a node and I do things
> > with the node, including starting testpmd on the node". But it could
> > also be "I have a node, but I also have other non-node resources at my
> > disposal and it's up to me how I utilize those". If we can make the
> > imports work then this is likely the best option.
>
> It might be me slightly stuck in the old ways of doing things, but I
> might slightly favor the overloading methods approach. This is really
> because, at least in my mind, the SUT node is somewhat of a central
> API for the developer to use during testing, so having a method on
> that API for creating a shell for you to use on the node makes sense
> to me. It creates more of a "one stop shop" kind of idea where
> developers have to do less reading about how to do things and can just
> look at the methods of the SUT node to get what they would need.
>

This was the case before we introduced the testpmd shell, which is a
standalone object (as in the dev uses the sut node and the test pmd
object to do what they need). One advantage of using sut methods to
instantiate testpmd shells is that devs won't need to import the
TestPmd shell, but I don't know which of these is going to be better.

> That being said, I think in any other framework the passing of the
> node into the shell would easily make more sense and I'm not opposed
> to going that route either. In general, I agree that not using a
> factory with a generic will make things much easier in the future.
>
> >
> > > The current approach appears to me to be top-down instead of bottom-up.
> > > We take the most abstracted part and we work our way down. But all we
> > > want is concreteness to the end user (developer).
> > >
> > > > Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> > > >
> > > > def __init__(self, interactive_session: SSHClient,
> > > >          logger: DTSLogger,
> > > >          get_privileged_command: Callable[[str], str] | None,
> > > >          app_args: EalTestPmdParams | None = None,
> > > >          timeout: float = SETTINGS.timeout,
> > > >      ) -> None:
> > > >      super().__init__(interactive_session, logger, get_privileged_command)
> > > >      self.state = TestPmdState()
> > > >
> > > > Where EalTestPmdParams would be something that enforces that
> > > > app_args.app_params is of the TestPmdParameters type.
> > > >
> > > > But thinking more about this, we're probably better off switching the
> > > > params composition. Instead of TestPmdParameters being part of
> > > > EalParameters, we do it the other way around. This way the type of
> > > > app_args could just be TestPmdParameters and the types should work.
> > > > Or we pass the args separately, but that would likely require ditching
> > > > the factories and replacing them with methods (or overloading them).
> > > >
> > > > And hopefully the imports won't be impossible to solve. :-)
> > >
> > > It is what I feared, and I think it may become even more convoluted. As
> > > you said, ditching the factories will simplify things and make it more
> > > straightforward. So, we wouldn't find ourselves in problems like these.
> > >
> > > I don't have a strong preference in approach between:
> > > * overloading node methods
> > > * dedicated node methods
> > > * let the shells ingest nodes instead
> > >
> > > But if I were to give priority, I'd take it from last to first. Letting
> > > shells ingest nodes will decouple the situation adding an extra step of
> > > simplification.
> >
> > +1 for simplification.
> >
> > > I may not see the full picture though. The two are
> > > reasonable but, having a dedicated node method will stop the requirement
> > > to import the shell we need, and it's pretty much equivalent... but
> > > overloading also is very new to Python, so I may prefer to stick to more
> > > established.
> > >
> >
> > Let's try shells ingesting nodes if the imports work out then. If not,
> > we can fall back to dedicated node methods.
> >
> > > Letting TestPmdParams take EalParams, instead of the other way around,
> > > would naturally follow the bottom-up approach too. Allowing Params to
> > > arbitrarily append string arguments – as proposed, would also allow
> > > users to use a plain (EalParams + string). So sounds like a good
> > > approach overall.
>
> This I like a lot. We don't want to force EalParams to have
> TestpmdParams nested inside of them because other DPDK apps might need
> them too and this fixes the issue of always having to assert what type
> of inner params you have.

Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

[Jeremy Spewock]

On Wed, Apr 10, 2024 at 9:36 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Wed, Apr 10, 2024 at 1:27 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> >
> > On 10/04/2024 07:53, Juraj Linkeš wrote:
> > > I have a general question. What are these changes for? Do you
> > > anticipate us needing this in the future? Wouldn't it be better to add
> > > it only when we need it?
> >
> > It's been sometime since we raised this task internally. This patch and
> > the next one arise from some survey done on old DTS test cases.
> > Unfortunately, I can't pinpoint.
> >
> > Specifically for this patch though, the timeout bit is useful in
> > conjunction with the related change in the next. Instead of giving an
> > optional timeout argument to all the commands where we may want to
> > change it, aren't we better off with providing a facility to temporarily
> > change this for the current scope?
> >
>
> This is a good question. If the scope is just one command, then no. If
> it's more than one, then maybe yes. I don't know which is better.
>
> We should also consider that this would introduce a difference in API
> between the interactive and non-interactive sessions. Do we want to do
> this there as well?

I believe there already is a difference in this case since the
interactive shell doesn't support modification of timeout on a
per-command basis. This is mainly because the way interactive shells
handle timeouts is on a lower level than sending a command using
fabric. Currently the interactive shells are modifying the timeout on
the channel of the connection, whereas fabric supports a keyword
argument that can modify timeouts on a per-command basis.

Of course we could also change the interactive shell send_command to
modify the timeout of the shell, but something else to note here is
that changing the timeout of the channel of the connection is slightly
different than giving a timeout for a command. This is because when
you change the timeout of the channel you're setting the timeout for
read/write operations on that channel. So, if you send a command and
give a timeout of 5 seconds for example, as long as you are receiving
output from the shell at least every 5 seconds, the command actually
wouldn't ever timeout. If we want to make the interactive shell
support passing a timeout per command, I would recommend we do it in a
different way that is more representative of your *command* having a
timeout instead of the shell's channel having a timeout.

Waiting for the status of a link to be "up" in testpmd was an
exception where I did allow you to give a specific timeout for the
command and this is exactly for the reason above. I wanted to make
sure that the user was able to specify how long they wanted to wait
for this status to be what they expect as opposed to how long to wait
for getting output from the channel. This is not supported in any
other method of the interactive shell.

>
> Also, maybe set_timeout should be a property or we could just make
> _timeout public.
> And is_privileged should just be privileged, as it's a property (which
> shouldn't contain a verb; if it was a method it would be a good name).
<snip>
> >