Thanks for the review. I will try to use OVS_WAIT_UNTIL (or something similar)
On Thu, Aug 2, 2018 at 7:33 AM, Ilya Maximets <i.maxim...@samsung.com> wrote: > Hello. Thanks for the patch. > I'm not much familiar with that functionality thus someone > else should review the sanity of this patch, but I have few > comments about testing itself. See inline. > > Best regard, Ilya Maximets. > > > Added test for snoop command to check for the initial handshake messages > > when a bridge connects to a controller via 'unix' connection method. > > > > Signed-off-by: Ashish Varma <ashishvarma.ovs at gmail.com> > > --- > > tests/ovs-ofctl.at | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at > > index 06597d7..794277b 100644 > > --- a/tests/ovs-ofctl.at > > +++ b/tests/ovs-ofctl.at > > @@ -3184,3 +3184,31 @@ AT_CHECK([grep -q "ct_dpif|DBG|.*ct_flush: zone > 123" ovs-vswitchd.log]) > > > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > + > > + > > +AT_SETUP([ovs-ofctl snoop-unix-connection]) > > +OVS_VSWITCHD_START > > + > > +dnl setup controller for br0 before starting the controller > > +AT_CHECK([ovs-vsctl set-controller br0 unix:testcontroller]) > > + > > +dnl then start listening on the '.snoop' connection > > +AT_CHECK([ovs-ofctl --detach --pidfile=ovsofctl_snoop.pid snoop br0 1> > snoopbr0.txt 2>&1]) > > +on_exit 'kill `cat ovsofctl_snoop.pid`' > > +on_exit 'unlink snoopbr0.txt' > > + > > +dnl finally start the controller > > +AT_CHECK([ovs-testcontroller --detach --pidfile punix:testcontroller], > [0], [ignore]) > > Could you, please, add '-vsyslog:off' to this command? > > > +on_exit 'kill `cat ovs-testcontroller.pid`' > > +OVS_WAIT_UNTIL([test -e testcontroller]) > > + > > +dnl wait for 2 seconds for snoop to collect the messages from the bridge > > +sleep 2 > > Waiting few seconds is a bad style, IMHO. For example this could > lead to test failures in parallel testing if the thread will have > no enough time to work. > > Is it possible to replace this by something like OVS_WAIT_UNTIL ? > > > + > > +dnl check some of the initial openflow setup messages > > +AT_CHECK([egrep "OFPT_FEATURES_REQUEST" snoopbr0.txt 1> /dev/null 2>&1]) > > +AT_CHECK([egrep "OFPT_FEATURES_REPLY" snoopbr0.txt 1> /dev/null 2>&1]) > > +AT_CHECK([egrep "OFPT_SET_CONFIG" snoopbr0.txt 1> /dev/null 2>&1]) > > + > > +OVS_VSWITCHD_STOP(["/connection failed (No such file or directory)/d"]) > > +AT_CLEANUP > > -- > > 2.7.4 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
