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

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

On Mon, May 20, 2024 at 1:31 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 14/05/2024 21:14, jspewock@iol.unh.edu wrote:
> > +class CriticalInteractiveShell(InteractiveShell):
> <snip>
> > +    _get_priviledged_command: Callable[[str], str] | None
> typo: privileged

Ack.

> > +
> > +    def __init__(
> > +        self,
> > +        interactive_session: SSHClient,
> > +        logger: DTSLogger,
> > +        get_privileged_command: Callable[[str], str] | None,
> > +        app_args: str = "",
> > +        timeout: float = SETTINGS.timeout,
> > +    ) -> None:
> > +        """Store parameters for creating an interactive shell, but do not start the application.
> > +
> > +        Note that this method also does not create the channel for the application, as this is
> > +        something that isn't needed until the application starts.
> > +
> > +        Args:
> > +            interactive_session: The SSH session dedicated to interactive shells.
> > +            logger: The logger instance this session will use.
> > +            get_privileged_command: A method for modifying a command to allow it to use
> > +                elevated privileges. If :data:`None`, the application will not be started
> > +                with elevated privileges.
> > +            app_args: The command line arguments to be passed to the application on startup.
> > +            timeout: The timeout used for the SSH channel that is dedicated to this interactive
> > +                shell. This timeout is for collecting output, so if reading from the buffer
> > +                and no output is gathered within the timeout, an exception is thrown. The default
> > +                value for this argument may be modified using the :option:`--timeout` command-line
> > +                argument or the :envvar:`DTS_TIMEOUT` environment variable.
> > +        """
> > +        self._interactive_session = interactive_session
> > +        self._logger = logger
> > +        self._timeout = timeout
> > +        self._app_args = app_args
> > +        self._get_priviledged_command = get_privileged_command
> > +
> > +    def __enter__(self: CriticalInteractiveShellType) -> CriticalInteractiveShellType:
>
> This kind of type hinting is achievable with Python's own `Self`, which
> you can already add to this patch because mypy installs
> typing_extensions. Nevertheless, `Self` is also being introduced
> properly in my mypy update patch series.

This is a great point. I actually didn't know the `Self` annotation
was a thing at the time of writing, but saw you using it in some of
your own patches. I think that would fit much better here and will
make the change.

>
> > +
> > +    def __exit__(self, type: BaseException, value: BaseException, traceback: TracebackType) -> None:
> Since you are not using the arguments I'd just ignore them with `_`.

Good point. I initially left them in case there was some kind of need
for using them somehow in the future, but there really is no need for
them.

> > +        """Exit the context block.
> > +
> > +        Upon exiting a context block with this class, we want to ensure that the instance of the
> > +        application is explicitly closed and properly cleaned up using it's close method. Note that
> > +        because this method returns :data:`None` if an exception was raised within the block, it is
> > +        not handled and will be re-raised after the application is closed.
> > +
> > +        Args:
> > +            type: Type of exception that was thrown in the context block if there was one.
> > +            value: Value of the exception thrown in the context block if there was one.
> > +            traceback: Traceback of the exception thrown in the context block if there was one.
> > +        """
> > +        self.close()
>
> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> > index d1a9d8a6d2..08b8ba6a3e 100644
> > --- a/dts/framework/remote_session/interactive_shell.py
> > +++ b/dts/framework/remote_session/interactive_shell.py
> > @@ -89,16 +89,19 @@ def __init__(
> >                   and no output is gathered within the timeout, an exception is thrown.
> >           """
> >           self._interactive_session = interactive_session
> > -        self._ssh_channel = self._interactive_session.invoke_shell()
> > -        self._stdin = self._ssh_channel.makefile_stdin("w")
> > -        self._stdout = self._ssh_channel.makefile("r")
> > -        self._ssh_channel.settimeout(timeout)
> > -        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
> >           self._logger = logger
> >           self._timeout = timeout
> >           self._app_args = app_args
> > +        self._init_channel()
> >           self._start_application(get_privileged_command)
> >
> > +    def _init_channel(self):
> > +        self._ssh_channel = self._interactive_session.invoke_shell()
> > +        self._stdin = self._ssh_channel.makefile_stdin("w")
> > +        self._stdout = self._ssh_channel.makefile("r")
> > +        self._ssh_channel.settimeout(self._timeout)
> > +        self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
> > +
> >       def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None:
> >           """Starts a new interactive application based on the path to the app.
> >
> Hilariously I've made the same exact change in my testpmd params patch
> series!

I noticed this as well, it is very funny that we both thought to break
this into its own method. Must be a useful addition then!

> > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > index 3701c47408..41f6090a7e 100644
> > --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > @@ -112,17 +112,21 @@ def pmd_scatter(self, mbsize: int) -> None:
> >               ),
> >               privileged=True,
> >           )
> > -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > -        testpmd.start()
> > -
> > -        for offset in [-1, 0, 1, 4, 5]:
> > -            recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
> > -            self._logger.debug(f"Payload of scattered packet after forwarding: \n{recv_payload}")
> > -            self.verify(
> > -                ("58 " * 8).strip() in recv_payload,
> > -                f"Payload of scattered packet did not match expected payload with offset {offset}.",
> > -            )
> > -        testpmd.stop()
> > +        with testpmd_shell as testpmd:
> > +            testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > +            testpmd.start()
> > +
> > +            for offset in [-1, 0, 1, 4, 5]:
> > +                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
> > +                self._logger.debug(
> > +                    f"Payload of scattered packet after forwarding: \n{recv_payload}"
> > +                )
> > +                self.verify(
> > +                    ("58 " * 8).strip() in recv_payload,
> > +                    "Payload of scattered packet did not match expected payload with offset "
> > +                    f"{offset}.",
> > +                )
> > +            testpmd.stop()
>
> Since we are exiting the context, implicitly it means we want to stop
> and close. Can't we also implicit call stop when closing?

It wouldn't hurt to also stop when we close I suppose. I really just
left close as something explicit because start is also something we do
more explicitly. When you quit testpmd in the close method it's going
to stop forwarding no matter what anyway, so I guess this isn't really
needed in the first place. Regardless, I can add something that stops
for you before quitting out of testpmd in the exit method, that will
at least save us some extra lines.