<div dir="ltr"><div dir="ltr">On Fri, Aug 23, 2024 at 10:54 AM Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu">jspewock@iol.unh.edu</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Aug 21, 2024 at 12:25 PM Dean Marx <<a href="mailto:dmarx@iol.unh.edu" target="_blank">dmarx@iol.unh.edu</a>> wrote:<br>
><br>
> add csum_set_hw method to testpmd shell class. Port over<br>
> set_verbose and port start/stop from queue start/stop suite.<br>
<br>
Since we had that discussion in a DTS meeting about there not really<br>
being a rule against multiple dependencies or anything like that, I<br>
think it might be better if we start moving to just always depending<br>
on other patches rather than duplicating code in between multiple<br>
series'. Not a call out to you at all because I think I have multiple<br>
patches open right now where I also borrow from other suites because I<br>
didn't want long dependency lists, but I think the lists of<br>
dependencies might end up being easier to track than where the code is<br>
from. It also makes for more targeted commit messages.<br>
<br>
Let me know what you think though. This might be something worth<br>
talking about with the larger development group as well to get more<br>
opinions on it.<br></blockquote><div><snip></div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +class ChecksumOffloadOptions(Flag):<br>
> + """Flag representing checksum hardware offload layer options."""<br>
> +<br>
> + #:<br>
> + ip = auto()<br>
> + #:<br>
> + udp = auto()<br>
> + #:<br>
> + tcp = auto()<br>
> + #:<br>
> + sctp = auto()<br>
> + #:<br>
> + outerip = auto()<br>
> + #:<br>
> + outerudp = auto()<br>
> +<br>
> + def __str__(self):<br>
> + """String method for use in csum_set_hw."""<br>
> + if self == ChecksumOffloadOptions.outerip:<br>
> + return "outer-ip"<br>
> + elif self == ChecksumOffloadOptions.outerudp:<br>
> + return "outer-udp"<br>
<br>
It might be easier to name these values outer_ip and outer_udp and<br>
then just do a str.replace("_", "-") on them to get the same result.<br></blockquote><div><br></div><div>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:</div><div><br></div><div>for name, offload in ChecksumOffloadOptions.__members__.items():</div><div> if offload in layer:</div><div> (action)</div><div><br></div><div>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.</div><div> </div><div><snip></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I honestly didn't know the `title()` method of a string existed in<br>
python until I just did a little searching and it seems strange to me,<br>
but it would be helpful for this use case. It also is weird to me that<br>
they would have everything other than outer-ip and outer-udp be all<br>
upper case.</blockquote><div><br></div><div> 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. </div></div></div>