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

Message ID 20240326190422.577028-3-luca.vizzarro@arm.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series dts: add testpmd params and statefulness |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Luca Vizzarro March 26, 2024, 7:04 p.m. UTC
  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(-)
  

Comments

Jeremy Spewock March 28, 2024, 4:48 p.m. UTC | #1
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
>
  
Juraj Linkeš April 9, 2024, 2:56 p.m. UTC | #2
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
> >
  
Luca Vizzarro April 10, 2024, 9:34 a.m. UTC | #3
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
>>>
  
Juraj Linkeš April 10, 2024, 9:58 a.m. UTC | #4
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
> >>>
>
  
Nicholas Pratte May 28, 2024, 3:43 p.m. UTC | #5
Tested-by: Nicholas Pratte <npratte@iol.unh.edu>
Reviewed-by: Nicholas Pratte <npratte@iol.unh.edu>

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>
> ---
>  .../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

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 "