On 8/10/26 7:20 PM, Mike Pattrick wrote: > On Mon, Aug 10, 2026 at 6:25 AM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: > > On 8/9/26 3:03 PM, Eli Britstein wrote: > > RFC 791 permits zero-payload last fragments, but common stacks > > including the Linux kernel discard them. Align the reassembly test > > with that policy: use valid one-byte fragments, and update expectations > > for duplicate handling (recirc flow and expiry coverage count). > > > > Assisted-by: composer-2.5-fast, Cursor > > Signed-off-by: Eli Britstein <[email protected] > <mailto:[email protected]>> > > --- > > v2-v1: > > - Fixed non robust allocated recirc_id. > > > > tests/ofproto-dpif.at <http://ofproto-dpif.at> | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/tests/ofproto-dpif.at <http://ofproto-dpif.at> > b/tests/ofproto-dpif.at <http://ofproto-dpif.at> > > index ee6ac873d..644b7b7fb 100644 > > --- a/tests/ofproto-dpif.at <http://ofproto-dpif.at> > > +++ b/tests/ofproto-dpif.at <http://ofproto-dpif.at> > > @@ -5536,23 +5536,24 @@ table=2 tcp actions=3 > > AT_CHECK([ovs-ofctl -O OpenFlow11 replace-flows br0 flows.txt]) > > > > dnl Test frag expiry. > > -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"]) > > +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0015 0001 8192 40 06 3315 ac11370d ac11370b 00"]) > > ovs-appctl time/warp 10000 > > -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"]) > > +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0015 0001 8192 40 06 3315 ac11370d ac11370b 00"]) > > > > dnl Test that no packets flow. > > So, this is not true anymore, the second packet is now delivered to > the destination, hence the extra datapath flow below. It seems to > be not an intended behavior of this test. What is also not clear to > me is that the test seems to assume that expiration timeout is 10 > seconds, not 15, which may be the root of the issue here. By the > time the second packet arrives the previous one is not expired yet, > though the comments like "Test frag expiry" and "Expire second frag" > suggest that "time/warp 10000" supposed to expire the fragment. > > Should we change the warp instead of the expected flows? > > > This was from a while ago and I don't have any notes on my intention for this > test. > But the comment looks incorrect as you point out. From the functionality of > the test > it looks like it's testing that an overlapping fragment gets rejected from > ipf and > sent, with the time warp clearing any dp flows first.
I'm not sure about that. If the intention was to check that the packet is sent, we would have the second datapath flow here from the start, as we're not waiting for the flows to expire after the second 'receive'. > > -M > > > > CC: Mike. > > > -AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) | sed > s'/recirc(.*)/recirc(X)/'], [0], [dnl > > +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) | sed > 's/recirc[[^)]]*)/recirc()/g' | sort], [0], [dnl > > nit: Should probably use strip_recirc helper here. And we should keep > the same pattern for all other calls within the test, so it is consistent. > > > flow-dump from the main thread: > > > -recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth(src=00:26:b9:8c:b0:f9),eth_type(0x0800),ipv4(src=172.17.55.13/128.0.0.0,proto=6,frag=later > <http://172.17.55.13/128.0.0.0,proto=6,frag=later>), packets:0, bytes:0, > used:never, actions:ct(commit,zone=10),recirc(X) > > > +recirc(),in_port(90),packet_type(ns=0,id=0),eth(src=00:26:b9:8c:b0:f9),eth_type(0x0800),ipv4(src=172.17.55.13/128.0.0.0,proto=6,frag=later > <http://172.17.55.13/128.0.0.0,proto=6,frag=later>), packets:0, bytes:0, > used:never, actions:ct(commit,zone=10),recirc() > > > +recirc(),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=later), > packets:0, bytes:0, used:never, actions:2 > > ]) > > > > dnl Expire second frag. > > ovs-appctl time/warp 10000 > > > > dnl Test frag purge > > -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0014 0002 8192 40 06 3315 ac11370d ac11370b"]) > > +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0015 0002 8192 40 06 3314 ac11370d ac11370b 00"]) > > ovs-appctl time/warp 33000 > > -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0014 0003 8192 40 06 3314 ac11370d ac11370b"]) > > +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af > 0026b98cb0f9 0800 4500 0015 0003 8192 40 06 3313 ac11370d ac11370b 00"]) > > > > dnl Test that no packets flow. > > AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) | sed > s'/recirc(.*)/recirc(X)/'], [0], [dnl > > @@ -5563,8 +5564,9 @@ > recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth(src=00:26:b9:8c:b0:f9),eth_t > > dnl Purge second frag > > ovs-appctl time/warp 33000 > > > > -dnl Make sure all four packets are counted properly in the coverage. > > -AT_CHECK([ovs-appctl coverage/show | grep -c "^ipf.*total: 2"], [0], [2 > > +dnl Make sure expiry and purge counters reflect the four packets sent. > > +dnl The duplicate second fragment is counted as overlap, not a second > expiry. > > +AT_CHECK([ovs-appctl coverage/show | grep -c "^ipf.*total: 2"], [0], [1 > > ]) > > > > zero1208=$(printf '%0*d' 2416 0) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
