Re: [PATCH v1 1/2] dts: add context manager for interactive shells

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

On 9. 7. 2024 18:31, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> Interactive shells are managed in a way currently where they are closed
> and cleaned up at the time of garbage collection. Due to there being no
> guarantee of when this garbage collection happens in Python, there is no
> way to consistently know when an application will be closed without
> manually closing the application yourself when you are done with it.
> This doesn't cause a problem in cases where you can start another
> instance of the same application multiple times on a server, but this
> isn't the case for primary applications in DPDK. The introduction of
> primary applications, such as testpmd, adds a need for knowing previous
> instances of the application have been stopped and cleaned up before
> starting a new one, which the garbage collector does not provide.
> 
> To solve this problem, a new class is added which acts as a base class
> for interactive shells that enforces that instances of the
> application be managed using a context manager. Using a context manager
> guarantees that once you leave the scope of the block where the
> application is being used for any reason, the application will be closed
> immediately. This avoids the possibility of the shell not being closed
> due to an exception being raised or user error. The interactive shell
> class then becomes shells that can be started/stopped manually or at the
> time of garbage collection rather than through a context manager.
> 
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Reviewed-by: Patrick Robb <probb@iol.unh.edu>
> Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
> ---

Just one minor inconsequential point below. My tag is still valid.

> diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
> index eca27acfd8..377bff129d 100644
> --- a/dts/tests/TestSuite_smoke_tests.py
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -100,7 +100,8 @@ def test_devices_listed_in_testpmd(self) -> None:
>               List all devices found in testpmd and verify the configured devices are among them.
>           """
>           testpmd_driver = TestPmdShell(self.sut_node)
> -        dev_list = [str(x) for x in testpmd_driver.get_devices()]
> +        with testpmd_driver as testpmd:

The usual way to use context managers in Python is without the intent of 
using the object after it leaves the context:

with TestPmdShell(self.sut_node) as testpmd:

That said, the way you did it in the scatter test case seems fine 
because it looks more readable. Maybe we can just change it here, but 
it's a minor point and doesn't really matter.

> +            dev_list = [str(x) for x in testpmd.get_devices()]
>           for nic in self.nics_in_node:
>               self.verify(
>                   nic.pci in dev_list,