Re: [PATCH 2/3] dts: add testpmd methods for test suite

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

I think the subject line of this commit makes sense in the context of
this series, but might be less helpful when the commit is applied to
the git tree. For this reason I might favor changing it to something
that briefly says what the added testpmd commands actually do.

On Fri, Jun 21, 2024 at 1:23 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> Several methods are either imported from currently in-develoment test
> suites, or they have been created specifically for this test suite.
>
> Methods here that can be found in other test suites include vlan
> filtering, adding and removing vlan tags, and setting promiscuous mode.
>
> Methods that are specific to this test suite include adding or removing
> mac addresses, and adding or removing multicast mac addresses.

You probably could make the subject about the adding/modification of
mac addresses judging by the body.

>
> Bugzilla ID: 1454
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 209 ++++++++++++++++++
>  1 file changed, 209 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ec22f72221..8a7d5905b3 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -767,6 +767,215 @@ def show_port_info(self, port_id: int) -> TestPmdPort:
>
>          return TestPmdPort.parse(output)
>
> +    def set_mac_addr(self, port_id: int, mac_address: str, add: bool = True, verify: bool = True):

I'm not sure it makes sense to default the `add` parameter in this
method or the following. It seems to me like this method would likely
be split pretty evenly between adding and removing and I can't think
of a reason why one would be done in the general case over the other,
so I think it would be more clear if we didn't default it.

> +        """Add or remove a mac address on a given port.
> +

Is it worth mentioning that we are adding the MAC address to the
allowlist of the port?

> +        Args:
> +            port_id: The port ID the mac address is set on.
> +            mac_address: The mac address to be added or removed to the specified port.
> +            add: If :data:`True`, add the specified mac address. If :data:`False`, remove specified
> +                mac address.
> +            verify: If :data:'True', assert that the 'mac_addr' operation was successful. If
> +                :data:'False', run the command and skip this assertion.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If either the 'add' or 'remove' operations fail.

This error documentation confuses me a little because it sounds like
both `add` and `remove` operations are happening in this method when
really either one happens or the other happens. Maybe changing this to
just saying the "mac address operation" failed would fix this.

> +        """

I think something that could save you a lot of space in this method
(and the next one) is creating a variable for what command to use if
`add` is true or not.
You could do something like this:

mac_cmd = "add" if add else "remove"

> +        if add:
> +            output = self.send_command(f"mac_addr add {port_id} {mac_address}")
> +        else:
> +            output = self.send_command(f"mac_addr remove {port_id} {mac_address}")

Then you don't need this if-statement because this just becomes:
output = self.send_command(f"mac_addr {mac_cmd} {port_id} {mac_address}")

> +        if "Bad arguments" in output:
> +            self._logger.debug("Invalid argument provided to mac_addr")
> +            raise InteractiveCommandExecutionError("Invalid argument provided")
> +
> +        if verify:

If you have the variable you can also remove this if-statement about
which output/error to provide. Since it's the same error in either
case you can just do:

