Re: [PATCH v3 1/2] dts: add csum HW offload to testpmd shell

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

Hey Dean,

This patch is looking good, I like the changes from the previous
version, there were just a few comments that I left but they should be
pretty easy fixes.

On Mon, Aug 26, 2024 at 5:22 PM Dean Marx <dmarx@iol.unh.edu> wrote:
<snip>
> +    def csum_set_hw(self, layer: ChecksumOffloadOptions, port_id: int, verify: bool = True) -> None:
> +        """Enables hardware checksum offloading on the specified layer.
> +
> +        Args:
> +            layer: The layer that checksum offloading should be enabled on.
> +            port_id: The port number to enable checksum offloading on, should be within 0-32.
> +            verify: If :data:`True` the output of the command will be scanned in an attempt to
> +                verify that checksum offloading was enabled on the port.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If checksum offload is not enabled successfully.
> +        """
> +        for name, offload in ChecksumOffloadOptions.__members__.items():
> +            if offload in layer:
> +                name = name.replace("_", "-")
> +                csum_output = self.send_command(f"csum set {name} hw {port_id}")
> +            if verify:

I think this line and all of the ones below it in this method need to
be indented one more time so that they are only done if the offload is
one that the user is trying to set. csum_output might not exist if the
previous statement is false, so I don't think we would be able to even
use it in that case, but even if we could I don't think we really need
to verify if the offload is set or not if it isn't in `layer`.

> +                if ("Bad arguments" in csum_output
> +                    or f"Please stop port {port_id} first" in csum_output
> +                        or f"checksum offload is not supported by port {port_id}" in csum_output):
> +                    self._logger.debug(f"Csum set hw error:\n{csum_output}")
> +                    raise InteractiveCommandExecutionError(
> +                        f"Failed to set csum hw {name} mode on port {port_id}"
> +                    )
> +            success = False
> +            if "-" in name:
> +                name.title()

For both this and the call to upper() below, they return a new string
that has the results applied, so `name` here would have to get
re-assigned for the changes to take place. So I think this line would
need to be `name = name.title()`.

> +            else:
> +                name.upper()
> +            if f"{name} checksum offload is hw" in csum_output:

Another thing you could do which I should have thought about sooner is
you could just change this if-statement to be:

if f"{name} checksum offload is hw" in csum_output.lower():

and then you wouldn't need to worry at all about which letters are
uppercase or lowercase in the output and it would all be the same so
you don't have to do this either upper() or title() call. It might be
technically less efficient, but we've mentioned before that we're fine
with that as long as it isn't grossly inefficient, so if it is easier
to just make it all lowercase then I think it is fine :).

> +                success = True
> +            if not success and verify:
> +                self._logger.debug(f"Failed to set csum hw mode on port {port_id}:\n{csum_output}")

Should we also raise the exception here as a failure?


> +
>      def _close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.stop()
> --
> 2.44.0
>