Re: [RFC v1 1/2] dts: add additional vlan configuration to testpmd shell class

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

Hey Nick,

Patch looks pretty good to me, just a few minor comments below.

On Mon, Aug 5, 2024 at 1:13 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> The ethertype test suite requires many different internal runtime
> vlan offload options to test ethertype configuration. The following patch
>  adds a new RxVlanOffloadOptions class that contains all the different
> vlan configuration options available within testpmd. Additionally, an
> extra method has beena added to adjust both the inner and outer tpids of

Extra "a" here at the end of "been."

> ingressing and egressing packets.
>
> Bugzilla ID: 1505
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> ---
<snip>
> +    def set_vlan_tpid(self, port_id: int, tpid: int, inner_id: bool) -> None:

Is this one of those things that you can't really verify from testpmd
output? That makes sense if it is, but we should probably note why
this method is different in the doc-string.

> +        """Set ethertype tpid values using the ethdev api.
> +
> +        Args:
> +            port_id: the ID of the port the tpid is being changed on.

"the" should be capitalized here.

> +            tpid: The tpid value being changed on the port.
> +            inner_id: If :data:`True`, set the inner tpid to the specified value. If
> +                :data:`False`, change the outer tpid to the specified value.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If either `port_id` or `tpid` value is invalid.
> +        """
> +        if tpid < 0 or tpid > 65535:
> +            raise InteractiveCommandExecutionError("Invalid TPID value given.")
> +        output = self.send_command(
> +            f'vlan set {"inner" if inner_id else "outer"} tpid {tpid} {port_id}'
> +        )
> +        if output.startswith("Invalid port"):
> +            raise InteractiveCommandExecutionError(f"Invalid port ID {port_id} given.")
> +
<snip>
>
> +    def set_vlan_offload_option(
> +        self, port: int, offload_option: VLANOffloadFlag, on: bool, verify: bool = True
> +    ) -> None:
> +        """Enable extended vlans on the specified port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            on: If :data:`True`, extended vlan turn on, otherwise it turns off.
> +            offload_options: The rx vlan offload option to be set.
> +            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.
> +        """
> +        offload_output = self.send_command(
> +            f"vlan set {str(offload_option).lower()} {'on' if on else 'off'} {port}"

Because flags can be combinations of multiple values, this might not
work if you passed in a flag that had STRIP and FILTER turned on at
the same time, for example. I think the string version would come out
to be something like STRIP|FILTER which definitely wouldn't be what
you want. Unfortunately, I think the only way around that though is to
have conditional values for each option and then checking to see if
the flag contains that option. Something like

if VLANOffloadFlag.STRIP in options:
    self.send_command(vlan set strip on 0)

Obviously with proper string formatting. Although, another thing you
might be able to do is if you can iterate through the members of the
VLANOffloadFlag class, you could do something more like:

for offload in VLANOffloadFlag.__members__:
    if offload in options:
        self.send_command(f"vlan set {offload.name.lower()} on 0")

And that would be a shorter way that covers you for any combination of
offloads. Could be fancy and make it a one-liner too if you wanted to,
but that's up to you.

> +        )
> +        if verify:
> +            if on ^ (str(offload_option) in str(self.show_port_info(port_id=port).vlan_offload)):

Is there a reason you can't directly compare the flags? The different
possible combinations of options will all have unique values, so I
think you should just be able to do `offload_option ==
self.show_port_info(port_id=port).vlan_offload)`



> +                self._logger.debug(
> +                    f"""Failed to set {offload_option} {'on' if on else 'off'} port {port}:
> +                        \n{offload_output}
> +                    """
> +                )
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set {offload_option} {'on' if on else 'off'} port {port}."
> +                )
> +
>      def port_stop_all(self, verify: bool = True) -> None:
>          """Stop all ports.
>
> --
> 2.44.0
>