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 v3 1/3] Added VLAN commands to testpmd_shell class
Date: Mon, 17 Jun 2024 10:37:53 -0400	[thread overview]
Message-ID: <CAAA20US8w9t-Ci8W18qwbBPicRR7_ah6bLpOHxdB2syfUjQY9Q@mail.gmail.com> (raw)
In-Reply-To: <20240614150238.26374-2-dmarx@iol.unh.edu>

In the methods you added here, you made all of the parameters to them
optional with a default. This might make sense to allow developers to
be less verbose when calling the method with the defaults, but in the
case of things like the port ID, vlan IDs, and things of that nature I
think making these positional arguments that are required makes more
sense for readability. For example, if I were to call
testpmd.vlan_filter_set_on(), it is not clear where this vlan filter
is being modified. The default is 0 so it will set it on port 0, but
if you're just reading the test suite you wouldn't know that without
looking at the testpmd class. Verify is left as an optional parameter
because we really want to verify that the commands were successful in
all cases, but there could be an obscure reason in the future that a
developer might not really care if a command to testpmd was successful
or not.

I also noticed in most cases where you were verifying commands that
you had to check if multiple failures messages were present. Would it
be easier to check if the command was successful by querying things
like VLANs on a port or VLAN filtering status of a port instead of
trying to cover all error cases?

On Fri, Jun 14, 2024 at 11:03 AM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> The testpmd_shell class is intended for sending commands to
>  a remote testpmd session within a test suite, and as of right now
> it is missing most commands. Added commands for vlan filtering,
> stripping, and insertion, as well as stopping and starting ports
> during testpmd runtime.

While it's true that the testpmd class is missing a lot of commands
that exist in testpmd, that isn't really why you are adding them.
Maybe switching this to say something like "It is missing
functionality required for verifying VLAN features on a PMD." might be
more descriptive.

>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
<snip>
> +    def vlan_filter_set_on(self, port: int = 0, verify: bool = True):
> +        """Set vlan filter on.
> +
> +        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 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 {port}")
> +        if verify:
> +            if "Invalid port" in filter_cmd_output:

This doesn't really verify that the filter was set as much as it
verifies that the port exists. Is there anything that testpmd prints
when it is successful, or possibly a way to query existing VLAN
filters on a port to see if it was set?

> +                self._logger.debug(f"Failed to enable vlan filter: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to enable vlan filter.")
> +
> +    def vlan_filter_set_off(self, port: int = 0, verify: bool = True):
> +        """Set vlan filter off.
> +
> +        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 filtering was disabled 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 off {port}")
> +        if verify:
> +            if "Invalid port" in filter_cmd_output:

Same thing as above comment about verification.

> +                self._logger.debug(f"Failed to disable vlan filter: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to disable vlan filter.")
> +
<snip>
> +    def vlan_strip_set_on(self, port: int = 0, verify: bool = True):
> +        """Enable vlan stripping on the specified 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 stripping was enabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        vlan_strip_output = self.send_command(f"vlan set strip on {port}")
> +        if verify:
> +            if "Invalid port" in vlan_strip_output:

This is similar to above where it isn't really verifying that the vlan
stripping was enabled.

> +                self._logger.debug(f"Failed to add vlan tag: \n{vlan_strip_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to add vlan tag.")
> +
> +    def vlan_strip_set_off(self, port: int = 0, verify: bool = True):
> +        """Disable vlan stripping on the specified 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 stripping was disabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        vlan_strip_output = self.send_command(f"vlan set strip off {port}")
> +        if verify:
> +            if "Invalid port" in vlan_strip_output:

Same here.

> +                self._logger.debug(f"Failed to enable stripping: \n{vlan_strip_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to enable stripping on port {port}.")

On all of the other verification methods the message for the error
doesn't include specifics like what port it was on or what vlan tag
failed. I am generally more in favor of including specific information
like this and many of the other testpmd methods do, so if possible I
think we should add some of this information to the other methods here
as well to make it consistent.

> +
<snip>
> 2.44.0
>

  parent reply	other threads:[~2024-06-17 14:38 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 [this message]
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
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=CAAA20US8w9t-Ci8W18qwbBPicRR7_ah6bLpOHxdB2syfUjQY9Q@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).