DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jeremy Spewock <jspewock@iol.unh.edu>
To: Dean Marx <dmarx@iol.unh.edu>
Cc: Honnappa.Nagarahalli@arm.com, juraj.linkes@pantheon.tech,
	 probb@iol.unh.edu, paul.szczepanek@arm.com,
	yoan.picchi@foss.arm.com,  bruce.richardson@intel.com,
	luca.vizzarro@arm.com, dev@dpdk.org
Subject: Re: [PATCH v7 2/3] dts: add VLAN methods to testpmd shell
Date: Wed, 26 Jun 2024 14:22:56 -0400	[thread overview]
Message-ID: <CAAA20UQemJj7zV+h-51yi8dgVGGC6zBYn1pOWefXAVfosD4DsQ@mail.gmail.com> (raw)
In-Reply-To: <20240625153324.27257-2-dmarx@iol.unh.edu>

The functionality all looks good to me, just a few documentation
nit-picks and a suggestion for something that I think could shorten up
one function.

On Tue, Jun 25, 2024 at 11:34 AM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> added the following methods to testpmd shell class:
> vlan set filter on/off, rx vlan add/rm,
> vlan set strip on/off, port stop/start all/port,
> tx vlan set/reset, set promisc/verbose
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 278 ++++++++++++++++++
>  1 file changed, 278 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ec22f72221..d504e92a45 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -806,6 +806,284 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
<snip>
> +    def rx_vlan_add(self, vlan: int, port: int, verify: bool = True):
> +        """Add specified vlan tag to the filter list on a port.
> +
> +        Args:
> +            vlan: The vlan tag to add, should be within 1-1005, 1-4094 extended.
> +            port: The port number to add the tag on, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                the vlan tag was added to the filter list on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the tag
> +            is not added.

Just a little documentation nit-pick, I noticed this second line is
indented in most other methods, we should probably keep this
consistent.

> +        """
> +        vlan_add_output = self.send_command(f"rx_vlan add {vlan} {port}")
> +        if verify:
> +            if "VLAN-filtering disabled" in vlan_add_output or "Invalid vlan_id" in vlan_add_output:
> +                self._logger.debug(f"Failed to add vlan tag {vlan} on port {port}: \n{vlan_add_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to add vlan tag {vlan} on port {port}.")
> +
> +    def rx_vlan_rm(self, vlan: int, port: int, verify: bool = True):
> +        """Remove specified vlan tag from filter list on a port.
> +
> +        Args:
> +            vlan: The vlan tag to remove, should be within 1-4094.
> +            port: The port number to remove the tag from, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                the vlan tag was removed from the filter list on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the tag
> +            is not removed.

Same thing here.

> +        """
> +        vlan_rm_output = self.send_command(f"rx_vlan rm {vlan} {port}")
> +        if verify:
> +            if "VLAN-filtering disabled" in vlan_rm_output or "Invalid vlan_id" in vlan_rm_output:
> +                self._logger.debug(f"Failed to remove vlan tag {vlan} on port {port}: \n{vlan_rm_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to remove vlan tag {vlan} on port {port}.")
> +
<snip>
> +    def tx_vlan_set(self, port: int, vlan: int, verify: bool = True):
> +        """Set hardware insertion of vlan tags in packets sent on a port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            vlan: The vlan tag to insert, should be within 1-4094.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan insertion was enabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the insertion
> +            tag is not set.

This should probably also be indented.

> +        """
> +        vlan_insert_output = self.send_command(f"tx_vlan set {port} {vlan}")
> +        if verify:
> +            if ("Please stop port" in vlan_insert_output or "Invalid vlan_id" in vlan_insert_output
> +            or "Invalid port" in vlan_insert_output):
> +                self._logger.debug(f"Failed to set vlan insertion tag {vlan} on port {port}: \n{vlan_insert_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to set vlan insertion tag {vlan} on port {port}.")
> +
<snip>
> +    def set_promisc(self, port: int, on: bool, verify: bool = True):
> +        """Turns promiscuous mode on/off for the specified port
> +
> +        Args:
> +            port: Port number to use, should be within 0-32.
> +            on: If :data:`True`, turn promisc mode on, otherwise turn off.
> +            verify: If :data:`True` an additional command will be sent to verify that promisc mode
> +                is properly set. Defaults to :data:`True`.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and promisc mode
> +            is not correctly set.

This should probably also be indented.

> +        """
> +        if on:
> +            promisc_output = self.send_command(f"set promisc {port} on")
> +        else:
> +            promisc_output = self.send_command(f"set promisc {port} off")

