[ovs-dev] [PATCH 2/3] autotest: add autotest framework for adding kernel module unit tests

[Ben Pfaff]

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 <azhou at 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

> +m4_define([OVS_KMOD_VSWITCHD_STOP],
> +  [AT_CHECK([ovs-vsctl del-br br0])

I think that $1 should be quoted below, e.g. ([$1]):

> +   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])
> +   ]
> +)

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 ])
> +    ]
> +)

kmod-testsuite.at has a lot in common with testsuite.at.  Could the
common macros be factored out into a mutually included .at?

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.