From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 40265440FA; Tue, 28 May 2024 23:07:42 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 22D4440691; Tue, 28 May 2024 23:07:42 +0200 (CEST) Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com [209.85.208.181]) by mails.dpdk.org (Postfix) with ESMTP id 8C6FA40691 for ; Tue, 28 May 2024 23:07:40 +0200 (CEST) Received: by mail-lj1-f181.google.com with SMTP id 38308e7fff4ca-2e716e3030aso12628141fa.2 for ; Tue, 28 May 2024 14:07:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1716930460; x=1717535260; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=4hZQbyPOZwCZLa/faZTh+Qiy2WNB8A8Qx1Ss5cB9NVw=; b=VgQ+SUMsv1Ugvxwy1GOIbtzituLV7U5cYUpYsrB5w2Ub+Tg9oJdKwv3PpTF680FJAj HZB1Yuw3RxllyKUkxFyXl89NdjgFkmS9jTX8dxwm6cUHzbeHWMAssVbcbQjad2vGAvOz 3hFKBU/n7L0tLWhyMLVCMYHvRNYX/yH6hXOn4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716930460; x=1717535260; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4hZQbyPOZwCZLa/faZTh+Qiy2WNB8A8Qx1Ss5cB9NVw=; b=nCwaWLYhtMm6veosSPsbI9x5JRjmu7WFYpkycKjhozADjH2x+we1LuCxNfpVbIKfGm caFYYJpH1udCPFT40NYCTjnjRYg01yDdiV1zSxh7tRj9/Lk1S0JrV/hYyb6Wdhl0aTRD iehIRjjQEdUWQUdtQZBk6J2TTbz4aoxhmimQxmP4TPIeAczdoYfmizoMI58l1zV0v7Qy EJ6WrBrTxbxtvKRRTCEEQd47BMhaNqXivSTyzdjrf4MS1PuJFT7LuKlRz7sbwt1Zkmav ipzzsYff6xsRM3FNkL2u35dRCdeQ1CTgQH2e9U0DS0sofUE/mMpdaQhtNa5BXbgHoX/R 88qg== X-Gm-Message-State: AOJu0Yz5rntY5t8MbU/ij8i/qKndBcB/elwS2nfjFgM4vbZM5YBqun+S IP5gq6GxRJCM0/ahbSDv9ksekAQ6x2iyYrcBlhSUESNCiV8PgLpBXAEun4VyWCWCOvQ8YOCNT0G NKCRAStDDCC8T6tv8rPzRFYebRJod4+gX4WnTLYIvutDc9N3e X-Google-Smtp-Source: AGHT+IG6cU8hQJPmmZbzVVk/t3JG5lrJrHbVvrPLAoFTSglYadsYeGpe/2QeeunbqQgORmXlpqOFERJh2YQd8WoCjHk= X-Received: by 2002:a2e:968c:0:b0:2e0:4cbb:858a with SMTP id 38308e7fff4ca-2e95b0c093bmr73491981fa.27.1716930459575; Tue, 28 May 2024 14:07:39 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240509112057.1167947-1-luca.vizzarro@arm.com> <20240509112057.1167947-8-luca.vizzarro@arm.com> In-Reply-To: <20240509112057.1167947-8-luca.vizzarro@arm.com> From: Jeremy Spewock Date: Tue, 28 May 2024 17:07:27 -0400 Message-ID: Subject: Re: [PATCH v2 7/8] dts: rework interactive shells To: Luca Vizzarro Cc: dev@dpdk.org, =?UTF-8?Q?Juraj_Linke=C5=A1?= , Paul Szczepanek Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, May 9, 2024 at 7:21=E2=80=AFAM Luca Vizzarro wrote: > > The way nodes and interactive shells interact makes it difficult to > develop for static type checking and hinting. The current system relies > on a top-down approach, attempting to give a generic interface to the > test developer, hiding the interaction of concrete shell classes as much > as possible. When working with strong typing this approach is not ideal, > as Python's implementation of generics is still rudimentary. > > This rework reverses the tests interaction to a bottom-up approach, > allowing the test developer to call concrete shell classes directly, > and let them ingest nodes independently. While also re-enforcing type > checking and making the code easier to read. > > Signed-off-by: Luca Vizzarro > Reviewed-by: Paul Szczepanek > --- > dts/framework/params/eal.py | 6 +- > dts/framework/remote_session/dpdk_shell.py | 104 ++++++++++++++++ > .../remote_session/interactive_shell.py | 75 +++++++----- > dts/framework/remote_session/python_shell.py | 4 +- > dts/framework/remote_session/testpmd_shell.py | 64 +++++----- > dts/framework/testbed_model/node.py | 36 +----- > dts/framework/testbed_model/os_session.py | 36 +----- > dts/framework/testbed_model/sut_node.py | 112 +----------------- > .../testbed_model/traffic_generator/scapy.py | 4 +- > dts/tests/TestSuite_hello_world.py | 7 +- > dts/tests/TestSuite_pmd_buffer_scatter.py | 21 ++-- > dts/tests/TestSuite_smoke_tests.py | 2 +- > 12 files changed, 201 insertions(+), 270 deletions(-) > create mode 100644 dts/framework/remote_session/dpdk_shell.py > > def __init__( > self, > - interactive_session: SSHClient, > - logger: DTSLogger, > - get_privileged_command: Callable[[str], str] | None, > + node: Node, > app_params: Params =3D Params(), > + privileged: bool =3D False, > timeout: float =3D SETTINGS.timeout, > + start_on_init: bool =3D True, > ) -> None: > """Create an SSH channel during initialization. > > Args: > - interactive_session: The SSH session dedicated to interactiv= e 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 wi= ll not be started > - with elevated privileges. > + node: The node on which to run start the interactive shell. > app_params: The command line parameters to be passed to the = application on startup. > + privileged: Enables the shell to run as superuser. > timeout: The timeout used for the SSH channel that is dedica= ted to this interactive > shell. This timeout is for collecting output, so if read= ing from the buffer > and no output is gathered within the timeout, an excepti= on is thrown. > + start_on_init: Start interactive shell automatically after o= bject initialisation. > """ > - self._interactive_session =3D interactive_session > - self._ssh_channel =3D self._interactive_session.invoke_shell() > + self._node =3D node > + self._logger =3D node._logger > + self._app_params =3D app_params > + self._privileged =3D privileged > + self._timeout =3D timeout > + # Ensure path is properly formatted for the host > + self._update_path(self._node.main_session.join_remote_path(self.= path)) > + > + self.__post_init__() > + > + if start_on_init: > + self.start_application() What's the reason for including start_on_init? Is there a time when someone would create an application but not want to start it when they create it? It seems like it is always true currently and I'm not sure we would want it to be delayed otherwise (except in cases like the context manager patch where we want to enforce that it is only started for specific periods of time). > + > + def __post_init__(self): Is the name of this method meant to mimic that of the dataclasses? It might also make sense to call it something like `_post_init()` as just a regular private method, I'm not sure it matters either way. Additionally, I think in other super classes which contained functions that were optionally implemented by subclasses we omitted the `pass` and just left the function stub empty other than the doc-string. Either way this does the same thing, but it might be better to make them consistent one way or the other. > + """Overridable. Method called after the object init and before a= pplication start.""" > + pass > + > > - def _start_application(self, get_privileged_command: Callable[[str],= str] | None) -> None: > + def start_application(self) -> None: > """Starts a new interactive application based on the path to the= app. > > This method is often overridden by subclasses as their process f= or > starting may look different. > - > - Args: > - get_privileged_command: A function (but could be any callabl= e) that produces > - the version of the command with elevated privileges. > """ > - start_command =3D f"{self.path} {self._app_params}" > - if get_privileged_command is not None: > - start_command =3D get_privileged_command(start_command) > + self._setup_ssh_channel() > + > + start_command =3D self._make_start_command() > + if self._privileged: > + start_command =3D self._node.main_session._get_privileged_co= mmand(start_command) > self.send_command(start_command) > > + def _make_start_command(self) -> str: It might make sense to put this above the start_application method since that is where it gets called. > + """Makes the command that starts the interactive shell.""" > + return f"{self.path} {self._app_params or ''}" > + > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewor= k/remote_session/testpmd_shell.py > index ef3f23c582..92930d7fbb 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -7,9 +7,7 @@ > > Typical usage example in a TestSuite:: > > - testpmd_shell =3D self.sut_node.create_interactive_shell( > - TestPmdShell, privileged=3DTrue > - ) > + testpmd_shell =3D TestPmdShell() Maybe adding a parameter to this instantiation in the example would still be useful. So something like `TestPmdShell(self.sut_node)` instead just because this cannot be instantiated without any arguments. > devices =3D testpmd_shell.get_devices() > for device in devices: > print(device) > 2.34.1 >