Thanks for the review, please see comments inline. On Thu, Jan 15, 2015 at 8:12 AM, Ben Pfaff <b...@nicira.com> wrote: > On Tue, Jan 13, 2015 at 05:28:06PM -0800, Andy Zhou wrote: >> This patch adds a basic infrastructure for developing and running >> kernel module unit tests. Currently OVS contains thousands >> of useful unit tests for user space programs. It is desirable to >> have corresponding kernel module unit tests. >> >> This commit adds basic framework for adding kernel module tests. Like >> user space unit tests, Kmod tests are based autotest framework, thus >> are similar to existing unit tests. For references, kmod-traffic.at >> contains a simple ping test. >> >> "make check-kmod" can be invoked on any build machine as a root >> user. Since kernel testing can potentially crash the kernel, it is >> not recommended to run those tests directly on a development machine, >> but rather a testing VM, such as ones can be launched by vagrant. >> >> Signed-off-by: Andy Zhou <az...@nicira.com> > > We've been waiting a long time for someone to write some kernel module > tests. Thanks a lot for stepping forward to add the infrastructure and > getting started on the tests. > > I have a few comments. > > I think that KMOD_TESTSUITE_AT should include the .at files from the > regular testsuite that the kmod testsuite uses, so that kmod-testsuite > gets rebuilt when those change. For example, I think that > kmod-testsuite includes ofproto-macros.at, which means that > kmod-testsuite should be rebuild when ofproto-macros.at changes, so I > think that KMOD_TESTSUITE_AT should list ofproto-macros.at > Good point, I missed them. >> +m4_define([OVS_KMOD_VSWITCHD_STOP], >> + [AT_CHECK([ovs-vsctl del-br br0]) > > I think that $1 should be quoted below, e.g. ([$1]): > Will do. >> + OVS_SWITCHD_STOP($1) >> + AT_CHECK([modprobe -r openvswitch]) >> + ]) > > I think that as written this will expand literally without any spacing, > e.g. DEL_NAMESPACES([a], [b], [c]) will become: > ip netns del aip netns del bip netns del c >> +m4_define([DEL_NAMESPACES], >> + [m4_foreach([ns], [$@], >> + [ip netns del ns]) >> + ] >> +) > Would "[ip netns del ns; ] work? > While there's no correctness problem with extra spaces inside [] here, > it doesn't match the style we've used elsewhere: >> +m4_define([ADD_VETH], >> + [ ovs-vsctl del-port $3 ovs-$1 >> + ip netns exec $2 ip link del $1 >> + AT_CHECK([ ip link add $1 type veth peer name ovs-$1 ]) >> + AT_CHECK([ ip link set $1 netns $2 ]) >> + AT_CHECK([ ovs-vsctl add-port $3 ovs-$1 ]) >> + AT_CHECK([ ip link set dev ovs-$1 up ]) >> + AT_CHECK([ ip netns exec $2 ifconfig $1 $4 netmask $5 up ]) >> + ] >> +) > Right, will fix.
> kmod-testsuite.at has a lot in common with testsuite.at. Could the > common macros be factored out into a mutually included .at? > Will do. > I find myself wondering whether we should adopt some naming convention > for namespaces created by the testsuite, such as a prefix. That would > make it safe(r) to delete all of the namespaces created by the tests. Any suggestions? How about "at_" ? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
