Re: [PATCH v3 1/3] Added VLAN commands to testpmd_shell class

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