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

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

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