Re: [PATCH v2 2/2] dts: checksum offload test suite

[Dean Marx <dmarx@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 4794 bytes --]

<snip>

> You could probably combine this line with the previous since they are
> from the same module.
>
> > +from scapy.packet import Raw  # type: ignore[import-untyped]
>
> I think you can also import `Packet` from this module if you wanted to
> combine another two lines as well.
>
>
Wow I didn't even notice that good catch


> > +
> > +from framework.remote_session.testpmd_shell import (
> > +    SimpleForwardingModes,
>
> This reminds me of a question I've had for a little while now which
> is, should this be imported from the module that it originates from
> (params) or is it fine to just grab it from the testpmd shell where it
> is also imported? I guess I don't really see this causing a problem at
> all since there isn't really a chance of any circular imports in this
> case or things that would be breaking, but I just don't know if there
> is any kind of guideline regarding these scenarios.
>

I briefly looked for some best practice guidelines about this kind of thing
but I couldn't find anything explicit. However, I'm going to assume it's
probably preferred to do a direct import like you mentioned so I'm going to
change that.

<snip>

> > +        testpmd.start()
> > +        self.send_packet_and_capture(packet=packet)
> > +        verbose_output = testpmd.extract_verbose_output(testpmd.stop())
> > +        for packet in verbose_output:
> > +            if packet.dst_mac == "00:00:00:00:00:01":
>
> Since this method is the one that relies on this MAC address being set
> on the packet, it might be helpful to set that MAC on the packet
> before sending it in the same method. Why this address is the one you
> were searching for would then be clear at just a glance at the send
> method which could be useful. That or you could note in the doc-string
> what you expect the MAC to be.
>

Funny that you mentioned this because I actually tried to set the
destination mac address within the send_packet_and_verify_checksums method
and I couldn't get it to work no matter what I tried. For some reason even
though the mac address was the one I set after send_packet_and_capture,
none of the packets in verbose_output would have that mac address. I tried
debugging for a while but I just couldn't get it to work, even with the
adjust_addresses patch applied it was breaking so I just removed it
entirely and set them all in the test cases, which worked fine. I did add
an extra arg to the send_and_verify_checksums method called ID, which I
passed the mac_id variable to so that it's cleaner.


>
> > +                if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in
> packet.ol_flags:
> > +                    isIP = True
> > +                else:
> > +                    isIP = False
> > +                if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in
> packet.ol_flags:
> > +                    isL4 = True
> > +                else:
> > +                    isL4 = False
> > +            else:
> > +                isIP = False
> > +                isL4 = False
>
> Would having this else statement break the booleans if there was
> another noise packet after the one that you sent in the verbose
> output? I think that would make both of these booleans false by the
> end of the loop even if one was supposed to be true. You might be able
> to fix this however with either a break statement after you find the
> first packet with the right MAC address, or you could use the built-in
> `any()` function that python offers.
>

Yeah you're right, not sure what I was thinking there haha. I simplified it
back down to two lines in the new version.

<snip>

>
> Not really important at all (especially because it won't affect the
> order that the cases are run in) but it might make sense to put the
> two individual tests of l3_rx and l4_rx before the test of both of
> them combined. Again, super nit-picky, but it makes sense in my head
> to see the individual parts tested and then see them tested together.
> I'll leave it up to you if you think it makes sense to just leave it
> as is :).
>

Makes sense to me, swapped them


>
> > +
> > +    def test_vlan_checksum(self) -> None:
> > +        """Tests VLAN Rx checksum hardware offload and verify packet
> reception."""
>
> What is this testing? Just that the checksums work with VLANs also
> set? That's fine if so I just wasn't sure initially since it looks
> like the method is checking to see if you can receive the packets and
> then if the checksums are right.
>

Yes it's just testing to ensure checksums work with VLAN packets according
to the test plan. I'm not entirely sure why they also check to make sure
they're received, but it was in the test plan so I just left it in there.

<snip>

Other than that, all the docstring errors and function names were fixed in
both patches. Thanks for the review Jeremy!

[-- Attachment #2: Type: text/html, Size: 6464 bytes --]