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

Two commits recently fixed an issue with setting the corrrect l3 and l4
flow information when conntrack reassembles packet fragments.

c98f776 datapath: Clear the L4 portion of the key for "later" fragments
2609173 datapath: Properly set L4 keys on "later" IP fragments

This test checks for regressions that might break this feature.  It
counts on the fact that when the bug is present the udp src port
will not be correct.  It will either be zero or else some other
garbage value.  So the test feeds some fragments through for
reassembly and then checks to make sure that the udp srce port
is actually the correct value of 5001.

Tested by reverting the above commits and observing that the test
then fails.

Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---

V2 - Break up long lines with dnl to help with email formatting
---
 tests/system-traffic.at | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

On Thu, Oct 03, 2019 at 10:46:09AM -0700, Greg Rose wrote:
> Two commits recently fixed an issue with setting the corrrect l3 and l4
> flow information when conntrack reassembles packet fragments.
> 
> c98f776 datapath: Clear the L4 portion of the key for "later" fragments
> 2609173 datapath: Properly set L4 keys on "later" IP fragments
> 
> This test checks for regressions that might break this feature.  It
> counts on the fact that when the bug is present the udp src port
> will not be correct.  It will either be zero or else some other
> garbage value.  So the test feeds some fragments through for
> reassembly and then checks to make sure that the udp srce port
> is actually the correct value of 5001.
> 
> Tested by reverting the above commits and observing that the test
> then fails.---
> 
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>

Applied to master, thank you!

On 10/3/2019 11:07 AM, Ben Pfaff wrote:
> On Thu, Oct 03, 2019 at 10:46:09AM -0700, Greg Rose wrote:
>> Two commits recently fixed an issue with setting the corrrect l3 and l4
>> flow information when conntrack reassembles packet fragments.
>>
>> c98f776 datapath: Clear the L4 portion of the key for "later" fragments
>> 2609173 datapath: Properly set L4 keys on "later" IP fragments
>>
>> This test checks for regressions that might break this feature.  It
>> counts on the fact that when the bug is present the udp src port
>> will not be correct.  It will either be zero or else some other
>> garbage value.  So the test feeds some fragments through for
>> reassembly and then checks to make sure that the udp srce port
>> is actually the correct value of 5001.
>>
>> Tested by reverting the above commits and observing that the test
>> then fails.---
>>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> Applied to master, thank you!

Awesome, thanks Ben!

I had a lot of fun writing that little test - figuring out the correct 
placement of those '[]' brackets caused some
consternation for me to say the least.  But it gave me a chance to read 
up on the autom4te documentation.

Fun stuff!

;^)

- Greg

On Thu, Oct 03, 2019 at 11:09:35AM -0700, Gregory Rose wrote:
> I had a lot of fun writing that little test - figuring out the correct
> placement of those '[]' brackets caused some
> consternation for me to say the least.  But it gave me a chance to
> read up on the autom4te documentation.

m4 and Autom4te and Autotest are a pain.  They work just well enough to
not replace them.  Thanks for tolerating them.

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.

Darrell


On Thu, Oct 3, 2019 at 10:46 AM Greg Rose <gvrose8192@gmail.com> wrote:

