[ovs-dev] [PATCH v2] tests: Add check for correct l3l4 conntrack frag reassembly

Gregory Rose gvrose8192 at gmail.com
Fri Oct 4 14:53:01 UTC 2019


On 10/4/2019 7:44 AM, Darrell Ball wrote:
>
>
> On Fri, Oct 4, 2019 at 7:05 AM Gregory Rose <gvrose8192 at gmail.com 
> <mailto:gvrose8192 at gmail.com>> wrote:
>
>
>     On 10/3/2019 6:56 PM, Darrell Ball wrote:
>     > Thanks for the patch
>     >
>     > This approach will not work for the userspace datapath
>     >
>     > Few issues off the top of my head:
>     >
>     > 1/ packet-out frees the packet (which is a fragment in this case)
>     > after completion
>     >    hence multiple packet-outs need to be part of a single Openflow
>     > bundle command as in other similar tests
>     >    This test involves 2 fragments for the first 2 packets.
>     >
>     > 2/ Userpsace datapath checks UDP checksums; for V6 packets, they
>     need
>     > to always be correct and they are not presently
>     >
>     > 3/ UDP header lengths cannot be larger than the memory allocated
>     for
>     > the packet, else sanity checking will filter
>     >     out the packet
>     >
>     > Alternatively, if you want to use this simplified approach, you can
>     > disable the test for the userspace datapath.
>
>     There's no reason that I know of to run this test in the userspace
>     datapath. 
>
>
> yep
>
>     It should be disabled. 
>
>
>     How do you do that?
>
>
> You need to add a macro to check applicability
> for kernel, it will do nothing for check-kmod case; for full 
> check-kernel support, 'if desired', you need to check for versions 
> that have the fix
> for userspace, it will unconditionally skip the test, although a 
> comment explaining why would be helpful
> see the example for CHECK_CT_DPIF_PER_ZONE_LIMIT which handles 
> check-kmod vs check-system-userspace
>
> I noticed a couple other issues:
>
> The test is labeled "fragment reassembly test":
>
> a/ All the conntrack fragmentation tests include a reassembly aspect 
> so a better
> name describing the special purpose of the test might be helpful along 
> with a
> comment explaining the special purpose for the test.
>
> b/ The "test" part of the name is redundant

Haha - OK

The patch is already applied - I'll send a follow-up patch including 
your suggestions.

Thanks,

- Greg



More information about the dev mailing list