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

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

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")