[libvirt] [PATCH 0/2] V2 Modify generic ethernet interface so it will work when sVirt is enabled with qemu
Version 2 of https://www.redhat.com/archives/libvir-list/2011-September/msg00795.html. Instead of creating a new library for the TAP functions and refactoring code to there I have only refactored the actual TAP creation in util/bridge.c. This simplifies the patch and achieves the same goal. AUTHORS | 1 + src/libvirt_bridge.syms | 2 + src/qemu/qemu_command.c | 79 ++++++++++++++++++++++++++++++-- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 15 ++++++ src/qemu/qemu_process.c | 13 +++++ src/util/bridge.c | 116 +++++++++++++++++++++++++++++++---------------- src/util/bridge.h | 9 ++++ 8 files changed, 196 insertions(+), 43 deletions(-)
This refactors the TAP creation code out of brAddTap into a new function brCreateTap to allow it to be used on its own. I have also changed ifSetInterfaceMac to brSetInterfaceMac and exported it since it is will be needed by code outside of util/bridge.c in the next patch. AUTHORS | 1 + src/libvirt_bridge.syms | 2 + src/util/bridge.c | 116 +++++++++++++++++++++++++++++++---------------- src/util/bridge.h | 9 ++++ 4 files changed, 89 insertions(+), 39 deletions(-) --- diff --git a/AUTHORS b/AUTHORS index b3da705..391f83a 100644 --- a/AUTHORS +++ b/AUTHORS @@ -199,6 +199,7 @@ Patches have also been contributed by: Dan Horák <dan@danny.cz> Sage Weil <sage@newdream.net> David L Stevens <dlstevens@us.ibm.com> + Tyler Coumbes <coumbes@gmail.com> [....send patches to get your name here....] diff --git a/src/libvirt_bridge.syms b/src/libvirt_bridge.syms index c3773bd..669830b 100644 --- a/src/libvirt_bridge.syms +++ b/src/libvirt_bridge.syms @@ -12,10 +12,12 @@ brAddTap; brDeleteTap; brDeleteBridge; brDelInetAddress; +brCreateTap; brHasBridge; brInit; brSetEnableSTP; brSetForwardDelay; brSetInetNetmask; +brSetInterfaceMac; brSetInterfaceUp; brShutdown; diff --git a/src/util/bridge.c b/src/util/bridge.c index d63b2a0..4875f52 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -278,7 +278,7 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, # endif /** - * ifSetInterfaceMac: + * brSetInterfaceMac: * @ctl: bridge control pointer * @ifname: interface name to set MTU for * @macaddr: MAC address (VIR_MAC_BUFLEN in size) @@ -288,7 +288,7 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, * * Returns 0 in case of success or an errno code in case of failure. */ -static int ifSetInterfaceMac(brControl *ctl, const char *ifname, +int brSetInterfaceMac(brControl *ctl, const char *ifname, const unsigned char *macaddr) { struct ifreq ifr; @@ -478,32 +478,12 @@ brAddTap(brControl *ctl, bool up, int *tapfd) { - int fd; - struct ifreq ifr; - if (!ctl || !ctl->fd || !bridge || !ifname) return EINVAL; - if ((fd = open("/dev/net/tun", O_RDWR)) < 0) - return errno; - - memset(&ifr, 0, sizeof(ifr)); - - ifr.ifr_flags = IFF_TAP|IFF_NO_PI; + errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd); -# ifdef IFF_VNET_HDR - if (vnet_hdr && brProbeVnetHdr(fd)) - ifr.ifr_flags |= IFF_VNET_HDR; -# else - (void) vnet_hdr; -# endif - - if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { - errno = EINVAL; - goto error; - } - - if (ioctl(fd, TUNSETIFF, &ifr) < 0) + if(*tapfd < 0 || errno) goto error; /* We need to set the interface MAC before adding it @@ -512,32 +492,22 @@ brAddTap(brControl *ctl, * seeing the kernel allocate random MAC for the TAP * device before we set our static MAC. */ - if ((errno = ifSetInterfaceMac(ctl, ifr.ifr_name, macaddr))) + if ((errno = brSetInterfaceMac(ctl, *ifname, macaddr))) goto error; /* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. */ - if ((errno = brSetInterfaceMtu(ctl, bridge, ifr.ifr_name))) - goto error; - if ((errno = brAddInterface(ctl, bridge, ifr.ifr_name))) + if ((errno = brSetInterfaceMtu(ctl, bridge, *ifname))) goto error; - if (up && ((errno = brSetInterfaceUp(ctl, ifr.ifr_name, 1)))) + if ((errno = brAddInterface(ctl, bridge, *ifname))) goto error; - if (!tapfd && - (errno = ioctl(fd, TUNSETPERSIST, 1))) - goto error; - VIR_FREE(*ifname); - if (!(*ifname = strdup(ifr.ifr_name))) + if (up && ((errno = brSetInterfaceUp(ctl, *ifname, 1)))) goto error; - if (tapfd) - *tapfd = fd; - else - VIR_FORCE_CLOSE(fd); return 0; error: - VIR_FORCE_CLOSE(fd); + VIR_FORCE_CLOSE(*tapfd); return errno; } @@ -803,4 +773,72 @@ cleanup: return ret; } +/** + * brCreateTap: + * @ctl: bridge control pointer + * @ifname: the interface name + * @vnet_hr: whether to try enabling IFF_VNET_HDR + * @tapfd: file descriptor return value for the new tap device + * + * Creates a tap interface. + * If the @tapfd parameter is supplied, the open tap device file + * descriptor will be returned, otherwise the TAP device will be made + * persistent and closed. The caller must use brDeleteTap to remove + * a persistent TAP devices when it is no longer needed. + * + * Returns 0 in case of success or an errno code in case of failure. + */ + +int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED, + char **ifname, + int vnet_hdr, + int *tapfd) +{ + + int fd; + struct ifreq ifr; + + if (!ifname) + return EINVAL; + + if ((fd = open("/dev/net/tun", O_RDWR)) < 0) + return errno; + + memset(&ifr, 0, sizeof(ifr)); + + ifr.ifr_flags = IFF_TAP|IFF_NO_PI; + +# ifdef IFF_VNET_HDR + if (vnet_hdr && brProbeVnetHdr(fd)) + ifr.ifr_flags |= IFF_VNET_HDR; +# else + (void) vnet_hdr; +# endif + + if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { + errno = EINVAL; + goto error; + } + + if (ioctl(fd, TUNSETIFF, &ifr) < 0) + goto error; + + if (!tapfd && + (errno = ioctl(fd, TUNSETPERSIST, 1))) + goto error; + VIR_FREE(*ifname); + if (!(*ifname = strdup(ifr.ifr_name))) + goto error; + if(tapfd) + *tapfd = fd; + else + VIR_FORCE_CLOSE(fd); + return 0; + + error: + VIR_FORCE_CLOSE(fd); + + return errno; +} + #endif /* WITH_BRIDGE */ diff --git a/src/util/bridge.h b/src/util/bridge.h index 93f0b33..c462a08 100644 --- a/src/util/bridge.h +++ b/src/util/bridge.h @@ -106,6 +106,15 @@ int brGetEnableSTP (brControl *ctl, const char *bridge, int *enable); +int brCreateTap (brControl *ctl, + char **ifname, + int vnet_hdr, + int *tapfd); + +int brSetInterfaceMac (brControl *ctl, + const char *ifname, + const unsigned char *macaddr); + # endif /* WITH_BRIDGE */ #endif /* __QEMUD_BRIDGE_H__ */
Long subject line; I trimmed it: bridge: modify for use when sVirt is enabled with qemu Use 'git shortlog -30' to get a feel for typical subjects. On 10/24/2011 05:43 PM, Tyler Coumbes wrote:
This refactors the TAP creation code out of brAddTap into a new function brCreateTap to allow it to be used on its own. I have also changed ifSetInterfaceMac to brSetInterfaceMac and exported it since it is will be needed by code outside of util/bridge.c in the next patch.
AUTHORS | 1 + src/libvirt_bridge.syms | 2 + src/util/bridge.c | 116 +++++++++++++++++++++++++++++++---------------- src/util/bridge.h | 9 ++++ 4 files changed, 89 insertions(+), 39 deletions(-) +++ b/src/libvirt_bridge.syms @@ -12,10 +12,12 @@ brAddTap; brDeleteTap; brDeleteBridge; brDelInetAddress; +brCreateTap;
Not sorted, but easy to fix.
-static int ifSetInterfaceMac(brControl *ctl, const char *ifname, +int brSetInterfaceMac(brControl *ctl, const char *ifname, const unsigned char *macaddr)
I touched up the indentation here to match.
{ struct ifreq ifr; @@ -478,32 +478,12 @@ brAddTap(brControl *ctl, bool up, int *tapfd) { - int fd; - struct ifreq ifr; - if (!ctl || !ctl->fd || !bridge || !ifname) return EINVAL;
- if ((fd = open("/dev/net/tun", O_RDWR))< 0) - return errno; - - memset(&ifr, 0, sizeof(ifr)); - - ifr.ifr_flags = IFF_TAP|IFF_NO_PI; + errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd);
Generally, we prefer returning -1 on failure, rather than a positive errno value; but this is pre-existing.
-# ifdef IFF_VNET_HDR - if (vnet_hdr&& brProbeVnetHdr(fd)) - ifr.ifr_flags |= IFF_VNET_HDR; -# else - (void) vnet_hdr; -# endif - - if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { - errno = EINVAL; - goto error; - } - - if (ioctl(fd, TUNSETIFF,&ifr)< 0) + if(*tapfd< 0 || errno)
space after if.
}
+/** + * brCreateTap: + * @ctl: bridge control pointer + * @ifname: the interface name + * @vnet_hr: whether to try enabling IFF_VNET_HDR + * @tapfd: file descriptor return value for the new tap device + * + * Creates a tap interface. + * If the @tapfd parameter is supplied, the open tap device file + * descriptor will be returned, otherwise the TAP device will be made + * persistent and closed. The caller must use brDeleteTap to remove + * a persistent TAP devices when it is no longer needed. + * + * Returns 0 in case of success or an errno code in case of failure.
So your choice of convention isn't typical, but fits the existing code.
+ */ + +int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED,
No space after function names.
+ char **ifname, + int vnet_hdr, + int *tapfd) +{ + + int fd;
Extra blank line.
+ struct ifreq ifr; + + if (!ifname) + return EINVAL; + + if ((fd = open("/dev/net/tun", O_RDWR))< 0) + return errno;
Inconsistent indentation.
+ + memset(&ifr, 0, sizeof(ifr)); + + ifr.ifr_flags = IFF_TAP|IFF_NO_PI; + +# ifdef IFF_VNET_HDR + if (vnet_hdr&& brProbeVnetHdr(fd)) + ifr.ifr_flags |= IFF_VNET_HDR; +# else + (void) vnet_hdr;
I don't like the cast to void; marking the parameter ATTRIBUTE_UNUSED is sufficient. But those are minor. So: ACK. I squashed this in and pushed. diff --git i/src/libvirt_bridge.syms w/src/libvirt_bridge.syms index 669830b..626f6ee 100644 --- i/src/libvirt_bridge.syms +++ w/src/libvirt_bridge.syms @@ -9,10 +9,10 @@ brAddBridge; brAddInetAddress; brAddInterface; brAddTap; -brDeleteTap; -brDeleteBridge; -brDelInetAddress; brCreateTap; +brDelInetAddress; +brDeleteBridge; +brDeleteTap; brHasBridge; brInit; brSetEnableSTP; diff --git i/src/util/bridge.c w/src/util/bridge.c index 4875f52..952f0f3 100644 --- i/src/util/bridge.c +++ w/src/util/bridge.c @@ -288,8 +288,9 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, * * Returns 0 in case of success or an errno code in case of failure. */ -int brSetInterfaceMac(brControl *ctl, const char *ifname, - const unsigned char *macaddr) +int +brSetInterfaceMac(brControl *ctl, const char *ifname, + const unsigned char *macaddr) { struct ifreq ifr; @@ -483,7 +484,7 @@ brAddTap(brControl *ctl, errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd); - if(*tapfd < 0 || errno) + if (*tapfd < 0 || errno) goto error; /* We need to set the interface MAC before adding it @@ -789,12 +790,12 @@ cleanup: * Returns 0 in case of success or an errno code in case of failure. */ -int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED, - char **ifname, - int vnet_hdr, - int *tapfd) +int +brCreateTap(brControl *ctl ATTRIBUTE_UNUSED, + char **ifname, + int vnet_hdr ATTRIBUTE_UNUSED, + int *tapfd) { - int fd; struct ifreq ifr; @@ -802,7 +803,7 @@ int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED, return EINVAL; if ((fd = open("/dev/net/tun", O_RDWR)) < 0) - return errno; + return errno; memset(&ifr, 0, sizeof(ifr)); @@ -811,8 +812,6 @@ int brCreateTap (brControl *ctl ATTRIBUTE_UNUSED, # ifdef IFF_VNET_HDR if (vnet_hdr && brProbeVnetHdr(fd)) ifr.ifr_flags |= IFF_VNET_HDR; -# else - (void) vnet_hdr; # endif if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch makes the changes to the generic ethernet interface for QEMU. Allowing it to be used with sVirt enabled. src/qemu/qemu_command.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 15 +++++++++ src/qemu/qemu_process.c | 13 ++++++++ 4 files changed, 107 insertions(+), 4 deletions(-) --- diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 30c0be6..b00920b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -404,6 +404,62 @@ qemuOpenVhostNet(virDomainDefPtr def, } +int qemuEthernetIfaceCreate(virDomainDefPtr def, + virDomainNetDefPtr net, + virBitmapPtr qemuCaps) +{ + int err; + int tapfd = 0; + int vnet_hdr = 0; + brControl *brctl = NULL; + unsigned char tapmac[VIR_MAC_BUFLEN]; + + if (qemuCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) && + net->model && STREQ(net->model, "virtio")) + vnet_hdr = 1; + + if (!net->ifname || + STRPREFIX(net->ifname, "vnet") || + strchr(net->ifname, '%')) { + VIR_FREE(net->ifname); + if (!(net->ifname = strdup("vnet%d"))) { + virReportOOMError(); + goto error; + } + } + + if(brInit(&brctl) < 0) + goto error; + + err = brCreateTap(brctl, &net->ifname, vnet_hdr, &tapfd); + virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); + if (tapfd < 0 || err) + goto error; + + memcpy(tapmac, net->mac, VIR_MAC_BUFLEN); + /* Discourage bridge from using TAP dev MAC */ + tapmac[0] = 0xFE; + err = brSetInterfaceMac(brctl, net->ifname, tapmac); + + if (err) + goto error; + + err = brSetInterfaceUp(brctl, net->ifname, 1); + + if (err) + goto error; + + brShutdown(brctl); + + return tapfd; + + error: + brShutdown(brctl); + VIR_FORCE_CLOSE(tapfd); + return -1; +} + + static int qemuDomainDeviceAliasIndex(virDomainDeviceInfoPtr info, const char *prefix) { @@ -2154,14 +2210,17 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, case VIR_DOMAIN_NET_TYPE_ETHERNET: virBufferAddLit(&buf, "tap"); - if (net->ifname) { - virBufferAsprintf(&buf, "%cifname=%s", type_sep, net->ifname); - type_sep = ','; - } if (net->data.ethernet.script) { + if (net->ifname) { + virBufferAsprintf(&buf, "%cifname=%s", type_sep, net->ifname); + type_sep = ','; + } virBufferAsprintf(&buf, "%cscript=%s", type_sep, net->data.ethernet.script); type_sep = ','; + } else if(net->ifname) { + virBufferAsprintf(&buf, "%cfd=%s", type_sep, tapfd); + type_sep = ','; } is_tap = true; break; @@ -4190,6 +4249,18 @@ qemuBuildCommandLine(virConnectPtr conn, if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) goto no_memory; + } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET && + !net->data.ethernet.script) { + int tapfd = qemuEthernetIfaceCreate(def, net, qemuCaps); + if (tapfd < 0) + goto error; + + last_good_net = i; + virCommandTransferFD(cmd, tapfd); + + if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", + tapfd) >= sizeof(tapfd_name)) + goto no_memory; } if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 00e58a2..b21eeb6 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -142,6 +142,10 @@ int qemuOpenVhostNet(virDomainDefPtr def, virBitmapPtr qemuCaps, int *vhostfd); +int qemuEthernetIfaceCreate(virDomainDefPtr def, + virDomainNetDefPtr net, + virBitmapPtr qemuCaps); + int qemudCanonicalizeMachine(struct qemud_driver *driver, virDomainDefPtr def); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 037f4aa..715efb2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -680,6 +680,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, iface_connected = true; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) goto cleanup; + } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + !net->data.ethernet.script) + { + if ((tapfd = qemuEthernetIfaceCreate(vm->def, net, priv->qemuCaps)) < 0) + goto cleanup; + iface_connected = true; } if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) @@ -1820,6 +1826,7 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; int vlan; char *hostnet_name = NULL; + brControl *brctl = NULL; for (i = 0 ; i < vm->def->nnets ; i++) { virDomainNetDefPtr net = vm->def->nets[i]; @@ -1916,6 +1923,14 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, } #endif + if(detach->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + !detach->data.ethernet.script) { + if(brInit(&brctl) < 0) + goto cleanup; + brDeleteTap(brctl, detach->ifname); + brShutdown(brctl); + } + if ((driver->macFilter) && (detach->ifname != NULL)) { if ((errno = networkDisallowMacOnPort(driver, detach->ifname, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a7fe86c..b8ebf36 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3300,6 +3300,7 @@ void qemuProcessStop(struct qemud_driver *driver, int logfile = -1; char *timestamp; char ebuf[1024]; + brControl *brctl = NULL; VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d", vm->def->name, vm->pid, migrated); @@ -3421,6 +3422,18 @@ void qemuProcessStop(struct qemud_driver *driver, networkReleaseActualDevice(net); } + def = vm->def; + for(i=0; i < def->nnets; i++) { + virDomainNetDefPtr net = def->nets[i]; + if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + !net->data.ethernet.script) { + if(brInit(&brctl) > 0) { + brDeleteTap(brctl, net->ifname); + brShutdown(brctl); + } + } + } + retry: if ((ret = qemuRemoveCgroup(driver, vm, 0)) < 0) { if (ret == -EBUSY && (retries++ < 5)) {
On 10/24/2011 05:44 PM, Tyler Coumbes wrote:
This patch makes the changes to the generic ethernet interface for QEMU. Allowing it to be used with sVirt enabled.
src/qemu/qemu_command.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 15 +++++++++ src/qemu/qemu_process.c | 13 ++++++++ 4 files changed, 107 insertions(+), 4 deletions(-)
I haven't reviewed this one closely yet, but it would help if you added a test in tests/qemuxml2argvtest.c and a new pair of files in tests/qemuxml2argvdata/* that exercise the code to prove the new command line options look sane. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Oct 27, 2011 at 10:25 PM, Eric Blake <eblake@redhat.com> wrote:
On 10/24/2011 05:44 PM, Tyler Coumbes wrote:
This patch makes the changes to the generic ethernet interface for QEMU. Allowing it to be used with sVirt enabled.
src/qemu/qemu_command.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 15 +++++++++ src/qemu/qemu_process.c | 13 ++++++++ 4 files changed, 107 insertions(+), 4 deletions(-)
I haven't reviewed this one closely yet, but it would help if you added a test in tests/qemuxml2argvtest.c and a new pair of files in tests/qemuxml2argvdata/* that exercise the code to prove the new command line options look sane.
Two questions about creating a test for this code. How do I get the actual args so I can compare them to my expected args to see what isn't matching up? VIR_TEST_DEBUG=2 shows me what tests fail but not any detail about why. I think it is because one of the parameters to qemu is a file descriptor which could change for each run. Is there some type of wildcard I can use like fd=%. I don't see any other tests using file descriptors to compare to and am not finding anything in any documentation I can find. Thanks, Tyler
On 10/28/2011 08:59 PM, Tyler Coumbes wrote:
On Thu, Oct 27, 2011 at 10:25 PM, Eric Blake<eblake@redhat.com> wrote:
On 10/24/2011 05:44 PM, Tyler Coumbes wrote:
This patch makes the changes to the generic ethernet interface for QEMU. Allowing it to be used with sVirt enabled.
src/qemu/qemu_command.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 15 +++++++++ src/qemu/qemu_process.c | 13 ++++++++ 4 files changed, 107 insertions(+), 4 deletions(-)
I haven't reviewed this one closely yet, but it would help if you added a test in tests/qemuxml2argvtest.c and a new pair of files in tests/qemuxml2argvdata/* that exercise the code to prove the new command line options look sane.
Two questions about creating a test for this code. How do I get the actual args so I can compare them to my expected args to see what isn't matching up? VIR_TEST_DEBUG=2 shows me what tests fail but not any detail about why.
VIR_TEST_DEBUG=1 shows a diff between expected and actual text; I'm not sure why VIR_TEST_DEBUG=2 wouldn't do the same, without diving into the test code more.
I think it is because one of the parameters to qemu is a file descriptor which could change for each run. Is there some type of wildcard I can use like fd=%. I don't see any other tests using file descriptors to compare to and am not finding anything in any documentation I can find.
tests/qemuxml2argvdata/qemuxml2argv-restore-v2-fd.args is an example file that hard-codes a particular fd, and sets things up so that qemuxml2argvtest.c passes that hard-coded fd through DO_TEST_FULL, which in turn passes it through the migrateFd argument of qemuBuildCommandLine. This may mean that we need to refactor things slightly to make it easier for testing. One idea is to have normal calls to qemuBuildCommandLine call through a function pointer that opens a TAP and returns that FD; whereas running it from the testsuite sets that function pointer to provide a replacement function that just returns a hard-coded number rather than opening an fd. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Oct 31, 2011 at 11:37 AM, Eric Blake <eblake@redhat.com> wrote:
On 10/28/2011 08:59 PM, Tyler Coumbes wrote:
On Thu, Oct 27, 2011 at 10:25 PM, Eric Blake<eblake@redhat.com> wrote:
On 10/24/2011 05:44 PM, Tyler Coumbes wrote:
This patch makes the changes to the generic ethernet interface for QEMU. Allowing it to be used with sVirt enabled.
src/qemu/qemu_command.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 15 +++++++++ src/qemu/qemu_process.c | 13 ++++++++ 4 files changed, 107 insertions(+), 4 deletions(-)
I haven't reviewed this one closely yet, but it would help if you added a test in tests/qemuxml2argvtest.c and a new pair of files in tests/qemuxml2argvdata/* that exercise the code to prove the new command line options look sane.
Two questions about creating a test for this code. How do I get the actual args so I can compare them to my expected args to see what isn't matching up? VIR_TEST_DEBUG=2 shows me what tests fail but not any detail about why.
VIR_TEST_DEBUG=1 shows a diff between expected and actual text; I'm not sure why VIR_TEST_DEBUG=2 wouldn't do the same, without diving into the test code more.
I think it is because one of the parameters to qemu is a file descriptor which could change for each run. Is there some type of wildcard I can use like fd=%. I don't see any other tests using file descriptors to compare to and am not finding anything in any documentation I can find.
tests/qemuxml2argvdata/qemuxml2argv-restore-v2-fd.args is an example file that hard-codes a particular fd, and sets things up so that qemuxml2argvtest.c passes that hard-coded fd through DO_TEST_FULL, which in turn passes it through the migrateFd argument of qemuBuildCommandLine.
This may mean that we need to refactor things slightly to make it easier for testing. One idea is to have normal calls to qemuBuildCommandLine call through a function pointer that opens a TAP and returns that FD; whereas running it from the testsuite sets that function pointer to provide a replacement function that just returns a hard-coded number rather than opening an fd.
Still not having any luck. I have to be missing something simple. Below are the test xml and args files I have added in tests/qemuxml2argvdata/. They are based on the qemuxml2argv-net-eth test. qemuxml2argv-net-eth-fd.xml <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <memory>219100</memory> <currentMemory>219100</currentMemory> <vcpu>1</vcpu> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> </os> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <target dev='hda' bus='ide'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk> <controller type='ide' index='0'/> <interface type='ethernet'> <mac address='00:11:22:33:44:55'/> </interface> <memballoon model='virtio'/> </devices> </domain> qemuxml2argv-net-eth-fd.args LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net nic,\ macaddr=00:11:22:33:44:55,vlan=0 -net tap,fd=7,\ vlan=0 -serial none -parallel none -usb At first I tried adding the following to qemuxml2argvtest.c DO_TEST("net-eth-fd", false, NONE); Then after your suggestion I tried DO_TEST_FULL("net-eth-fd", "stdio", 7, false, QEMU_CAPS_MIGRATE_QEMU_FD); Both resulted in the same result. A failed test with no actual and expected command line shown. Just FAILED like below. VIR_TEST_DEBUG=1 ./qemuxml2argvtest --snip-- 83) QEMU XML-2-ARGV net-eth ... OK 84) QEMU XML-2-ARGV net-eth-fd ... FAILED 85) QEMU XML-2-ARGV net-eth-ifname ... OK --snip-- I get the same output with VIR_TEST_DEBUG=2 as well. No actual and expected output on the FAILED. Maybe something is going wrong before it even gets that far? Not sure what I am missing. Below is the actual commandline generated from a test VM running on my development server. You can see the -netdev tap,fd=27, in the command line. /usr/bin/qemu-kvm -S -M pc-0.14 -cpu qemu32 -enable-kvm -m 1024 -smp 1,sockets=1,cores=1,threads=1 -name test -uuid 7fcf83f2-9c90-af39-e1f8-bdc7cadbf9c3 -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/usr/local/var/lib/libvirt/qemu/test.monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -drive file=/vms/disks/test.img,if=none,id=drive-ide0-0-0,format=qcow2,cache=writeback -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -drive if=none,media=cdrom,id=drive-ide0-1-0,readonly=on,format=raw -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 -netdev tap,fd=27,id=hostnet0 -device rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:a9:37:40,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -usb -vnc 127.0.0.1:0 -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
participants (2)
-
Eric Blake -
Tyler Coumbes