if "mac_addr_cmd error" in output:
    self._logger.debug(f"Failed to {mac_cmd} ...

> +            if add and "mac_addr_cmd error:" in output:
> +                self._logger.debug(f"Failed to add {mac_address} on port {port_id}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to add {mac_address} on port {port_id} \n{output}"
> +                )
> +            if not add and "mac_addr_cmd error" in output:
> +                self._logger.debug(f"Failed to remove {mac_address} on port {port_id}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to remove {mac_address} on port {port_id} \n{output}"
> +                )
> +
> +    def set_multicast_mac_addr(
> +        self, port_id: int, multi_addr: str, add: bool = True, verify: bool = True
> +    ):
> +        """Add or remove multicast mac address to a specified port filter.
> +
> +        Args:
> +            port_id: The port ID the multicast address is set on.
> +            multi_addr: The multicast address to be added to the filter.
> +            add: If :data:'True', add the specified multicast address to the port filter.
> +                If :data:'False', remove the specified multicast address from the port filter.
> +            verify: If :data:'True', assert that the 'mcast_addr' operations was successful.
> +                If :data:'False', execute the 'mcast_addr' operation and skip the assertion.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If either the 'add' or 'remove' operations fails.
> +        """
> +        if add:
> +            output = self.send_command(f"mcast_addr add {port_id} {multi_addr}")
> +        else:
> +            output = self.send_command(f"mcast_addr remove {port_id} {multi_addr}")
> +        if "Bad arguments" in output:
> +            self._logger.debug("Invalid arguments provided to mcast_addr")
> +            raise InteractiveCommandExecutionError("Invalid argument provided")
> +
> +        if verify:
> +            if (
> +                add
> +                and "Invalid multicast_addr"
> +                or "multicast address already filtered by port" in output

If you use the variable here as well, this is a little trickier
because this last line in the condition is different. Still can be
solved however by making it:
f"multicast address {'already' if add else 'not'} filtered by port"

> +            ):
> +                self._logger.debug(f"Failed to add {multi_addr} on port {port_id}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to add {multi_addr} on port {port_id} \n{output}"
> +                )
> +            if (
> +                not add
> +                and "Invalid multicast addr"

Is this a typo or does testpmd remove the underscore when you're removing?

> +                or "multicast address not filtered by port" in output
> +            ):
> +                self._logger.debug(f"Failed to remove {multi_addr} on port {port_id}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to remove {multi_addr} on port {port_id} \n{output}"
> +                )
> +
<snip>
> +
> +    def rx_vlan_rm(self, vlan: int, port: int, verify: bool = True):
> +        """Remove specified vlan tag from filter list on a port.
> +
> +        Args:
> +            vlan: The vlan tag to remove, should be within 1-4094.
> +            port: The port number to remove the tag from, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                the vlan tag was removed from 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 removed.

Other methods have this second line indented. I'm not sure if I
addressed this on the VLAN suite already or not but this should
probably do the same.

> +        """
<snip>
> +
> +    def set_promisc(self, port: int, on: bool, verify: bool = True):
> +        """Turns promiscuous mode on/off for the specified port.
> +
> +        Args:
> +            port: port number to use, should be within 0-32.
> +            on: if :data:`True`, turn promisc mode on, otherwise turn off.
> +            verify: if :data:`True` an additional command will be sent to verify that promisc mode
> +                is properly set. Defaults to :data:`True`.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and promisc mode
> +            is not correctly set.
> +        """
> +        if on:
> +            promisc_output = self.send_command(f"set promisc {port} on")
> +        else:
> +            promisc_output = self.send_command(f"set promisc {port} off")

This can also be done with a variable, or you can inline it by doing
{'on' if on else 'off'}.

> +        if verify:
> +            if on and "Promiscuous mode: enabled" not in self.send_command(
> +                f"show port info {port}"
> +            ):
> +                self._logger.debug(f"Failed to set promisc mode on port {port}: \n{promisc_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set promisc mode on port {port}."
> +                )
> +            elif not on and "Promiscuous mode: disabled" not in self.send_command(
> +                f"show port info {port}"
> +            ):
> +                self._logger.debug(f"Failed to set promisc mode on port {port}: \n{promisc_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set promisc mode on port {port}."
> +                )

The logger output and the error seem to be exactly the same here so we
should avoid duplicating them. There are a few ways to go about
combining these two cases in the conditional but I would probably just
use an f-string to conditionally look for "enabled" vs "disabled".

I wonder if this check would be easier using the dataclass for port
info since it will be a boolean inside of the dataclass rather than
just searching for a string. I think if you had a boolean for if
promisc mode was on you could use an XOR of add and is_promisc_mode
and it would have the same effect. Normally I avoid using the
dataclass if the check is simple without it just because I think it is
slightly faster that way, but if there is a good use-case for it (like
there is here) then I think we might as well use it.

I think using the testpmd.show_port_info method would make for a
cleaner approach, so I slightly favor that one.


> +
>      def show_port_stats_all(self) -> list[TestPmdPortStats]:
>          """Returns the statistics of all the ports.
>
> --
> 2.44.0
>