Re: [PATCH v13 1/2] dts: add VLAN methods to testpmd shell

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Wed, Aug 7, 2024 at 3:43 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>
> ---
<snip>
>
> +    def vlan_filter_set(self, port: int, on: bool, verify: bool = True) -> None:
> +        """Set vlan filter on.
> +
> +        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.
> +            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 still think that we should avoid comparing strings here.

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

"adds" here should be capitalized

> +            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) -> None:
> +        """Enable vlan stripping on the specified port.
> +
> +        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, we should probably compare the flag values 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) -> None:
> +        """Stop all ports.
> +
> +        Args:
> +            port: Specifies the port number to use, must be between 0-32.

This method no longer has a port parameter, so it would probably be
better to remove this.

> +            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) -> None:
> +        """Start all ports.
> +
> +        Args:
> +            port: Specifies the port number to use, must be between 0-32.

This method also does not have a port parameter.

> +            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.

It's probably better to indent this just to make it more clear that it
is a continuation line.

> +        """
> +        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) -> None:
> +        """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 this is the incorrect command, when I tried to use this in a
test suite it was throwing 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
>