Re: [PATCH v9 3/3] dts: add VLAN methods to testpmd shell

[Jeremy Spewock <jspewock@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
>