diff mbox series

[5/6] dts: add statefulness to InteractiveShell

Message ID 20240326190422.577028-6-luca.vizzarro@arm.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series dts: add testpmd params and statefulness |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Luca Vizzarro March 26, 2024, 7:04 p.m. UTC 
  The InteractiveShell class can be started in privileged mode, but this
is not saved for reference to the tests developer. Moreover, originally
a command timeout could only be set at initialisation, this can now be
amended and reset back as needed.

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>
---
 .../remote_session/interactive_shell.py        | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Jeremy Spewock March 28, 2024, 4:48 p.m. UTC | #1 
On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
<snip>
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index a2c7b30d9f..5d80061e8d 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
>      _stdout: channel.ChannelFile
>      _ssh_channel: Channel
>      _logger: DTSLogger
> +    __default_timeout: float

Only single underscores are used for other private variables, probably
better to keep that consistent with this one.

>      _timeout: float
>      _app_args: Params | None
> +    _is_privileged: bool = False
<snip>
> 2.34.1
>
Juraj Linkeš April 10, 2024, 6:53 a.m. UTC | #2 
I have a general question. What are these changes for? Do you
anticipate us needing this in the future? Wouldn't it be better to add
it only when we need it?

On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> <snip>
> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> > index a2c7b30d9f..5d80061e8d 100644
> > --- a/dts/framework/remote_session/interactive_shell.py
> > +++ b/dts/framework/remote_session/interactive_shell.py
> > @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
> >      _stdout: channel.ChannelFile
> >      _ssh_channel: Channel
> >      _logger: DTSLogger
> > +    __default_timeout: float
>
> Only single underscores are used for other private variables, probably
> better to keep that consistent with this one.
>

I agree, I don't see a reason for the double underscore.

> >      _timeout: float
> >      _app_args: Params | None
> > +    _is_privileged: bool = False
> <snip>
> > 2.34.1
> >
Luca Vizzarro April 10, 2024, 11:27 a.m. UTC | #3 
On 10/04/2024 07:53, Juraj Linkeš wrote:
> I have a general question. What are these changes for? Do you
> anticipate us needing this in the future? Wouldn't it be better to add
> it only when we need it?

It's been sometime since we raised this task internally. This patch and 
the next one arise from some survey done on old DTS test cases.
Unfortunately, I can't pinpoint.

Specifically for this patch though, the timeout bit is useful in 
conjunction with the related change in the next. Instead of giving an 
optional timeout argument to all the commands where we may want to 
change it, aren't we better off with providing a facility to temporarily 
change this for the current scope?

> 
> On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>>
>> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>> <snip>
>>> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
>>> index a2c7b30d9f..5d80061e8d 100644
>>> --- a/dts/framework/remote_session/interactive_shell.py
>>> +++ b/dts/framework/remote_session/interactive_shell.py
>>> @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
>>>       _stdout: channel.ChannelFile
>>>       _ssh_channel: Channel
>>>       _logger: DTSLogger
>>> +    __default_timeout: float
>>
>> Only single underscores are used for other private variables, probably
>> better to keep that consistent with this one.
>>
> 
> I agree, I don't see a reason for the double underscore.

Ack.

> 
>>>       _timeout: float
>>>       _app_args: Params | None
>>> +    _is_privileged: bool = False
>> <snip>
>>> 2.34.1
>>>
Juraj Linkeš April 10, 2024, 1:35 p.m. UTC | #4 
On Wed, Apr 10, 2024 at 1:27 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 07:53, Juraj Linkeš wrote:
> > I have a general question. What are these changes for? Do you
> > anticipate us needing this in the future? Wouldn't it be better to add
> > it only when we need it?
>
> It's been sometime since we raised this task internally. This patch and
> the next one arise from some survey done on old DTS test cases.
> Unfortunately, I can't pinpoint.
>
> Specifically for this patch though, the timeout bit is useful in
> conjunction with the related change in the next. Instead of giving an
> optional timeout argument to all the commands where we may want to
> change it, aren't we better off with providing a facility to temporarily
> change this for the current scope?
>

