Commit f7afeddc added code to report to systemd an array of interface indexes for all tap devices used by a guest. Unfortunately it not only didn't add code to report the ifindexes for macvtap interfaces (interface type='direct') or the tap devices used by type='ethernet', it ended up sending "-1" as the ifindex for each macvtap or hostdev interface. This resulted in a failure to start any domain that had a macvtap or hostdev interface (or actually any type other than "network" or "bridge"). This patch modifies qemuBuildInterfaceCommandLine() to only add an entry to the array of ifindexes for appropriate types, and to do so for all appropriate types ("network", "bridge", and "direct"). --- src/qemu/qemu_command.c | 59 ++++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 5 ++--- src/qemu/qemu_hotplug.c | 6 ++--- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7853125..81f6982 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -289,8 +289,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, int *tapfd, - size_t *tapfdSize, - int *nicindex) + size_t *tapfdSize) { const char *brname; int ret = -1; @@ -337,8 +336,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virDomainAuditNetDevice(def, net, tunpath, false); goto cleanup; } - if (virNetDevGetIndex(net->ifname, nicindex) < 0) - goto cleanup; if (virDomainNetGetActualBridgeMACTableManager(net) == VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LIBVIRT) { /* libvirt is managing the FDB of the bridge this device @@ -7756,7 +7753,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, int bootindex, virNetDevVPortProfileOp vmop, bool standalone, - int *nicindex) + size_t *nnicindexes, + int **nicindexes) { int ret = -1; char *nic = NULL, *host = NULL; @@ -7770,8 +7768,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virNetDevBandwidthPtr actualBandwidth; size_t i; - *nicindex = -1; - if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) return qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps); @@ -7818,7 +7814,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (qemuNetworkIfaceConnect(def, driver, net, qemuCaps, tapfd, - &tapfdSize, nicindex) < 0) + &tapfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) @@ -7830,6 +7826,47 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, goto cleanup; } + /* For types whose implementions use a netdev on the host, add an + * entry to nicifindexes for passing on to systemd. + */ + switch ((virDomainNetType)actualType) { + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_DIRECT: + { + int nicindex; + + /* network and bridge use a tap device, and direct uses a + * macvtap device + */ + if (virNetDevGetIndex(net->ifname, &nicindex) < 0 || + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex) < 0) + goto cleanup; + break; + } + + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_LAST: + /* These types don't use a network device on the host, but + * instead use some other type of connection to the emulated + * device in the qemu process. + * + * (Note that hostdev can't be considered as "using a network + * device", because by the time it is being used, it has been + * detached from the hostside network driver so it doesn't show + * up in the list of interfaces on the host - it's just some + * PCI device.) + */ + break; + } + /* Set bandwidth or warn if requested and not supported. */ actualBandwidth = virDomainNetGetActualBandwidth(net); if (actualBandwidth) { @@ -9279,13 +9316,9 @@ qemuBuildCommandLine(virConnectPtr conn, else vlan = i; - if (VIR_EXPAND_N(*nicindexes, *nnicindexes, 1) < 0) - goto error; - if (qemuBuildInterfaceCommandLine(cmd, driver, def, net, qemuCaps, vlan, bootNet, vmop, - standalone, - &((*nicindexes)[*nnicindexes - 1])) < 0) + standalone, nnicindexes, nicindexes) < 0) goto error; last_good_net = i; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 89e8351..ee81f92 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -1,7 +1,7 @@ /* * qemu_command.h: QEMU command generation * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -208,8 +208,7 @@ int qemuNetworkIfaceConnect(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, int *tapfd, - size_t *tapfdSize, - int *nicindex) + size_t *tapfdSize) ATTRIBUTE_NONNULL(2); int qemuPhysIfaceConnect(virDomainDefPtr def, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8691c7e..7db044d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1,7 +1,7 @@ /* * qemu_hotplug.c: QEMU device hotplug management * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -846,7 +846,6 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjPrivatePtr priv = vm->privateData; char **tapfdName = NULL; int *tapfd = NULL; - int nicindex = -1; size_t tapfdSize = 0; char **vhostfdName = NULL; int *vhostfd = NULL; @@ -916,8 +915,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, goto cleanup; memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); if (qemuNetworkIfaceConnect(vm->def, driver, net, - priv->qemuCaps, tapfd, &tapfdSize, - &nicindex) < 0) + priv->qemuCaps, tapfd, &tapfdSize) < 0) goto cleanup; iface_connected = true; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) --

The patch I posted failed to pass make check for two reasons: 1) There are valid use cases when the interface object is type='ethernet' but has no ifname. Apparently if you provide an "ifup" script name for -netdev but don't specify a tap device name, qemu will create a tap device for you, and in that case of course libvirt would be unable to provide the name to systemd. 2) Even if we avoid the code to look for the ifindex when ifname is NULL (see (1), make check will *still* fail because there are tests in the suite that have type='ethernet' and still have an ifname specified, but that device of course doesn't actually exist on the test system, so attempts to call virNetDevGetIndex() will fail. The solution here is to change qemuBuildInterfaceCommandline() so that it won't even try to add anything to the nicindexes array if NULL is sent in the args, and modify the calls from test programs to do exactly that. I intend to squash this patch into the original, already acked by danpb. --- src/qemu/qemu_command.c | 13 ++++++------- src/qemu/qemu_driver.c | 6 +----- tests/qemuxml2argvtest.c | 5 +---- tests/qemuxmlnstest.c | 5 +---- 4 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 81f6982..1e63905 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7840,10 +7840,12 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,

