On Wed, Aug 21, 2024 at 12:25 PM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> add csum_set_hw method to testpmd shell class. Port over
> set_verbose and port start/stop from queue start/stop suite.
Since we had that discussion in a DTS meeting about there not really
being a rule against multiple dependencies or anything like that, I
think it might be better if we start moving to just always depending
on other patches rather than duplicating code in between multiple
series'. Not a call out to you at all because I think I have multiple
patches open right now where I also borrow from other suites because I
didn't want long dependency lists, but I think the lists of
dependencies might end up being easier to track than where the code is
from. It also makes for more targeted commit messages.
Let me know what you think though. This might be something worth
talking about with the larger development group as well to get more
opinions on it.
<snip>
I actually like that idea a lot, I'm going to add the dependency and remove the corresponding methods, especially since it probably makes the maintainer's jobs easier when there's less code duplication. I can also send a message in the slack chat about this to see what other people think.
> +class ChecksumOffloadOptions(Flag):
> + """Flag representing checksum hardware offload layer options."""
> +
> + #:
> + ip = auto()
> + #:
> + udp = auto()
> + #:
> + tcp = auto()
> + #:
> + sctp = auto()
> + #:
> + outerip = auto()
> + #:
> + outerudp = auto()
> +
> + def __str__(self):
> + """String method for use in csum_set_hw."""
> + if self == ChecksumOffloadOptions.outerip:
> + return "outer-ip"
> + elif self == ChecksumOffloadOptions.outerudp:
> + return "outer-udp"
It might be easier to name these values outer_ip and outer_udp and
then just do a str.replace("_", "-") on them to get the same result.
Makes sense, I ended up just getting rid of the __str__ method entirely and iterating through the options within the csum set hw method with the __members__ method you mentioned later in this review. I was able to create a for loop that looks like this:
for name, offload in ChecksumOffloadOptions.__members__.items():
if offload in layer:
(action)
where .items() returns all the flags in a dictionary, where the key is a string of the flag name and the offload value is the actual flag instance from the class. This way I could just call name = name.replace("_", "-") within the loop and use name for send_command and offload for comparing flags.
<snip>
I honestly didn't know the `title()` method of a string existed in
python until I just did a little searching and it seems strange to me,
but it would be helpful for this use case. It also is weird to me that
they would have everything other than outer-ip and outer-udp be all
upper case.
Yeah it is really odd, I'm not sure if they had consistency in mind while developing this part of testpmd. The title command is a great idea though, I added that to the second part of the csum method and it really simplified everything.