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

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

On Thu, Jul 11, 2024 at 10:35 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
>
> 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.
>

This is a good point. Originally I also did it in two separate lines
because it used to have to be a call to a sut_node method and was just
long and convoluted, but I think now that you instantiate the class
directly doing it this way makes more sense. I have no problem with
updating both of them.

Just as one thing to note however, this context manager is a little
different by design. When writing it I actually made some minor tweaks
specifically so that the same instance could be used multiple times. I
figured this was something that we didn't really need to use and
probably wouldn't use often, but could be useful in the future if you
needed a shell that was identical ("identical" as in parameters-wise,
of course the instances would be different) across test cases since
all leaving the context does is close the shell.

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