DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jeremy Spewock <jspewock@iol.unh.edu>
To: Dean Marx <dmarx@iol.unh.edu>
Cc: probb@iol.unh.edu, npratte@iol.unh.edu, luca.vizzarro@arm.com,
	 yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com,
	 paul.szczepanek@arm.com, juraj.linkes@pantheon.tech,
	dev@dpdk.org
Subject: Re: [RFC v1 1/3] dts: add UDP tunnel command to testpmd shell
Date: Fri, 26 Jul 2024 16:18:15 -0400	[thread overview]
Message-ID: <CAAA20UQLVHhfajvu+h_Smpm6S9xrnu4mw7QO_Etr7aiMhJtk0w@mail.gmail.com> (raw)
In-Reply-To: <20240725162325.20933-2-dmarx@iol.unh.edu>

Hey Dean, these changes look good to me, I just had a few minor
comments/suggestions.

One thing I did notice was that the methods added here don't have
type-hints for their return-types, obviously functionally it makes no
difference since they don't return anything, but just adding the note
that says the return None is helpful for type checkers and
understanding the method at a glance.

On Thu, Jul 25, 2024 at 12:23 PM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> add udp_tunnel_port command to testpmd shell class,
> also ports over set verbose method from vlan suite
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 51 ++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index eda6eb320f..26114091d6 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -804,7 +804,56 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
> -    def _close(self) -> None:

It looks like this method might have been renamed by mistake in a
rebase, the name on main right now is _close. This could cause some
weird behavior in your testing since this is what the context manager
uses to close the session, but I don't think it would have any drastic
effect since the channel is still closed.

> +    def set_verbose(self, level: int, verify: bool = True):
> +        """Set debug verbosity level.
> +
> +        Args:
> +            level: 0 - silent except for error
> +                1 - fully verbose except for Tx packets
> +                2 - fully verbose except for Rx packets
> +                >2 - fully verbose
> +            verify: If :data:`True` the command output will be scanned to verify that verbose level
> +                is properly set. Defaults to :data:`True`.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and verbose level
> +            is not correctly set.
> +        """
> +        verbose_output = self.send_command(f"set verbose {level}")
> +        if verify:
> +            if "Change verbose level" not in verbose_output:
> +                self._logger.debug(f"Failed to set verbose level to {level}: \n{verbose_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set verbose level to {level}."
> +                )
> +
> +    def udp_tunnel_port(
> +        self, port_id: int, add: bool, udp_port: int, protocol: str, verify: bool = True
> +    ):
> +        """Configures a UDP tunnel on the specified port, for the specified protocol.
> +
> +        Args:
> +            port_id: ID of the port to configure tunnel on.
> +            add: If :data:`True`, adds tunnel, otherwise removes tunnel.
> +            udp_port: ID of the UDP port to configure tunnel on.
> +            protocol: Name of tunnelling protocol to use; options are vxlan, geneve, ecpri

If there are explicit choices that this has to be like this it might
be better to put these options into an enum and then pass that in as
the parameter here. That way it is very clear from just calling the
methods what your options are.



> +            verify: If :data:`True`, checks the output of the command to verify that
> +                no errors were thrown.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If verify is :data:`True` and command
> +                output shows an error.
> +        """
> +        action = "add" if add else "rm"
> +        cmd_output = self.send_command(
> +            f"port config {port_id} udp_tunnel_port {action} {protocol} {udp_port}"
> +        )
> +        if verify:
> +            if "Operation not supported" in cmd_output or "Bad arguments" in cmd_output:
> +                self._logger.debug(f"Failed to set UDP tunnel: \n{cmd_output}")
> +                raise InteractiveCommandExecutionError(f"Failed to set UDP tunnel: \n{cmd_output}")
> +
> +    def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.stop()
>          self.send_command("quit", "Bye...")
> --
> 2.44.0
>

  reply	other threads:[~2024-07-26 20:18 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-25 16:23 [RFC v1 0/3] VXLAN-GPE test suite Dean Marx
2024-07-25 16:23 ` [RFC v1 1/3] dts: add UDP tunnel command to testpmd shell Dean Marx
2024-07-26 20:18   ` Jeremy Spewock [this message]
2024-07-25 16:23 ` [RFC v1 2/3] dts: VXLAN gpe support test suite Dean Marx
2024-07-26 20:18   ` Jeremy Spewock
2024-07-25 16:23 ` [RFC v1 3/3] dts: conf schema VXLAN gpe support Dean Marx

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAAA20UQLVHhfajvu+h_Smpm6S9xrnu4mw7QO_Etr7aiMhJtk0w@mail.gmail.com \
    --to=jspewock@iol.unh.edu \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=dev@dpdk.org \
    --cc=dmarx@iol.unh.edu \
    --cc=juraj.linkes@pantheon.tech \
    --cc=luca.vizzarro@arm.com \
    --cc=npratte@iol.unh.edu \
    --cc=paul.szczepanek@arm.com \
    --cc=probb@iol.unh.edu \
    --cc=yoan.picchi@foss.arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).