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 8545C44104; Wed, 29 May 2024 22:37:50 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1A261402B0; Wed, 29 May 2024 22:37:50 +0200 (CEST) Received: from mail-pj1-f47.google.com (mail-pj1-f47.google.com [209.85.216.47]) by mails.dpdk.org (Postfix) with ESMTP id E1EB640273 for ; Wed, 29 May 2024 22:37:48 +0200 (CEST) Received: by mail-pj1-f47.google.com with SMTP id 98e67ed59e1d1-2c1a5913777so124467a91.2 for ; Wed, 29 May 2024 13:37:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1717015068; x=1717619868; 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=sDFKe6xTeW684d0Zf9GZOwlgIQUl8UAUksU1CNTWOYE=; b=OVceeKHdO8mq6x1vuiHJG/mxCdt6Y58gl3/BCDXbJzl9F8zsqcbzpF8mPzSoTPkpNa 743lHbM/Z/vn7Ry9+uKa3J1LG8/IpakYyc7raX35CV+fR9xnySKgUHHBsyNmKb8ruShz LaiNyO43GsPCj/0/my321biUFLIZarr0nJA5o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717015068; x=1717619868; 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=sDFKe6xTeW684d0Zf9GZOwlgIQUl8UAUksU1CNTWOYE=; b=WEHIsW5ar+KyofYSZW4wWZmCiK7jOEv1cz0EkX22k9yl0srR1aPGuZgLCS7OZQnraU er4cpyphQjgN6zJC05Lird9Arzd0Xy95KjIbFsc1WCdNM6xI7j8ZpP5LcNr3T8uAnpHV H9TY/Ts6koIiZmO1HsrSY0EYmCfgQiZNe8zNJ9h04YLHeMwRUXsalq7OGjNxYlw8ML97 vleU6v1G52/2OL9665TnBTIf4P1dHvcHn3ajTZEvpK/l054tAXrkkKCUrm/zpzHFtK10 Fef9V89vQZwm4B1MiULnF/S4mUCsKvcKsskt2aYECWowrXtOesuvMHCEmQsCkTq2Q/sa qUFA== X-Forwarded-Encrypted: i=1; AJvYcCVo3g66A1bP0/aMb5QHQVu8naM2wZG5IdHY/OWkiqNL9FF2pMCnbuMO9p3AgzgoyxFWpVew3wse4wOsT48= X-Gm-Message-State: AOJu0YxCzRUkFdm1nUjlGJte66QBiKc3wWz6wVsHLF3gdqZb/aNY17dz MTQ6O3TEjqNc2tKEf5/QGmgeEXqZppE9cpwEsbJwMyAw4yx+Hxeb6WVDFq8Bsaiw1ZQRKmepMXC Tw5p+JtKaWjcFLNhZcXvzfY3+kLGoBO2KokEjCg== X-Google-Smtp-Source: AGHT+IGLuI7pXcG2aopF0ITrptJ1sTWEh5dSUDJdKW5FWGxA/8OAHmVU22MlJ19uHkddSJyCeRiEwquoT7RLp2bBnjY= X-Received: by 2002:a17:90a:af90:b0:2ad:af1c:4fc with SMTP id 98e67ed59e1d1-2c1abb029b2mr223757a91.25.1717015067898; Wed, 29 May 2024 13:37:47 -0700 (PDT) MIME-Version: 1.0 References: <20240514201436.2496-1-jspewock@iol.unh.edu> <20240514201436.2496-3-jspewock@iol.unh.edu> <3220a75e-5401-44c7-a64d-ee950e4aaf1f@arm.com> In-Reply-To: <3220a75e-5401-44c7-a64d-ee950e4aaf1f@arm.com> From: Jeremy Spewock Date: Wed, 29 May 2024 16:37:36 -0400 Message-ID: Subject: Re: [PATCH v1 2/4] dts: add context manager for interactive shells To: Luca Vizzarro Cc: yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech, probb@iol.unh.edu, wathsala.vithanage@arm.com, thomas@monjalon.net, dev@dpdk.org 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 Mon, May 20, 2024 at 1:31=E2=80=AFPM Luca Vizzarro wrote: > > On 14/05/2024 21:14, jspewock@iol.unh.edu wrote: > > +class CriticalInteractiveShell(InteractiveShell): > > > + _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 =3D "", > > + timeout: float =3D 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 interact= ive shells. > > + logger: The logger instance this session will use. > > + get_privileged_command: A method for modifying a command t= o 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 a= pplication on startup. > > + timeout: The timeout used for the SSH channel that is dedi= cated to this interactive > > + shell. This timeout is for collecting output, so if re= ading from the buffer > > + and no output is gathered within the timeout, an excep= tion is thrown. The default > > + value for this argument may be modified using the :opt= ion:`--timeout` command-line > > + argument or the :envvar:`DTS_TIMEOUT` environment vari= able. > > + """ > > + self._interactive_session =3D interactive_session > > + self._logger =3D logger > > + self._timeout =3D timeout > > + self._app_args =3D app_args > > + self._get_priviledged_command =3D get_privileged_command > > + > > + def __enter__(self: CriticalInteractiveShellType) -> CriticalInter= activeShellType: > > 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, trac= eback: 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 ensur= e 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 r= aised within the block, it is > > + not handled and will be re-raised after the application is clo= sed. > > + > > + Args: > > + type: Type of exception that was thrown in the context blo= ck 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 contex= t block if there was one. > > + """ > > + self.close() > > > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/fr= amework/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 exce= ption is thrown. > > """ > > self._interactive_session =3D interactive_session > > - self._ssh_channel =3D self._interactive_session.invoke_shell() > > - self._stdin =3D self._ssh_channel.makefile_stdin("w") > > - self._stdout =3D self._ssh_channel.makefile("r") > > - self._ssh_channel.settimeout(timeout) > > - self._ssh_channel.set_combine_stderr(True) # combines stdout = and stderr streams > > self._logger =3D logger > > self._timeout =3D timeout > > self._app_args =3D app_args > > + self._init_channel() > > self._start_application(get_privileged_command) > > > > + def _init_channel(self): > > + self._ssh_channel =3D self._interactive_session.invoke_shell() > > + self._stdin =3D self._ssh_channel.makefile_stdin("w") > > + self._stdout =3D 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[[st= r], 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/Test= Suite_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=3DTrue, > > ) > > - testpmd.set_forward_mode(TestPmdForwardingModes.mac) > > - testpmd.start() > > - > > - for offset in [-1, 0, 1, 4, 5]: > > - recv_payload =3D self.scatter_pktgen_send_packet(mbsize + = offset) > > - self._logger.debug(f"Payload of scattered packet after for= warding: \n{recv_payload}") > > - self.verify( > > - ("58 " * 8).strip() in recv_payload, > > - f"Payload of scattered packet did not match expected p= ayload 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 =3D self.scatter_pktgen_send_packet(mbsiz= e + 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 expecte= d 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.