Re: [libvirt] [PATCH v2 2/5] Network: Add regression tests for the command line arguments

On 04/01/2011 06:45 AM, Michal Novotny wrote: > Make: passed > Make check: passed > Make syntax-check: passed > > Hi, > this is the patch that is adding regression tests for the network > bridge driver command-line arguments similar way it is done for > QEMU driver. This is checking the built dnsmasq parameters (using > networkBuildDhcpDaemonCommandLine() function) and comparing them > to excepted arguments in the *.argv files. > > This has been tested and working fine. > > Michal Same comments about the commit message as in 1/5 - don't include stuff about what tests passed, salutations, signatures; *do* include a short sample of the XML. > Signed-off-by: Michal Novotny&lt;minovotn(a)redhat.com&gt; > --- > src/network/bridge_driver.c | 27 ++++- > src/network/bridge_driver.h | 3 + > tests/Makefile.am | 11 ++ > tests/networkxml2argvdata/isolated-network.argv | 1 + > tests/networkxml2argvdata/isolated-network.xml | 11 ++ > .../nat-network-dns-txt-record.argv | 1 + > .../nat-network-dns-txt-record.xml | 24 ++++ > tests/networkxml2argvdata/nat-network.argv | 1 + > tests/networkxml2argvdata/nat-network.xml | 21 ++++ > tests/networkxml2argvdata/netboot-network.argv | 1 + > tests/networkxml2argvdata/netboot-network.xml | 14 +++ > .../networkxml2argvdata/netboot-proxy-network.argv | 1 + > .../networkxml2argvdata/netboot-proxy-network.xml | 13 ++ > tests/networkxml2argvdata/routed-network.argv | 1 + > tests/networkxml2argvdata/routed-network.xml | 9 ++ > tests/networkxml2argvtest.c | 119 ++++++++++++++++++++ > 16 files changed, 255 insertions(+), 3 deletions(-) > create mode 100644 tests/networkxml2argvdata/isolated-network.argv > create mode 100644 tests/networkxml2argvdata/isolated-network.xml > create mode 100644 tests/networkxml2argvdata/nat-network-dns-txt-record.argv > create mode 100644 tests/networkxml2argvdata/nat-network-dns-txt-record.xml > create mode 100644 tests/networkxml2argvdata/nat-network.argv > create mode 100644 tests/networkxml2argvdata/nat-network.xml > create mode 100644 tests/networkxml2argvdata/netboot-network.argv > create mode 100644 tests/networkxml2argvdata/netboot-network.xml > create mode 100644 tests/networkxml2argvdata/netboot-proxy-network.argv > create mode 100644 tests/networkxml2argvdata/netboot-proxy-network.xml > create mode 100644 tests/networkxml2argvdata/routed-network.argv > create mode 100644 tests/networkxml2argvdata/routed-network.xml > create mode 100644 tests/networkxml2argvtest.c > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 2e299f5..b6ce39d 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -613,11 +613,11 @@ cleanup: > return ret; > } > > -static int > -networkStartDhcpDaemon(virNetworkObjPtr network) > +int > +networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, > + char *pidfile) If you make a function global, you should add it to a .syms file. In this case (as we just discussed on IRC with eblake) you should create a new src/libvirt_network.syms file and add it to that (then add that .syms file to the appropriate places in src/Makefile.am) > { > virCommandPtr cmd = NULL; > - char *pidfile = NULL; As patched, this will not compile - you removed pidfile from the new function, but left the assignment to it in. (actually, all of the directory and file creation items should be moved down into networkStartDhcpDaemon, so that networkBuildDhcpDaemonCommandLine doesn't have any side effects.) > int ret = -1, err, ii; > virNetworkIpDefPtr ipdef; > > @@ -666,6 +666,27 @@ networkStartDhcpDaemon(virNetworkObjPtr network) > goto cleanup; > } > > + if (cmdout) > + *cmdout = cmd; > + > + ret = 0; > +cleanup: > + if (ret != 0) The standard practice in libvirt is to use "ret < 0" rather than "ret != 0". > + virCommandFree(cmd); > + return ret; > +} > + > +static int > +networkStartDhcpDaemon(virNetworkObjPtr network) > +{ > + virCommandPtr cmd = NULL; > + char *pidfile = NULL; > + int ret = -1; > + > + ret = networkBuildDhcpDaemonCommandLine(network,&cmd, pidfile); > + if (ret != 0) Again, ret < 0. > + goto cleanup; > + > if (virCommandRun(cmd, NULL)< 0) > goto cleanup; > > diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h > index 32d2ae7..8d82b67 100644 > --- a/src/network/bridge_driver.h > +++ b/src/network/bridge_driver.h > @@ -28,7 +28,10 @@ > # include<config.h> > > # include "internal.h" > +# include "network_conf.h" > +# include "command.h" > > int networkRegister(void); > +int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile); > > #endif /* __VIR_NETWORK__DRIVER_H */ > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 5896442..a3f8d00 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -50,6 +50,7 @@ EXTRA_DIST = \ > networkschematest \ > networkxml2xmlin \ > networkxml2xmlout \ > + networkxml2argvdata \ Yay for new tests! > nodedevschemadata \ > nodedevschematest \ > nodeinfodata \ > @@ -104,6 +105,8 @@ endif > > check_PROGRAMS += networkxml2xmltest > > +check_PROGRAMS += networkxml2argvtest > + > check_PROGRAMS += nwfilterxml2xmltest > > check_PROGRAMS += storagevolxml2xmltest storagepoolxml2xmltest > @@ -191,6 +194,8 @@ endif > > TESTS += networkxml2xmltest > > +TESTS += networkxml2argvtest > + > TESTS += storagevolxml2xmltest storagepoolxml2xmltest > > TESTS += nodedevxml2xmltest > @@ -308,6 +313,12 @@ networkxml2xmltest_SOURCES = \ > testutils.c testutils.h > networkxml2xmltest_LDADD = $(LDADDS) > > +networkxml2argvtest_SOURCES = \ > + networkxml2argvtest.c \ > + ../src/network/bridge_driver.c network/bridge_driver.h \ Rather than adding the source files, you should be adding the .la file libvirt_network.la. See other .la file additions for the proper pattern to follow. > + testutils.c testutils.h > +networkxml2argvtest_LDADD = $(LDADDS) > + > nwfilterxml2xmltest_SOURCES = \ > nwfilterxml2xmltest.c \ > testutils.c testutils.h > diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv > new file mode 100644 > index 0000000..1c173db > --- /dev/null > +++ b/tests/networkxml2argvdata/isolated-network.argv > @@ -0,0 +1 @@ > +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/private.pid --conf-file= --except-interface lo --dhcp-option=3 --listen-address 192.168.152.1 --dhcp-range 192.168.152.2,192.168.152.254 --dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases --dhcp-lease-max=253 --dhcp-no-override > diff --git a/tests/networkxml2argvdata/isolated-network.xml b/tests/networkxml2argvdata/isolated-network.xml > new file mode 100644 > index 0000000..cc320a9 > --- /dev/null > +++ b/tests/networkxml2argvdata/isolated-network.xml > @@ -0,0 +1,11 @@ > +<network> > +<name>private</name> > +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> > +<bridge name='virbr2' stp='on' delay='0' /> > +<mac address='52:54:00:17:3F:37'/> > +<ip address='192.168.152.1' netmask='255.255.255.0'> > +<dhcp> > +<range start='192.168.152.2' end='192.168.152.254' /> > +</dhcp> > +</ip> > +</network> > diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv > new file mode 100644 > index 0000000..55dcf02 > --- /dev/null > +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv > @@ -0,0 +1 @@ > +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/default.pid --conf-file= --except-interface lo --txt-record=example,example value --listen-address 192.168.122.1 Does the argument to ==txt-record need to be quoted? (probably not needed, but it might help readability in the logs - will it *hurt* anything to quote it?