/* network and bridge use a tap device, and direct uses a * macvtap device */ - if (virNetDevGetIndex(net->ifname, &nicindex) < 0 || - VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex) < 0) - goto cleanup; - break; + if (nicindexes && nnicindexes && net->ifname) {

+ if (virNetDevGetIndex(net->ifname, &nicindex) < 0 || + VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex) < 0) + goto cleanup; + } + break; } case VIR_DOMAIN_NET_TYPE_USER: @@ -8257,9 +8259,6 @@ qemuBuildCommandLine(virConnectPtr conn, virUUIDFormat(def->uuid, uuid); - *nnicindexes = 0; - *nicindexes = NULL; - emulator = def->emulator; if (!cfg->privileged) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bec05d4..04fa8fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6447,8 +6447,6 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, size_t i; virQEMUDriverConfigPtr cfg; virCapsPtr caps = NULL; - size_t nnicindexes = 0; - int *nicindexes = NULL; virCheckFlags(0, NULL); @@ -6634,14 +6632,12 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, &buildCommandLineCallbacks, true, qemuCheckFips(), - NULL, - &nnicindexes, &nicindexes))) + NULL, NULL, NULL))) goto cleanup; ret = virCommandToString(cmd); cleanup: - VIR_FREE(nicindexes); virObjectUnref(qemuCaps); virCommandFree(cmd); virDomainDefFree(def); diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 16f325e..7eba5c9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -263,8 +263,6 @@ static int testCompareXMLToArgvFiles(const char *xml, char *log = NULL; virCommandPtr cmd = NULL; size_t i; - size_t nnicindexes = 0; - int *nicindexes = NULL; virBitmapPtr nodeset = NULL; if (!(conn = virGetConnect())) @@ -355,7 +353,7 @@ static int testCompareXMLToArgvFiles(const char *xml, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, &testCallbacks, false, (flags & FLAG_FIPS), - nodeset, &nnicindexes, &nicindexes))) { + nodeset, NULL, NULL))) { if (!virtTestOOMActive() && (flags & FLAG_EXPECT_FAILURE)) { ret = 0; @@ -402,7 +400,6 @@ static int testCompareXMLToArgvFiles(const char *xml, ret = 0; out: - VIR_FREE(nicindexes); VIR_FREE(log); VIR_FREE(expectargv); VIR_FREE(actualargv); diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c index a068135..4220737 100644 --- a/tests/qemuxmlnstest.c +++ b/tests/qemuxmlnstest.c @@ -44,8 +44,6 @@ static int testCompareXMLToArgvFiles(const char *xml, char *log = NULL; char *emulator = NULL; virCommandPtr cmd = NULL; - size_t nnicindexes = 0; - int *nicindexes = NULL; if (!(conn = virGetConnect())) goto fail; @@ -122,7 +120,7 @@ static int testCompareXMLToArgvFiles(const char *xml, migrateFrom, migrateFd, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, &testCallbacks, false, false, NULL, - &nnicindexes, &nicindexes))) + NULL, NULL))) goto fail; if (!virtTestOOMActive()) { @@ -158,7 +156,6 @@ static int testCompareXMLToArgvFiles(const char *xml, ret = 0; fail: - VIR_FREE(nicindexes); VIR_FREE(log); VIR_FREE(emulator); VIR_FREE(expectargv);