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]> -- Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
