[libvirt] [PATCH] qemuxml2argvtest: Adapt to ethernet automatic tap creation
After 9c17d665fdc5 the tap device for ethernet network type is automatically precreated before spawning qemu. Problem is, the qemuxml2argvtest wasn't updated and thus is failing. Because of all the APIs that new code is calling, I had to mock a lot. Also, since the tap FDs are labeled separately from the rest of the devices/files I had to enable NOP security driver for the test too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- There are only 2 problems with this patch. All of them are in virNetDevTapCreate mock implementation: 1) new tap device has constant name. Even within one domain 2) new tap FDs are constant. Even within one domain I'm unable to come with better approach though. Having a static variable that is incremented each time the mock is called would not fly as it will give different results when combined with VIR_TEST_RANGE. Therefore I assume we are good so far with these two limitations. cfg.mk | 2 +- tests/qemuhotplugtest.c | 7 ---- .../qemuxml2argv-graphics-spice-timeout.args | 2 +- .../qemuxml2argv-net-eth-ifname.args | 2 +- .../qemuxml2argv-net-eth-names.args | 4 +- tests/qemuxml2argvdata/qemuxml2argv-net-eth.args | 2 +- tests/qemuxml2argvmock.c | 49 ++++++++++++++++++++-- tests/testutilsqemu.c | 9 ++++ 8 files changed, 61 insertions(+), 16 deletions(-) diff --git a/cfg.mk b/cfg.mk index 6f28eef..f5573db 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1139,7 +1139,7 @@ exclude_file_name_regexp--sc_copyright_usage = \ ^COPYING(|\.LESSER)$$ exclude_file_name_regexp--sc_flags_usage = \ - ^(docs/|src/util/virnetdevtap\.c$$|tests/(vir(cgroup|pci|usb)|nss)mock\.c$$) + ^(docs/|src/util/virnetdevtap\.c$$|tests/(vir(cgroup|pci|usb)|nss|qemuxml2argv)mock\.c$$) exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^(src/rpc/gendispatch\.pl$$|tests/) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 2298a68..1eb2b6a 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -341,7 +341,6 @@ mymain(void) { int ret = 0; struct qemuHotplugTestData data = {0}; - virSecurityManagerPtr mgr; #if !WITH_YAJL fputs("libvirt not compiled with yajl, skipping this test\n", stderr); @@ -369,12 +368,6 @@ mymain(void) if (!driver.lockManager) return EXIT_FAILURE; - if (!(mgr = virSecurityManagerNew("none", "qemu", - VIR_SECURITY_MANAGER_PRIVILEGED))) - return EXIT_FAILURE; - if (!(driver.securityManager = virSecurityManagerNewStack(mgr))) - return EXIT_FAILURE; - /* wait only 100ms for DEVICE_DELETED event */ qemuDomainRemoveDeviceWaitTime = 100; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args index 7ca17ae..8a29a7e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args @@ -26,7 +26,7 @@ id=virtio-disk0 \ media=cdrom,id=drive-ide0-1-0 \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ -device rtl8139,vlan=0,id=net0,mac=52:54:00:71:70:89,bus=pci.0,addr=0x7 \ --net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 \ +-net tap,fd=3,vlan=0,name=hostnet0 \ -serial pty \ -device usb-tablet,id=input0 \ -spice port=5900 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args index 22d6dd0..b96c933 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args @@ -20,4 +20,4 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -device rtl8139,vlan=0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x3 \ --net tap,ifname=nic02,script=/etc/qemu-ifup,vlan=0,name=hostnet0 +-net tap,fd=3,vlan=0,name=hostnet0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args index 0704178..a2c3f87 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args @@ -20,7 +20,7 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -device rtl8139,vlan=0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x3 \ --net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 \ +-net tap,fd=3,vlan=0,name=hostnet0 \ -device e1000,vlan=1,id=net1,mac=00:11:22:33:44:56,bus=pci.0,addr=0x4 \ --net tap,script=/etc/qemu-ifup,vlan=1,name=hostnet1 \ +-net tap,fd=3,vlan=1,name=hostnet1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args index b69cf52..b96c933 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args @@ -20,4 +20,4 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -device rtl8139,vlan=0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x3 \ --net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 +-net tap,fd=3,vlan=0,name=hostnet0 diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index b7dfebb..e2c19a6 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -21,12 +21,15 @@ #include <config.h> #include "internal.h" -#include "virnuma.h" +#include "vircommand.h" #include "virmock.h" -#include "virutil.h" +#include "virnetdev.h" +#include "virnetdevtap.h" +#include "virnuma.h" +#include "virscsi.h" #include "virstring.h" #include "virtpm.h" -#include "virscsi.h" +#include "virutil.h" #include <time.h> #include <unistd.h> @@ -98,3 +101,43 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, ignore_value(VIR_STRDUP(ret, "sg0")); return ret; } + +int +virNetDevTapCreate(char **ifname, + const char *tunpath ATTRIBUTE_UNUSED, + int *tapfd, + size_t tapfdSize, + unsigned int flags ATTRIBUTE_UNUSED) +{ + size_t i; + + for (i = 0; i < tapfdSize; i++) + tapfd[i] = STDERR_FILENO + 1 + i; + + return VIR_STRDUP(*ifname, "vnet0"); +} + +int +virNetDevSetMAC(const char *ifname ATTRIBUTE_UNUSED, + const virMacAddr *macaddr ATTRIBUTE_UNUSED) +{ + return 0; +} + +int +virCommandRun(virCommandPtr cmd ATTRIBUTE_UNUSED, + int *exitstatus) +{ + if (exitstatus) + *exitstatus = 0; + + return 0; +} + +void +virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + /* nada */ +} diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 1f854f5..eb4c6c8 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -555,6 +555,8 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary, int qemuTestDriverInit(virQEMUDriver *driver) { + virSecurityManagerPtr mgr = NULL; + memset(driver, 0, sizeof(*driver)); if (virMutexInit(&driver->lock) < 0) @@ -588,9 +590,16 @@ int qemuTestDriverInit(virQEMUDriver *driver) if (qemuTestCapsCacheInsert(driver->qemuCapsCache, "empty", NULL) < 0) goto error; + if (!(mgr = virSecurityManagerNew("none", "qemu", + VIR_SECURITY_MANAGER_PRIVILEGED))) + goto error; + if (!(driver->securityManager = virSecurityManagerNewStack(mgr))) + goto error; + return 0; error: + virObjectUnref(mgr); qemuTestDriverFree(driver); return -1; } -- 2.7.3
On Wed, Mar 23, 2016 at 04:35:58PM +0100, Michal Privoznik wrote:
After 9c17d665fdc5 the tap device for ethernet network type is automatically precreated before spawning qemu. Problem is, the qemuxml2argvtest wasn't updated and thus is failing. Because of all the APIs that new code is calling, I had to mock a lot. Also, since the tap FDs are labeled separately from the rest of the devices/files I had to enable NOP security driver for the test too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
There are only 2 problems with this patch. All of them are in virNetDevTapCreate mock implementation: 1) new tap device has constant name. Even within one domain 2) new tap FDs are constant. Even within one domain
I'm unable to come with better approach though. Having a static variable that is incremented each time the mock is called would not fly as it will give different results when combined with VIR_TEST_RANGE. Therefore I assume we are good so far with these two limitations.
Yeah, I think this isn't a big deal within scope of unit tests - they're not trying to validate semantic correctness, so returning fix name & fd is fine ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/23/2016 11:35 AM, Michal Privoznik wrote:
After 9c17d665fdc5 the tap device for ethernet network type is automatically precreated before spawning qemu. Problem is, the qemuxml2argvtest wasn't updated and thus is failing. Because of all the APIs that new code is calling, I had to mock a lot. Also, since the tap FDs are labeled separately from the rest of the devices/files I had to enable NOP security driver for the test too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
--- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c
+} + +int +virCommandRun(virCommandPtr cmd ATTRIBUTE_UNUSED, + int *exitstatus) +{ + if (exitstatus) + *exitstatus = 0; + + return 0; +}
The problem with mocking virCommandRun is that it is called by the test infrastructure (virTestRewrapFile(), which is used when regenerating the test results (VIR_TEST_REGENERATE_OUTPUT=1)). After this commit that function silently fails, which results in virFileWriteStr() calling strlen(NULL) and a crash of the test. (Nobody noticed this before because it's only called if you set VIR_TEST_REGENERATE_OUTPUT *and* the results of one of the qemuxml2argv tests has changed). So what's the most reasonable way to deal with this? I suppose we could rename virCommandRun to e.g. virCommandRunInternal() which would be called by a new virCommandRun(), then have virTestRewrapFile() call virCommandRunInternal() so that it wouldn't get the mocked version. That seems ugly, inefficient, and hackish, but I can't think of any way that isn't ugly, inefficient, and hackish...
On 04/12/2016 05:48 PM, Laine Stump wrote:
On 03/23/2016 11:35 AM, Michal Privoznik wrote:
After 9c17d665fdc5 the tap device for ethernet network type is automatically precreated before spawning qemu. Problem is, the qemuxml2argvtest wasn't updated and thus is failing. Because of all the APIs that new code is calling, I had to mock a lot. Also, since the tap FDs are labeled separately from the rest of the devices/files I had to enable NOP security driver for the test too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
--- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c
+} + +int +virCommandRun(virCommandPtr cmd ATTRIBUTE_UNUSED, + int *exitstatus) +{ + if (exitstatus) + *exitstatus = 0; + + return 0; +}
The problem with mocking virCommandRun is that it is called by the test infrastructure (virTestRewrapFile(), which is used when regenerating the test results (VIR_TEST_REGENERATE_OUTPUT=1)).
After this commit that function silently fails, which results in virFileWriteStr() calling strlen(NULL) and a crash of the test. (Nobody noticed this before because it's only called if you set VIR_TEST_REGENERATE_OUTPUT *and* the results of one of the qemuxml2argv tests has changed).
So what's the most reasonable way to deal with this? I suppose we could rename virCommandRun to e.g. virCommandRunInternal() which would be called by a new virCommandRun(), then have virTestRewrapFile() call virCommandRunInternal() so that it wouldn't get the mocked version. That seems ugly, inefficient, and hackish, but I can't think of any way that isn't ugly, inefficient, and hackish...
I can't really decide if mocking virCommand is good or not. On one hand it'll catch any virCommand calls in driver code that may leak into the test suite which mean host environment dependencies, on the other hand losing the ability to use virCommand in the test suite might be a problem some day. Either way we can work around this issue easily enough by just using system() to call test-wrap-argv.pl, I've sent a patch - Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Laine Stump -
Michal Privoznik