> Two commits recently fixed an issue with setting the corrrect l3 and l4
> flow information when conntrack reassembles packet fragments.
>
> c98f776 datapath: Clear the L4 portion of the key for "later" fragments
> 2609173 datapath: Properly set L4 keys on "later" IP fragments
>
> This test checks for regressions that might break this feature.  It
> counts on the fact that when the bug is present the udp src port
> will not be correct.  It will either be zero or else some other
> garbage value.  So the test feeds some fragments through for
> reassembly and then checks to make sure that the udp srce port
> is actually the correct value of 5001.
>
> Tested by reverting the above commits and observing that the test
> then fails.
>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> ---
>
> V2 - Break up long lines with dnl to help with email formatting
> ---
>  tests/system-traffic.at | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index bfc6bb5..9afd818 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -3245,6 +3245,32 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack |
> FORMAT_CT(10.1.1.2)], [0], [dnl
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([conntrack - fragment reassembly test])
> +CHECK_CONNTRACK()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +AT_DATA([flows.txt], [dnl
> +action=normal
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +
> +AT_CHECK([ovs-ofctl packet-out br0
> "packet=52540003287c525400444ab586dd6006f70605b02c4020010001000000000000000000000020200100010000000000000000000000101100000134e88deb13891389080803136161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
>
> +"16161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161"dnl
>
> +"61616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
> +"1616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161,
> actions=ct(table=1)"])
> +
> +AT_CHECK([ovs-ofctl packet-out br0
> "packet=52540003287c525400444ab586dd6006f70602682c402001000100000000000000000000002020010001000000000000000000000010110005a834e88deb6161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
> +"161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161,
> actions=ct(table=1)"])
> +
> +AT_CHECK([ovs-ofctl packet-out br0
> "packet=52540003287c525400444ab586dd6006f706033d1140200100010000000000000000000000202001000100000000000000000000001013891389033d923861616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
> +"1616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161610a,
> actions=ct(table=1)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | head -2 | tail -1 | grep -q -e
> ["]udp[(]src=5001["]])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_BANNER([conntrack - L7])
>
>  AT_SETUP([conntrack - IPv4 HTTP])
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Thu, Oct 3, 2019 at 6:56 PM Darrell Ball <dlu998@gmail.com> 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
>

'3' seems ok upon recalculation

So, the issues are '1' and '2'.



> Alternatively, if you want to use this simplified approach, you can
> disable the test for the userspace datapath.
>
> Darrell
>
>
> On Thu, Oct 3, 2019 at 10:46 AM Greg Rose <gvrose8192@gmail.com> wrote:
>
>> Two commits recently fixed an issue with setting the corrrect l3 and l4
>> flow information when conntrack reassembles packet fragments.
>>
>> c98f776 datapath: Clear the L4 portion of the key for "later" fragments
>> 2609173 datapath: Properly set L4 keys on "later" IP fragments
>>
>> This test checks for regressions that might break this feature.  It
>> counts on the fact that when the bug is present the udp src port
>> will not be correct.  It will either be zero or else some other
>> garbage value.  So the test feeds some fragments through for
>> reassembly and then checks to make sure that the udp srce port
>> is actually the correct value of 5001.
>>
>> Tested by reverting the above commits and observing that the test
>> then fails.
>>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>> ---
>>
>> V2 - Break up long lines with dnl to help with email formatting
>> ---
>>  tests/system-traffic.at | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index bfc6bb5..9afd818 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -3245,6 +3245,32 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack |
>> FORMAT_CT(10.1.1.2)], [0], [dnl
>>  OVS_TRAFFIC_VSWITCHD_STOP
>>  AT_CLEANUP
>>
>> +AT_SETUP([conntrack - fragment reassembly test])
>> +CHECK_CONNTRACK()
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +
>> +AT_DATA([flows.txt], [dnl
>> +action=normal
>> +])
>> +
>> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
>> +
>> +AT_CHECK([ovs-ofctl packet-out br0
>> "packet=52540003287c525400444ab586dd6006f70605b02c4020010001000000000000000000000020200100010000000000000000000000101100000134e88deb13891389080803136161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
>>
>> +"16161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161"dnl
>>
>> +"61616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
>> +"1616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161,
>> actions=ct(table=1)"])
>> +
>> +AT_CHECK([ovs-ofctl packet-out br0
>> "packet=52540003287c525400444ab586dd6006f70602682c402001000100000000000000000000002020010001000000000000000000000010110005a834e88deb6161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
>> +"161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161,
>> actions=ct(table=1)"])
>> +
>> +AT_CHECK([ovs-ofctl packet-out br0
>> "packet=52540003287c525400444ab586dd6006f706033d1140200100010000000000000000000000202001000100000000000000000000001013891389033d923861616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616"dnl
>> +"1616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161610a,
>> actions=ct(table=1)"])
>> +
>> +AT_CHECK([ovs-appctl dpctl/dump-flows | head -2 | tail -1 | grep -q -e
>> ["]udp[(]src=5001["]])
>> +
>> +OVS_TRAFFIC_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>>  AT_BANNER([conntrack - L7])
>>
>>  AT_SETUP([conntrack - IPv4 HTTP])
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>

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.  It should be disabled.

How do you do that?

- Greg

On Fri, Oct 4, 2019 at 7:05 AM Gregory Rose <gvrose8192@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


>
> - Greg
>
>
>

On 10/4/2019 7:44 AM, Darrell Ball wrote:
>
>
> On Fri, Oct 4, 2019 at 7:05 AM Gregory Rose <gvrose8192@gmail.com 
> <mailto:gvrose8192@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

On 10/4/2019 7:53 AM, Gregory Rose wrote:
>
> On 10/4/2019 7:44 AM, Darrell Ball wrote:
>>
>>
>>
>> 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
>
Darrell,

I've sent a patch out to fix this up.

Thanks,

- Greg