[v3,4/7] dts: allow passing parameters into interactive apps
Checks
Commit Message
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 is added be
default after all EAL parameters.
Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
dts/framework/remote_session/remote/testpmd_shell.py | 2 +-
dts/framework/testbed_model/sut_node.py | 11 +++++++----
2 files changed, 8 insertions(+), 5 deletions(-)
Comments
On Mon, Nov 13, 2023 at 9:28 PM <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 EAL
> parameters are handled so that the trailing "--" separator is added be
> default after all EAL parameters.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
> 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 += " -- -i"
> + self._app_args += " -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 = SETTINGS.timeout,
> privileged: bool = False,
> - eal_parameters: EalParameters | str | None = None,
> + eal_parameters: EalParameters | str = "",
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 = "",
> ) -> 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 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 = self.create_eal_parameters()
> -
> + eal_parameters = 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.
> # We need to append the build directory for DPDK apps
> if shell_cls.dpdk_app:
> shell_cls.path = 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 = True) -> None:
> --
> 2.42.0
>
On Thu, Nov 16, 2023 at 1:53 PM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:
> On Mon, Nov 13, 2023 at 9:28 PM <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 EAL
> > parameters are handled so that the trailing "--" separator is added be
> > default after all EAL parameters.
> >
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > ---
> > 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 += " -- -i"
> > + self._app_args += " -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 = SETTINGS.timeout,
> > privileged: bool = False,
> > - eal_parameters: EalParameters | str | None = None,
> > + eal_parameters: EalParameters | str = "",
>
> 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 = "",
> > ) -> 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 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 = self.create_eal_parameters()
> > -
> > + eal_parameters = 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 = 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 = True) -> None:
> > --
> > 2.42.0
> >
>
On Fri, Nov 17, 2023 at 7:08 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
>
>
> On Thu, Nov 16, 2023 at 1:53 PM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>>
>> On Mon, Nov 13, 2023 at 9:28 PM <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 EAL
>> > parameters are handled so that the trailing "--" separator is added be
>> > default after all EAL parameters.
>> >
>> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
>> > ---
>> > 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 += " -- -i"
>> > + self._app_args += " -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 = SETTINGS.timeout,
>> > privileged: bool = False,
>> > - eal_parameters: EalParameters | str | None = None,
>> > + eal_parameters: EalParameters | str = "",
>>
>> 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 = "",
>> > ) -> 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 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 = self.create_eal_parameters()
>> > -
>> > + eal_parameters = 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 ==
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?
>
>>
>> > # We need to append the build directory for DPDK apps
>> > if shell_cls.dpdk_app:
>> > shell_cls.path = 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 = True) -> None:
>> > --
>> > 2.42.0
>> >
On Mon, Nov 20, 2023 at 9:35 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:
> On Fri, Nov 17, 2023 at 7:08 PM Jeremy Spewock <jspewock@iol.unh.edu>
> wrote:
> >
> >
> >
> > On Thu, Nov 16, 2023 at 1:53 PM Juraj Linkeš <juraj.linkes@pantheon.tech>
> wrote:
> >>
> >> On Mon, Nov 13, 2023 at 9:28 PM <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 EAL
> >> > parameters are handled so that the trailing "--" separator is added be
> >> > default after all EAL parameters.
> >> >
> >> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> >> > ---
> >> > 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 += " -- -i"
> >> > + self._app_args += " -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 = SETTINGS.timeout,
> >> > privileged: bool = False,
> >> > - eal_parameters: EalParameters | str | None = None,
> >> > + eal_parameters: EalParameters | str = "",
> >>
> >> 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 = "",
> >> > ) -> 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 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 = self.create_eal_parameters()
> >> > -
> >> > + eal_parameters = 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 ==
> 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 = 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 = True) -> None:
> >> > --
> >> > 2.42.0
> >> >
>
@@ -32,7 +32,7 @@ def _start_application(
self, get_privileged_command: Callable[[str], str] | None
) -> None:
"""See "_start_application" in InteractiveShell."""
- self._app_args += " -- -i"
+ self._app_args += " -i"
super()._start_application(get_privileged_command)
def get_devices(self) -> list[TestPmdDevice]:
@@ -377,7 +377,8 @@ def create_interactive_shell(
shell_cls: Type[InteractiveShellType],
timeout: float = SETTINGS.timeout,
privileged: bool = False,
- eal_parameters: EalParameters | str | None = None,
+ eal_parameters: EalParameters | str = "",
+ app_parameters: str = "",
) -> 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 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 = self.create_eal_parameters()
-
+ eal_parameters = f"{eal_parameters} --"
# We need to append the build directory for DPDK apps
if shell_cls.dpdk_app:
shell_cls.path = 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 = True) -> None: