On Thu, Jul 11, 2024 at 10:35 AM Juraj Linkeš
<juraj.lin...@pantheon.tech> wrote:
>
>
>
> On 9. 7. 2024 18:31, jspew...@iol.unh.edu wrote:
> > From: Jeremy Spewock <jspew...@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 <jspew...@iol.unh.edu>
> > Reviewed-by: Juraj Linkeš <juraj.lin...@pantheon.tech>
> > Reviewed-by: Patrick Robb <pr...@iol.unh.edu>
> > Reviewed-by: Luca Vizzarro <luca.vizza...@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,
