On 1/27/22 00:44, Gaëtan Rivet wrote: > On Mon, Dec 27, 2021, at 14:26, Paolo Valerio wrote: >> When testing if an established connection is picked up, it could be >> useful to verify that the protocol state matches the expectation, that >> is, it moves to ESTABLISHED, as there's a chance that code modifications >> may break the TCP conn_update() in a way that it returns CT_UPDATE_VALID >> without moving to the correct state leading to a false positive. >> >> Signed-off-by: Paolo Valerio <[email protected]> >> --- >> tests/ofproto-dpif.at | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 1660b0856..07cb1e4eb 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -10875,6 +10875,12 @@ dnl AT_CHECK([ovs-appctl netdev-dummy/receive >> p2 '50540000000a505400000009080045 >> AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> '50540000000950540000000a080045000028fc264000400628a50a0101020a01010100020001396bb35a8cadbdb45010000a629b0000']) >> AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> '50540000000950540000000a080045000029fc274000400628a30a0101020a01010100020001396bb35a8cadbdb45018000a589200000a']) >> AT_CHECK([ovs-appctl netdev-dummy/receive p2 >> '50540000000a505400000009080045000028f2c84000400632030a0101010a010102000100028cadbdb4396bb35b5010000a629a0000']) >> + >> +# dnl Check that the protocol state moved to established after the >> pickup >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack], [0], [dnl >> +tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),protoinfo=(state=ESTABLISHED) >> +]) >> + >> AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> '50540000000950540000000a08004500022afc284000400626a10a0101020a01010100020001396bb35b8cadbdb45018000a941f0000 >> >> dnl >> >> 666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666 >> >> dnl >> >> 666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666 >> >> dnl >> > > In the context of this test, and considering the current coverage, I think > this makes sense. > > I think however that if we want to edit the TCP state machine, we ought > to implement unit-tests specifically for that, to explore the full state > machine > and verify that each possible combination of input results in the expected > state. > > So if that happens, this current patch would ossify the test suite in a > secondary place. > I don't think it's a big issue, it just means that if such more advanced > testing > was done, there is potential for additional diff noise. > > This is not blocking as far as I'm concerned. So in my opinion, > > Acked-by: Gaetan Rivet <[email protected]> >
Thanks, Paolo, Gaetan and Mike! Since it's test-only change, applied. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
