Provided a review for the wrong version... Tested-by: Nicholas Pratte <npra...@iol.unh.edu> Reviewed-by: Nicholas Pratte <npra...@iol.unh.edu>
On Thu, May 9, 2024 at 7:21 AM Luca Vizzarro <luca.vizza...@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 > `Params.from_str()` method. > > Signed-off-by: Luca Vizzarro <luca.vizza...@arm.com> > Reviewed-by: Paul Szczepanek <paul.szczepa...@arm.com> > --- > .../remote_session/interactive_shell.py | 12 +- > dts/framework/remote_session/testpmd_shell.py | 11 +- > dts/framework/testbed_model/node.py | 6 +- > dts/framework/testbed_model/os_session.py | 4 +- > dts/framework/testbed_model/sut_node.py | 124 ++++++++---------- > dts/tests/TestSuite_pmd_buffer_scatter.py | 3 +- > 6 files changed, 77 insertions(+), 83 deletions(-) > > diff --git a/dts/framework/remote_session/interactive_shell.py > b/dts/framework/remote_session/interactive_shell.py > index 074a541279..9da66d1c7e 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-untyped] > > 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_params: Params > > #: 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_params: Params = Params(), > timeout: float = SETTINGS.timeout, > ) -> None: > """Create an SSH channel during initialization. > @@ -74,7 +76,7 @@ def __init__( > get_privileged_command: A method for modifying a command to > allow it to use > elevated privileges. If :data:`None`, the application will > not be started > with elevated privileges. > - app_args: The command line arguments to be passed to the > application on startup. > + app_params: The command line parameters to be passed to the > application on startup. > timeout: The timeout used for the SSH channel that is dedicated > to this interactive > shell. This timeout is for collecting output, so if reading > from the buffer > and no output is gathered within the timeout, an exception > is thrown. > @@ -87,7 +89,7 @@ def __init__( > self._ssh_channel.set_combine_stderr(True) # combines stdout and > stderr streams > self._logger = logger > self._timeout = timeout > - self._app_args = app_args > + self._app_params = app_params > self._start_application(get_privileged_command) > > def _start_application(self, get_privileged_command: Callable[[str], > str] | None) -> None: > @@ -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_params}" > 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..7eced27096 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -22,6 +22,7 @@ > > from framework.exception import InteractiveCommandExecutionError > from framework.settings import SETTINGS > +from framework.testbed_model.sut_node import EalParams > from framework.utils import StrEnum > > from .interactive_shell import InteractiveShell > @@ -118,8 +119,14 @@ 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 ") > + self._app_params += " -i --mask-event intr_lsc" > + > + assert isinstance(self._app_params, EalParams) > + > + self.number_of_ports = ( > + len(self._app_params.ports) if self._app_params.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/node.py > b/dts/framework/testbed_model/node.py > index 74061f6262..6af4f25a3c 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_params: Params = Params(), > ) -> InteractiveShellType: > """Factory for interactive session handlers. > > @@ -222,7 +224,7 @@ def create_interactive_shell( > shell_cls, > timeout, > privileged, > - app_args, > + app_params, > ) > > def filter_lcores( > diff --git a/dts/framework/testbed_model/os_session.py > b/dts/framework/testbed_model/os_session.py > index d5bf7e0401..1a77aee532 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, > ) -> 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..c886590979 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. > > @@ -14,8 +15,9 @@ > import os > import tarfile > import time > +from dataclasses import dataclass, field > from pathlib import PurePath > -from typing import Type > +from typing import Literal, Type > > from framework.config import ( > BuildTargetConfiguration, > @@ -23,6 +25,7 @@ > NodeInfo, > SutNodeConfiguration, > ) > +from framework.params import Params, Switch > from framework.remote_session import CommandResult > from framework.settings import SETTINGS > from framework.utils import MesonArgs > @@ -34,62 +37,42 @@ > from .virtual_device import VirtualDevice > > > -class EalParameters(object): > - """The environment abstraction layer parameters. > - > - The string representation can be created by converting the instance to a > string. > - """ > +def _port_to_pci(port: Port) -> str: > + return port.pci > > - 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. > > - 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.:: > +@dataclass(kw_only=True) > +class EalParams(Params): > + """The environment abstraction layer parameters. > > - vdevs=[ > - VirtualDevice('net_ring0'), > - VirtualDevice('net_ring1') > - ] > - ports: The list of ports to allow. > - other_eal_param: user defined DPDK EAL parameters, e.g.: > + Attributes: > + 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.:: > + 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}" > - ) > + """ > + > + lcore_list: LogicalCoreList = field(metadata=Params.short("l")) > + memory_channels: int = field(metadata=Params.short("n")) > + prefix: str = field(metadata=Params.long("file-prefix")) > + no_pci: Switch > + vdevs: list[VirtualDevice] | None = field( > + default=None, metadata=Params.multiple() | Params.long("vdev") > + ) > + ports: list[Port] | None = field( > + default=None, > + metadata=Params.convert_value(_port_to_pci) | Params.multiple() | > Params.short("a"), > + ) > + other_eal_param: Params | None = None > + _separator: Literal[True] = field(default=True, init=False, > metadata=Params.short("-")) > > > class SutNode(Node): > @@ -350,11 +333,11 @@ def create_eal_parameters( > ascending_cores: bool = True, > prefix: str = "dpdk", > append_prefix_timestamp: bool = True, > - no_pci: bool = False, > + no_pci: Switch = None, > vdevs: list[VirtualDevice] | None = None, > ports: list[Port] | None = None, > other_eal_param: str = "", > - ) -> "EalParameters": > + ) -> EalParams: > """Compose the EAL parameters. > > Process the list of cores and the DPDK prefix and pass that along > with > @@ -393,24 +376,21 @@ def create_eal_parameters( > if prefix: > self._dpdk_prefix_list.append(prefix) > > - if vdevs is None: > - vdevs = [] > - > if ports is None: > ports = self.ports > > - return EalParameters( > + return EalParams( > lcore_list=lcore_list, > memory_channels=self.config.memory_channels, > prefix=prefix, > no_pci=no_pci, > vdevs=vdevs, > ports=ports, > - other_eal_param=other_eal_param, > + other_eal_param=Params.from_str(other_eal_param), > ) > > def run_dpdk_app( > - self, app_path: PurePath, eal_args: "EalParameters", timeout: float > = 30 > + self, app_path: PurePath, eal_params: EalParams, timeout: float = 30 > ) -> CommandResult: > """Run DPDK application on the remote node. > > @@ -419,14 +399,14 @@ def run_dpdk_app( > > Args: > app_path: The remote path to the DPDK application. > - eal_args: EAL parameters to run the DPDK application with. > + eal_params: EAL parameters to run the DPDK application with. > timeout: Wait at most this long in seconds for `command` > execution to complete. > > Returns: > The result of the DPDK app execution. > """ > return self.main_session.send_command( > - f"{app_path} {eal_args}", timeout, privileged=True, verify=True > + f"{app_path} {eal_params}", timeout, privileged=True, verify=True > ) > > def configure_ipv4_forwarding(self, enable: bool) -> None: > @@ -442,8 +422,8 @@ def create_interactive_shell( > shell_cls: Type[InteractiveShellType], > timeout: float = SETTINGS.timeout, > privileged: bool = False, > - app_parameters: str = "", > - eal_parameters: EalParameters | None = None, > + app_params: Params = Params(), > + eal_params: EalParams | None = None, > ) -> InteractiveShellType: > """Extend the factory for interactive session handlers. > > @@ -459,26 +439,26 @@ 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. > - eal_parameters: List of EAL parameters to use to launch the app. > If this > + app_params: The parameters to be passed to the application. > + eal_params: 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`. > - app_parameters: Additional arguments to pass into the > application on the > - command-line. > > Returns: > An instance of the desired interactive application shell. > """ > # We need to append the build directory and add EAL parameters for > DPDK apps > if shell_cls.dpdk_app: > - if not eal_parameters: > - eal_parameters = self.create_eal_parameters() > - app_parameters = f"{eal_parameters} -- {app_parameters}" > + if eal_params is None: > + eal_params = self.create_eal_parameters() > + eal_params.append_str(str(app_params)) > + app_params = eal_params > > shell_cls.path = self.main_session.join_remote_path( > self.remote_dpdk_build_dir, shell_cls.path > ) > > - return super().create_interactive_shell(shell_cls, timeout, > privileged, app_parameters) > + return super().create_interactive_shell(shell_cls, timeout, > privileged, app_params) > > def bind_ports_to_driver(self, for_dpdk: bool = True) -> None: > """Bind all ports on the SUT to a driver. > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py > b/dts/tests/TestSuite_pmd_buffer_scatter.py > index a020682e8d..c6e93839cb 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-untyped] > from scapy.utils import hexstr # type: ignore[import-untyped] > > +from framework.params import Params > 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_params=Params.from_str( > "--mbcache=200 " > f"--mbuf-size={mbsize} " > "--max-pkt-len=9000 " > -- > 2.34.1 >