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 9563143357; Fri, 17 Nov 2023 19:08:14 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5550E402EA; Fri, 17 Nov 2023 19:08:14 +0100 (CET) Received: from mail-pg1-f170.google.com (mail-pg1-f170.google.com [209.85.215.170]) by mails.dpdk.org (Postfix) with ESMTP id EA87840285 for ; Fri, 17 Nov 2023 19:08:12 +0100 (CET) Received: by mail-pg1-f170.google.com with SMTP id 41be03b00d2f7-5bd6ac9833fso1623074a12.0 for ; Fri, 17 Nov 2023 10:08:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1700244492; x=1700849292; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=qFu2yErPfVEitA5yxjDY5EFjSizOdD5jhdRwhsCtcJY=; b=YvEHq8SRXYeQoXj6SaGeMRXGzJskFGK2JUtA3PpzXPoF+lmPwxZSAE4OqyiIRgTV2Y CdXyo4LK6pjInOqa1k2geWQ6VbPtTSQy4VmQORPlagxggHKd5BTGfa/Q556WvfpbweTx 2lUzDF4Tb/IuN/QiQ6XlG1CmFCvbcRjvgZsMM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700244492; x=1700849292; h=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=qFu2yErPfVEitA5yxjDY5EFjSizOdD5jhdRwhsCtcJY=; b=Jidcd7CZnY8fMuGOxBe+hsCynmtBWEDAhQuGUsyB7BsAg3EYl+b7CqZ22SP3nRPB1X VpwMsijl+voiN96siZBMpeMHr4Rw12Ovsgqe9I1XT/fROTHf61NsYm9Mn8y8GCJ9mcDX 05PFdFlMZj4Up9UAsnLX/ZxzSE3LGtE3dNSQFGYK5hdv5DxlW8nYaxM/4VumcawPJ8Bp a66N91Kbt86SXfwwru5t7Qd8xgUu4+vLd3BpBS7V7mUfCZkEmJwKLiwRWthW9edqDJT5 fOxWX0d5tKOWGsMv4YExqmCHnoOA13WQviZYcyfSUJbv3aUrTeYcS8/duOIYsIWTQtJq CoSA== X-Gm-Message-State: AOJu0YzE8jE5G5nb6KRgbkoWOW8y2RJqTTGggvESCS9rrN7NKR8vdtLz K9DBi/vsG19UN/zTLyHlHRJGs22DL6rhgCh6SUfl5A== X-Google-Smtp-Source: AGHT+IENNwqk/rjSufpQ2mG/MjQX0YoHYugFf8l5aj5w/8+EKmOfNXfrwj0kn3G7wJUCWctEaPgtEtRD6oLVYZKcW6M= X-Received: by 2002:a17:90a:3f87:b0:27f:bd9e:5a15 with SMTP id m7-20020a17090a3f8700b0027fbd9e5a15mr303030pjc.28.1700244492043; Fri, 17 Nov 2023 10:08:12 -0800 (PST) MIME-Version: 1.0 References: <20231113202833.12900-1-jspewock@iol.unh.edu> <20231113202833.12900-5-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Fri, 17 Nov 2023 13:08:01 -0500 Message-ID: Subject: Re: [PATCH v3 4/7] dts: allow passing parameters into interactive apps To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, ferruh.yigit@amd.com, andrew.rybchenko@oktetlabs.ru, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000fadc4a060a5d043d" 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 --000000000000fadc4a060a5d043d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Nov 16, 2023 at 1:53=E2=80=AFPM Juraj Linke=C5=A1 wrote: > On Mon, Nov 13, 2023 at 9:28=E2=80=AFPM wrote: > > > > From: Jeremy Spewock > > > > Modified interactive applications to allow for the ability to pass > > parameters into the app on start up. Also modified the way EAL > > parameters are handled so that the trailing "--" separator is added be > > default after all EAL parameters. > > > > Signed-off-by: Jeremy Spewock > > --- > > dts/framework/remote_session/remote/testpmd_shell.py | 2 +- > > dts/framework/testbed_model/sut_node.py | 11 +++++++---- > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/dts/framework/remote_session/remote/testpmd_shell.py > b/dts/framework/remote_session/remote/testpmd_shell.py > > index 3ea16c7ab3..3f6a86aa35 100644 > > --- a/dts/framework/remote_session/remote/testpmd_shell.py > > +++ b/dts/framework/remote_session/remote/testpmd_shell.py > > @@ -32,7 +32,7 @@ def _start_application( > > self, get_privileged_command: Callable[[str], str] | None > > ) -> None: > > """See "_start_application" in InteractiveShell.""" > > - self._app_args +=3D " -- -i" > > + self._app_args +=3D " -i" > > super()._start_application(get_privileged_command) > > > > def get_devices(self) -> list[TestPmdDevice]: > > diff --git a/dts/framework/testbed_model/sut_node.py > b/dts/framework/testbed_model/sut_node.py > > index 4161d3a4d5..bcac939e72 100644 > > --- a/dts/framework/testbed_model/sut_node.py > > +++ b/dts/framework/testbed_model/sut_node.py > > @@ -377,7 +377,8 @@ def create_interactive_shell( > > shell_cls: Type[InteractiveShellType], > > timeout: float =3D SETTINGS.timeout, > > privileged: bool =3D False, > > - eal_parameters: EalParameters | str | None =3D None, > > + eal_parameters: EalParameters | str =3D "", > > I think it makes more sense if the type is EalParameters | None. With > this change, the str type of eal_parameters moved to app_parameters. > > > + app_parameters: str =3D "", > > ) -> InteractiveShellType: > > """Factory method for creating a handler for an interactive > session. > > > > @@ -392,12 +393,14 @@ def create_interactive_shell( > > eal_parameters: List of EAL parameters to use to launch th= e > app. If this > > isn't provided or an empty string is passed, it will > default to calling > > create_eal_parameters(). > > + app_parameters: Additional arguments to pass into the > application on the > > + command-line. > > Returns: > > Instance of the desired interactive application. > > """ > > - if not eal_parameters: > > + if not eal_parameters and shell_cls.dpdk_app: > > eal_parameters =3D self.create_eal_parameters() > > - > > + eal_parameters =3D f"{eal_parameters} --" > > I think this change is more complicated than it seems at first glance. > > Before we either passed EalParameters (meant for DPDK apps) or a > string (meant for other apps). There was no additional check for these > assumptions. > Now that we've split it, I see some cases which seem to be not covered. > > 1. The app is a DPDK app and we pass EalParameters. The two hyphens > are not added. > 2. The app is not a DPDK app and we pass EalParameters. Similarly to > current behavior (without the patch), we kinda assume that the caller > won't do this, but now that we're modifying the behavior, let's add a > check that won't allow EalParameters with non-DPDK apps. > > That is a good point that not all DPDk apps need the two hyphens, I can make that just specific to testpmd and change it instead so that we don't pass EalParameters into DPDK applications and I think that should cover these cases. > > # We need to append the build directory for DPDK apps > > if shell_cls.dpdk_app: > > shell_cls.path =3D self.main_session.join_remote_path( > > @@ -405,7 +408,7 @@ def create_interactive_shell( > > ) > > > > return super().create_interactive_shell( > > - shell_cls, timeout, privileged, str(eal_parameters) > > + shell_cls, timeout, privileged, f"{eal_parameters} > {app_parameters}" > > ) > > > > def bind_ports_to_driver(self, for_dpdk: bool =3D True) -> None: > > -- > > 2.42.0 > > > --000000000000fadc4a060a5d043d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Thu, Nov 16, 2023 at 1:53=E2=80=AFP= M Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
On Mon, Nov 13, 2023 at 9:28= =E2=80=AFPM <j= spewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Modified interactive applications to allow for the ability to pass
> parameters into the app on start up. Also modified the way EAL
> parameters are handled so that the trailing "--" separator i= s added be
> default after all EAL parameters.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>=C2=A0 dts/framework/remote_session/remote/testpmd_shell.py |=C2=A0 2 += -
>=C2=A0 dts/framework/testbed_model/sut_node.py=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 | 11 +++++++----
>=C2=A0 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/dts/framework/remote_session/remote/testpmd_shell.py b/dt= s/framework/remote_session/remote/testpmd_shell.py
> index 3ea16c7ab3..3f6a86aa35 100644
> --- a/dts/framework/remote_session/remote/testpmd_shell.py
> +++ b/dts/framework/remote_session/remote/testpmd_shell.py
> @@ -32,7 +32,7 @@ def _start_application(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self, get_privileged_command: Callab= le[[str], str] | None
>=C2=A0 =C2=A0 =C2=A0 ) -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """See "_start_a= pplication" in InteractiveShell."""
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._app_args +=3D " -- -i" > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._app_args +=3D " -i"
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super()._start_application(get_privi= leged_command)
>
>=C2=A0 =C2=A0 =C2=A0 def get_devices(self) -> list[TestPmdDevice]: > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/t= estbed_model/sut_node.py
> index 4161d3a4d5..bcac939e72 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -377,7 +377,8 @@ def create_interactive_shell(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls: Type[InteractiveShellType= ],
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timeout: float =3D SETTINGS.timeout,=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 privileged: bool =3D False,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 eal_parameters: EalParameters | str | Non= e =3D None,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 eal_parameters: EalParameters | str =3D &= quot;",

I think it makes more sense if the type is EalParameters | None. With
this change, the str type of eal_parameters moved to app_parameters.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 app_parameters: str =3D "",
>=C2=A0 =C2=A0 =C2=A0 ) -> InteractiveShellType:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """Factory method for= creating a handler for an interactive session.
>
> @@ -392,12 +393,14 @@ def create_interactive_shell(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 eal_parameters: List o= f EAL parameters to use to launch the app. If this
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 isn'= t provided or an empty string is passed, it will default to calling
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 create_e= al_parameters().
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 app_parameters: Additional = arguments to pass into the application on the
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 command-line.=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Instance of the desire= d interactive application.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not eal_parameters:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not eal_parameters and shell_cls.dpdk_= app:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 eal_parameters =3D sel= f.create_eal_parameters()
> -
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 eal_parameters =3D f"{= eal_parameters} --"

I think this change is more complicated than it seems at first glance.

Before we either passed EalParameters (meant for DPDK apps) or a
string (meant for other apps). There was no additional check for these
assumptions.
Now that we've split it, I see some cases which seem to be not covered.=

1. The app is a DPDK app and we pass EalParameters. The two hyphens
are not added.
2. The app is not a DPDK app and we pass EalParameters. Similarly to
current behavior (without the patch), we kinda assume that the caller
won't do this, but now that we're modifying the behavior, let's= add a
check that won't allow EalParameters with non-DPDK apps.


That is a good point that not all DPDk apps n= eed the two hyphens, I can make that just specific to testpmd and change it= instead so that we don't pass EalParameters into DPDK applications and= I think that should cover these cases.

=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # We need to append the build direct= ory for DPDK apps
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if shell_cls.dpdk_app:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls.path =3D sel= f.main_session.join_remote_path(
> @@ -405,7 +408,7 @@ def create_interactive_shell(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return super().create_interactive_sh= ell(
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls, timeout, privile= ged, str(eal_parameters)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls, timeout, privile= ged, f"{eal_parameters} {app_parameters}"
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
>
>=C2=A0 =C2=A0 =C2=A0 def bind_ports_to_driver(self, for_dpdk: bool =3D = True) -> None:
> --
> 2.42.0
>
--000000000000fadc4a060a5d043d--