On 04/27/2011 06:33 PM, Laine Stump wrote:
> 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<minovotn@xxxxxxxxxx>
>> ---
>> 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?
>
I was having issues with the --txt-record set with the quotes. There are
no quotes as you can see. And yes, unfortunately adding quoting does
hurt according to my testing :(
Michal
--
Michal Novotny <minovotn@xxxxxxxxxx>, RHCE
Virtualization Team (xen userspace), Red Hat
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list