[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com>
>> ---
>>   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 redhat com>, RHCE
Virtualization Team (xen userspace), Red Hat


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]