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

[Jeremy Spewock <jspewock@iol.unh.edu>]

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
>