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 v9 3/3] dts: add VLAN methods to testpmd shell
Date: Tue, 9 Jul 2024 17:22:33 -0400	[thread overview]
Message-ID: <CAAA20URQFGejzC1eYAoY3kcVn3yErVqv4ZX423f9FFRE1cva3A@mail.gmail.com> (raw)
In-Reply-To: <20240703164703.16809-3-dmarx@iol.unh.edu>

Hey Dean, one other thing that I just noticed which I should have
picked up on sooner is that all of the methods that you added here are
missing return type annotations. I believe these are generally things
we look for on all methods, and I'm surprised that we don't have
something that checks for this in the formatting script. Maybe that
would be a good thing to add. Obviously the methods don't return
anything so it doesn't harm much in the way of type-checking, but it
is generally better to be more explicit and mark that the methods
actually don't return anything.

On Wed, Jul 3, 2024 at 12:47 PM 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
> fixed: bug in vlan_offload for
> show port info, removed $ at end of regex
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 245 +++++++++++++++++-
>  1 file changed, 244 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ec22f72221..09d3bda5d6 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -98,7 +98,7 @@ def make_parser(cls) -> ParserFn:
>                  r"strip (?P<STRIP>on|off), "
>                  r"filter (?P<FILTER>on|off), "
>                  r"extend (?P<EXTEND>on|off), "
> -                r"qinq strip (?P<QINQ_STRIP>on|off)$",
> +                r"qinq strip (?P<QINQ_STRIP>on|off)",
>                  re.MULTILINE,
>                  named=True,
>              ),
> @@ -806,6 +806,249 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
> +    def vlan_filter_set(self, port: int, on: bool, verify: bool = True):
> +        """Set vlan filter on.

With that change I was thinking that, while it isn't a big enough deal
to justify a new version on its own, maybe this subject line of the
doc-string could be expanded on. Just because it saying that it sets
VLAN filtering on might be a little vague on its own. It makes more
sense once you see that one of the parameters is a port, but maybe
even just adding to this that it "Enables/Disables VLAN filtering on a
port" might be more clear that it is on one port and not all of them
and things like that.

> +
> +        Args:
> +            port: The port number to enable VLAN filter on, should be within 0-32.
> +            on: Sets filter on if :data:`True`, otherwise turns off.

This feels like it's missing a few words that would make it flow
better, particularly "otherwise turns off" seems off to me. Maybe
something like "if :data:`True` turn filtering on, otherwise turn it
off" could work.

> +            verify: If :data:`True`, the output of the command and show port info
> +                is scanned to verify that vlan filtering was enabled successfully.
> +                If not, it is considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        filter_cmd_output = self.send_command(f"vlan set filter {'on' if on else 'off'} {port}")
> +        if verify:
> +            if on ^ ("FILTER" in str(self.show_port_info(port_id=port).vlan_offload)):

I was wondering how Flags handle being converted into a string and it
seems like they just sort of put all of the other Flags they were
combined with into a string separated by "|". Just to help keep things
more stable in the case of things being renamed or anything, you can
also do this same "in" comparison using the flag for filtering itself.
So, for example, `VLANOffloadFlag.FILTER in
self.show_port_info(port_id=port).vlan_offload`

> +                self._logger.debug(f"Failed to set filter on port {port}: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set VLAN filter on port {port}."
> +                )
> +
> +    def rx_vlan(self, vlan: int, port: int, add: bool, verify: bool = True):
> +        """Add specified vlan tag to the filter list on a port.

Since there is a parameter for whether or not we are adding or
removing, this doc-string should likely reflect that rather than just
saying it adds a specified VLAN.

> +
> +        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.
> +            add: adds the tag if :data:`True`, otherwise removes tag.

Probably better to say "removes the tag" here.

> +            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.
> +        """
> +        rx_output = self.send_command(f"rx_vlan {'add' if add else 'rm'} {vlan} {port}")
> +        if verify:
> +            if (
> +                "VLAN-filtering disabled" in rx_output
> +                or "Invalid vlan_id" in rx_output
> +                or "Bad arguments" in rx_output
> +            ):
> +                self._logger.debug(
> +                    f"Failed to {'add' if add else 'remove'} tag {vlan} port {port}: \n{rx_output}"
> +                )
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to {'add' if add else 'remove'} tag {vlan} on port {port}."
> +                )
> +
> +    def vlan_strip_set(self, port: int, on: bool, verify: bool = True):
> +        """Enable vlan stripping on the specified port.

This subject line should also reflect that it can be enabled or
disabled in the method.

> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            on: If :data:`True`, will turn strip on, otherwise will turn off.
> +            verify: If :data:`True`, the output of the command and show port info
> +                is scanned to verify that vlan stripping was enabled on the specified port.
> +                If not, it is considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and stripping
> +                fails to update.
> +        """
> +        strip_output = self.send_command(f"vlan set strip {'on' if on else 'off'} {port}")
> +        if verify:
> +            if on ^ ("STRIP" in str(self.show_port_info(port_id=port).vlan_offload)):\

Same thing here as above, it might be better to compare the Flags here
rather than strings.

> +                self._logger.debug(
> +                    f"Failed to set strip {'on' if on else 'off'} port {port}: \n{strip_output}"
> +                )
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set strip {'on' if on else 'off'} port {port}."
> +                )
> +
> +    def port_stop_all(self, verify: bool = True):
> +        """Stop all ports.
> +
> +        Args:
> +            port: Specifies the port number to use, must be between 0-32.

This looks like it isn't included in the parameters anymore so it's
probably better to remove this part of the doc-string as well.

> +            verify: If :data:`True`, the output of the command is scanned
> +                to ensure all ports are stopped. If not, it is considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and all ports
> +                fail to stop.
> +        """
> +        port_output = self.send_command("port stop all")
> +        if verify:
> +            if "Done" not in port_output:
> +                self._logger.debug(f"Failed to stop all ports: \n{port_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to stop all ports.")
> +
<snip>
> +
> +    def port_start_all(self, verify: bool = True):
> +        """Start all ports.
> +
> +        Args:
> +            port: Specifies the port number to use, must be between 0-32.

Same here.

> +            verify: If :data:`True`, the output of the command is scanned
> +                to ensure all ports are started. If not, it is considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and all ports
> +            fail to start.
> +        """
> +        port_output = self.send_command("port start all")
> +        if verify:
> +            if "Done" not in port_output:
> +                self._logger.debug(f"Failed to start all ports: \n{port_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to start all ports.")
> +
<snip>
> +    def tx_vlan_reset(self, port: int, verify: bool = True):
> +        """Disable hardware insertion of vlan tags in packets sent on a port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan insertion was disabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the insertion
> +                tag is not reset.
> +        """
> +        vlan_insert_output = self.send_command(f"tx_vlan set {port}")

I think the testpmd command to disable hardware insertion is actually
`tx_vlan reset {port_id}`, when I ran this command I was getting
errors.

> +        if verify:
> +            if "Please stop port" in vlan_insert_output or "Invalid port" in vlan_insert_output:
> +                self._logger.debug(
> +                    f"Failed to reset vlan insertion on port {port}: \n{vlan_insert_output}"
> +                )
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to reset vlan insertion on port {port}."
> +                )
> +
<snip>
> 2.44.0
>

  reply	other threads:[~2024-07-09 21:22 UTC|newest]

Thread overview: 75+ 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
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 [this message]
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

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=CAAA20URQFGejzC1eYAoY3kcVn3yErVqv4ZX423f9FFRE1cva3A@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).