You can inline this if-else-statement to shorten it up. Something like this:
promisc_output = self.send_command(f"set promisc {port} {'on' if on
else 'off''}")

> +        if verify:
> +            if (on and "Promiscuous mode: enabled" not in
> +            self.send_command(f"show port info {port}")):
> +                self._logger.debug(f"Failed to set promisc mode on port {port}: \n{promisc_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to set promisc mode on port {port}.")
> +            elif (not on and "Promiscuous mode: disabled" not in
> +            self.send_command(f"show port info {port}")):
> +                self._logger.debug(f"Failed to set promisc mode on port {port}: \n{promisc_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to set promisc mode on port {port}.")
> +

I actually mentioned this on Nick's patch, but I think this could be
shortened. I'll copy what I said there:

The logger output and the error seem to be exactly the same here so we
should avoid duplicating them. There are a few ways to go about
combining these two cases in the conditional but I would probably just
use an f-string to conditionally look for "enabled" vs "disabled".

I wonder if this check would be easier using the dataclass for port
info since it will be a boolean inside of the dataclass rather than
just searching for a string. I think if you had a boolean for if
promisc mode was on you could use an XOR of add and is_promisc_mode
and it would have the same effect. Normally I avoid using the
dataclass if the check is simple without it just because I think it is
slightly faster that way, but if there is a good use-case for it (like
there is here) then I think we might as well use it.

I think using the testpmd.show_port_info method would make for a
cleaner approach, so I slightly favor that one.


> +
> +    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 close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.44.0
>

  reply	other threads:[~2024-06-26 18:23 UTC|newest]

Thread overview: 79+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-11 16:15 [PATCH v2 0/2] VLAN test suite Dean Marx
2024-06-11 16:15 ` [PATCH v2 1/2] Initial implementation for " Dean Marx
2024-06-11 16:15 ` [PATCH v2 2/2] conf schema Dean Marx
2024-06-14 15:02 ` [PATCH v3 0/3] VLAN Test Suite Dean Marx
2024-06-14 15:02   ` [PATCH v3 1/3] Added VLAN commands to testpmd_shell class Dean Marx
2024-06-14 15:59     ` Patrick Robb
2024-06-14 20:29       ` Jeremy Spewock
2024-06-14 21:24         ` Patrick Robb
2024-06-17 14:37     ` Jeremy Spewock
2024-06-14 15:02   ` [PATCH v3 2/3] Initial implementation for VLAN test suite Dean Marx
2024-06-14 16:19     ` Patrick Robb
2024-06-17 14:56     ` Jeremy Spewock
2024-06-14 15:02   ` [PATCH v3 3/3] Config schema Dean Marx
2024-06-17 14:59     ` Jeremy Spewock
2024-06-17 14:35   ` [PATCH v3 0/3] VLAN Test Suite Jeremy Spewock
2024-06-17 17:50   ` Patrick Robb
2024-06-18 15:20   ` [PATCH v4 1/3] dts: refactored VLAN test suite Dean Marx
2024-06-18 15:20     ` [PATCH v4 2/3] dts: updated testpmd shell class Dean Marx
2024-06-18 15:20     ` [PATCH v4 3/3] dts: config schema Dean Marx
2024-06-18 16:29   ` [PATCH v5 1/3] dts: updated testpmd shell class Dean Marx
2024-06-18 16:29     ` [PATCH v5 2/3] dts: refactored VLAN test suite Dean Marx
2024-06-21 20:53       ` Jeremy Spewock
2024-06-18 16:29     ` [PATCH v5 3/3] dts: config schema Dean Marx
2024-06-21 20:53       ` Jeremy Spewock
2024-06-21 20:50     ` [PATCH v5 1/3] dts: updated testpmd shell class Jeremy Spewock
2024-06-24 18:17   ` [PATCH v6 " Dean Marx
2024-06-24 18:17     ` [PATCH v6 2/3] dts: refactored VLAN test suite Dean Marx
2024-06-24 18:17     ` [PATCH v6 3/3] dts: config schema Dean Marx
2024-06-25 15:33   ` [PATCH v7 1/3] dts: VLAN test suite implementation Dean Marx
2024-06-25 15:33     ` [PATCH v7 2/3] dts: add VLAN methods to testpmd shell Dean Marx
2024-06-26 18:22       ` Jeremy Spewock [this message]
2024-06-25 15:33     ` [PATCH v7 3/3] dts: config schema Dean Marx
2024-06-26 18:23       ` Jeremy Spewock
2024-06-26 18:21     ` [PATCH v7 1/3] dts: VLAN test suite implementation Jeremy Spewock
2024-06-28 14:00   ` [PATCH v8 1/3] dts: add VLAN methods to testpmd shell Dean Marx
2024-06-28 14:00     ` [PATCH v8 2/3] dts: VLAN test suite implementation Dean Marx
2024-07-01 19:52       ` Jeremy Spewock
2024-06-28 14:00     ` [PATCH v8 3/3] dts: config schema Dean Marx
2024-07-01 19:48     ` [PATCH v8 1/3] dts: add VLAN methods to testpmd shell Jeremy Spewock
2024-07-03 16:47   ` [PATCH v9 1/3] dts: config schema Dean Marx
2024-07-03 16:47     ` [PATCH v9 2/3] dts: VLAN test suite implementation Dean Marx
2024-07-03 16:47     ` [PATCH v9 3/3] dts: add VLAN methods to testpmd shell Dean Marx
2024-07-09 21:22       ` Jeremy Spewock
2024-07-03 16:50   ` [PATCH v10 1/3] " Dean Marx
2024-07-03 16:50     ` [PATCH v10 2/3] dts: VLAN test suite implementation Dean Marx
2024-07-09 21:22       ` Jeremy Spewock
2024-07-03 16:50     ` [PATCH v10 3/3] dts: config schema Dean Marx
2024-07-05 15:55       ` Patrick Robb
2024-07-10 15:38       ` Jeremy Spewock
2024-07-17 20:31   ` [PATCH v11 1/3] dts: add VLAN methods to testpmd shell Dean Marx
2024-07-17 20:31     ` [PATCH v11 2/3] dts: VLAN test suite implementation Dean Marx
2024-07-19 15:35       ` Jeremy Spewock
2024-07-17 20:31     ` [PATCH v11 3/3] dts: config schema Dean Marx
2024-07-19 15:35       ` Jeremy Spewock
2024-07-19 15:35     ` [PATCH v11 1/3] dts: add VLAN methods to testpmd shell Jeremy Spewock
2024-07-24 16:30 ` [PATCH v12 0/3] dts: refactored VLAN test suite Dean Marx
2024-07-24 16:30   ` [PATCH v12 1/3] dts: add VLAN methods to testpmd shell Dean Marx
2024-07-24 16:30   ` [PATCH v12 2/3] dts: VLAN test suite implementation Dean Marx
2024-07-24 16:30   ` [PATCH v12 3/3] dts: config schema Dean Marx
2024-08-07 19:43   ` [PATCH v13 0/2] dts: refactored VLAN test suite Dean Marx
2024-08-07 19:43     ` [PATCH v13 1/2] dts: add VLAN methods to testpmd shell Dean Marx
2024-08-09 17:23       ` Jeremy Spewock
2024-08-07 19:43     ` [PATCH v13 2/2] dts: VLAN test suite implementation Dean Marx
2024-08-09 17:23       ` Jeremy Spewock
2024-08-23 21:16     ` [PATCH v14 0/2] dts: port over VLAN test suite Dean Marx
2024-08-23 21:16       ` [PATCH v14 1/2] dts: add VLAN methods to testpmd shell Dean Marx
2024-09-04 19:49         ` Jeremy Spewock
2024-08-23 21:16       ` [PATCH v14 2/2] dts: VLAN test suite implementation Dean Marx
2024-09-04 19:49         ` Jeremy Spewock
2024-09-11 17:43     ` [PATCH v14 0/1] dts: port over VLAN test suite Dean Marx
2024-09-11 17:43       ` [PATCH v14 1/1] dts: VLAN test suite implementation Dean Marx
2024-09-18 20:38       ` [PATCH v15 0/1] dts: port over VLAN test suite Dean Marx
2024-09-18 20:38         ` [PATCH v15 1/1] dts: VLAN test suite implementation Dean Marx
2024-09-18 20:49       ` [PATCH v15 0/1] dts: port over VLAN test suite Dean Marx
2024-09-18 20:49         ` [PATCH v15] dts: VLAN test suite implementation Dean Marx
2024-10-08 17:20           ` [PATCH v16 0/1] dts: port over VLAN test suite Dean Marx
2024-10-08 17:20             ` [PATCH v16] dts: VLAN test suite implementation Dean Marx
2024-10-08 19:18               ` [PATCH v17 0/1] port over VLAN test suite Dean Marx
2024-10-08 19:18                 ` [PATCH v17 1/1] dts: VLAN test suite implementation 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=CAAA20UQemJj7zV+h-51yi8dgVGGC6zBYn1pOWefXAVfosD4DsQ@mail.gmail.com \
    --to=jspewock@iol.unh.edu \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=dmarx@iol.unh.edu \
    --cc=juraj.linkes@pantheon.tech \
    --cc=luca.vizzarro@arm.com \
    --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).