This is a good question. If the scope is just one command, then no. If
it's more than one, then maybe yes. I don't know which is better.

We should also consider that this would introduce a difference in API
between the interactive and non-interactive sessions. Do we want to do
this there as well?

Also, maybe set_timeout should be a property or we could just make
_timeout public.
And is_privileged should just be privileged, as it's a property (which
shouldn't contain a verb; if it was a method it would be a good name).

> >
> > On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
> >>
> >> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >> <snip>
> >>> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> >>> index a2c7b30d9f..5d80061e8d 100644
> >>> --- a/dts/framework/remote_session/interactive_shell.py
> >>> +++ b/dts/framework/remote_session/interactive_shell.py
> >>> @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
> >>>       _stdout: channel.ChannelFile
> >>>       _ssh_channel: Channel
> >>>       _logger: DTSLogger
> >>> +    __default_timeout: float
> >>
> >> Only single underscores are used for other private variables, probably
> >> better to keep that consistent with this one.
> >>
> >
> > I agree, I don't see a reason for the double underscore.
>
> Ack.
>
> >
> >>>       _timeout: float
> >>>       _app_args: Params | None
> >>> +    _is_privileged: bool = False
> >> <snip>
> >>> 2.34.1
> >>>
>
Luca Vizzarro April 10, 2024, 2:07 p.m. UTC | #5 
On 10/04/2024 14:35, Juraj Linkeš wrote:
> We should also consider that this would introduce a difference in API
> between the interactive and non-interactive sessions. Do we want to do
> this there as well?

Could definitely add it there as well. You are referring to 
RemoteSession I presume, right?
Juraj Linkeš April 12, 2024, 12:33 p.m. UTC | #6 
On Wed, Apr 10, 2024 at 4:07 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 14:35, Juraj Linkeš wrote:
> > We should also consider that this would introduce a difference in API
> > between the interactive and non-interactive sessions. Do we want to do
> > this there as well?
>
> Could definitely add it there as well. You are referring to
> RemoteSession I presume, right?

Yes.
diff mbox series

Patch

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index a2c7b30d9f..5d80061e8d 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -41,8 +41,10 @@  class InteractiveShell(ABC):
     _stdout: channel.ChannelFile
     _ssh_channel: Channel
     _logger: DTSLogger
+    __default_timeout: float
     _timeout: float
     _app_args: Params | None
+    _is_privileged: bool = False
 
     #: Prompt to expect at the end of output when sending a command.
     #: This is often overridden by subclasses.
@@ -88,7 +90,7 @@  def __init__(
         self._ssh_channel.settimeout(timeout)
         self._ssh_channel.set_combine_stderr(True)  # combines stdout and stderr streams
         self._logger = logger
-        self._timeout = timeout
+        self._timeout = self.__default_timeout = timeout
         self._app_args = app_args
         self._start_application(get_privileged_command)
 
@@ -105,6 +107,7 @@  def _start_application(self, get_privileged_command: Callable[[str], str] | None
         start_command = f"{self.path} {self._app_args or ''}"
         if get_privileged_command is not None:
             start_command = get_privileged_command(start_command)
+            self._is_privileged = True
         self.send_command(start_command)
 
     def send_command(self, command: str, prompt: str | None = None) -> str:
@@ -150,3 +153,16 @@  def close(self) -> None:
     def __del__(self) -> None:
         """Make sure the session is properly closed before deleting the object."""
         self.close()
+
+    @property
+    def is_privileged(self) -> bool:
+        """Property specifying if the interactive shell is running in privileged mode."""
+        return self._is_privileged
+
+    def set_timeout(self, timeout: float):
+        """Set the timeout to use with the next commands."""
+        self._timeout = timeout
+
+    def reset_timeout(self):
+        """Reset the timeout to the default setting."""
+        self._timeout = self.__default_timeout