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 434D843393; Tue, 21 Nov 2023 22:55:44 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0C6DC402C3; Tue, 21 Nov 2023 22:55:44 +0100 (CET) Received: from mail-pl1-f173.google.com (mail-pl1-f173.google.com [209.85.214.173]) by mails.dpdk.org (Postfix) with ESMTP id 01D0E402C0 for ; Tue, 21 Nov 2023 22:55:41 +0100 (CET) Received: by mail-pl1-f173.google.com with SMTP id d9443c01a7336-1cf5901b4c8so27085705ad.1 for ; Tue, 21 Nov 2023 13:55:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1700603741; x=1701208541; 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=ISKiEB7fqH2lT2AQtC0ggy5KX3CrLoAp3Taqha+k1sE=; b=WgJJi+iFzvwnuKSXLEt4fbVIq5Dbqv0YbMeYIlSTzFqpevfq9de2ra62y88VxbYls2 35gtGnYYQWdISuLEem32OdZh9eYxUToRPdSvc4YmMSPzqEXK/TF9AeQK62VMBOq1bZoV mUo17iAOunHwNcaMYNuPyT3QaE+5Mr48NIlZs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700603741; x=1701208541; 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=ISKiEB7fqH2lT2AQtC0ggy5KX3CrLoAp3Taqha+k1sE=; b=ZPlFaPvgRDO2ZBrhCyYdAaZXd2rgp+Raam+JAeePoO/hRwCaO+nEz9SBEWPC1UwDUN 28EgITHRFX/u6k1k6rj5qjlGZV+xmdMRTr4BjL8PKOYbrDLt1QICTKzkhYhn67rtYtvB nsDGywmGrt1/zGuElBvu9+R/rOPRd6+5mkBDwKFmX1PO8mRpk/jqRI0ixV9xHG8RoA7k 3fodMBcI0EIgUGzwDUuZ1bWC8XsJ3v9in8GABDAcR4dVwy7kUlH9toMNTnHUMAFliHGY ygsHAfprhcqk4Gj9QmSQpRpnyhFLlgHh0n1/4lTxT0zRRAQCYE/FdJGG/N35T4cngVhC zy5Q== X-Gm-Message-State: AOJu0YyQcqNMzf0W+lTGBHzK1fFbMRQFaIPW+0kp+cxmbCmuoiNMWsPv l31aalbkklPZALwIlxtDw3Lzj9aJEEdyknvVNEXeHQ== X-Google-Smtp-Source: AGHT+IExFYXtA4KYTDOkYqAbywvkvKTm7UNc7qGGeDR3D85atTH3/AHJwTQFZ80fewAaezBVhvRvibHNEAamOnJI35w= X-Received: by 2002:a17:90b:3509:b0:280:5b33:d30 with SMTP id ls9-20020a17090b350900b002805b330d30mr586013pjb.44.1700603741001; Tue, 21 Nov 2023 13:55:41 -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: Tue, 21 Nov 2023 16:55:29 -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="000000000000e30721060ab0a939" 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 --000000000000e30721060ab0a939 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Nov 20, 2023 at 9:35=E2=80=AFAM Juraj Linke=C5=A1 wrote: > On Fri, Nov 17, 2023 at 7:08=E2=80=AFPM Jeremy Spewock > wrote: > > > > > > > > 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 > the app. If this > >> > isn't provided or an empty string is passed, it wil= l > 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 > > I don't know whether all DPDK apps need EAL parameters (I thought they > needed them). My point is about the condition: the two hyphens are > only added for DPDK apps when *default* EAL parameters are used. When > non-default EAL parameters are passed (i.e. when eal_parameters =3D=3D > True), the hyphens are not added for DPDK apps. > > > and change it instead so that we don't pass EalParameters into DPDK > applications and I think that should cover these cases. > > > > I think you meant non-DPDK applications here, right? > I did mean non-DPDK, yes. > > > > >> > >> > # 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 > >> > > --000000000000e30721060ab0a939 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Mon, Nov 20, 2023 at 9:35=E2=80=AFA= M Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
On Fri, Nov 17, 2023 at 7:08= =E2=80=AFPM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
>
>
> On Thu, Nov 16, 2023 at 1:53=E2=80=AFPM Juraj Linke=C5=A1 <juraj.li= nkes@pantheon.tech> wrote:
>>
>> On Mon, Nov 13, 2023 at 9:28=E2=80=AFPM <jspewock@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 EA= L
>> > parameters are handled so that the trailing "--" se= parator is 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_shel= l.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(
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self, get_privileged_comman= d: Callable[[str], str] | None
>> >=C2=A0 =C2=A0 =C2=A0 ) -> None:
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """See "= ;_start_application" 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&qu= ot;
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super()._start_application(= get_privileged_command)
>> >
>> >=C2=A0 =C2=A0 =C2=A0 def get_devices(self) -> list[TestPmdD= evice]:
>> > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/fr= amework/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(
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls: Type[Interactive= ShellType],
>> >=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 | None =3D None,
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 eal_parameters: EalParameters | = str =3D "",
>>
>> I think it makes more sense if the type is EalParameters | None. W= ith
>> this change, the str type of eal_parameters moved to app_parameter= s.
>>
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 app_parameters: str =3D "&q= uot;,
>> >=C2=A0 =C2=A0 =C2=A0 ) -> InteractiveShellType:
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """Factory m= ethod 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_parameter= s: List of 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 callin= g
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= create_eal_parameters().
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 app_parameters: Ad= ditional arguments to pass into the application on the
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 comm= and-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 t= he desired 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_parameter= s =3D self.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 gla= nce.
>>
>> Before we either passed EalParameters (meant for DPDK apps) or a >> string (meant for other apps). There was no additional check for t= hese
>> 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 hyphen= s
>> 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 cal= ler
>> 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 ca= n make that just specific to testpmd

I don't know whether all DPDK apps need EAL parameters (I thought they<= br> needed them). My point is about the condition: the two hyphens are
only added for DPDK apps when *default* EAL parameters are used. When
non-default EAL parameters are passed (i.e. when eal_parameters =3D=3D
True), the hyphens are not added for DPDK apps.

> and change it instead so that we don't pass EalParameters into DPD= K applications and I think that should cover these cases.
>

I think you meant non-DPDK applications here, right?
<= br>
I did mean non-DPDK, yes.
=C2=A0

>
>>
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # We need to append the bui= ld directory 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.pat= h =3D self.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_inter= active_shell(
>> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls, timeout= , privileged, str(eal_parameters)
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls, timeout= , privileged, 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
>> >
--000000000000e30721060ab0a939--