There are times when it's necessary to learn the actual type of a network connection before any resources have been allocated (e.g. during qemuProcessPrepareDomain()), but in the past it was necessary to call networkAllocateActualDevice() in order to have the actual type filled in. This new function returns the type of network that *will be* setup once it actually happens, but without making any changes on the host. --- src/network/bridge_driver.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ src/network/bridge_driver.h | 6 +++- 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c673cc1..0d99991 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4736,6 +4736,78 @@ networkGetNetworkAddress(const char *netname, char **netaddr) return ret; } +/* networkGetActualType: + * @dom: domain definition that @iface belongs to + * @iface: the original NetDef from the domain + * + * Looks up the network reference by iface, and returns the actual + * type of the connection without allocating any resources. + * + * Returns 0 on success, -1 on failure. + */ +int +networkGetActualType(virDomainNetDefPtr iface) +{ + virNetworkDriverStatePtr driver = networkGetDriver(); + virNetworkObjPtr network = NULL; + virNetworkDefPtr netdef = NULL; + int ret = -1; + + if (!driver || iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) + return iface->type; + + if (iface->data.network.actual) + return iface->data.network.actual->type; + + network = virNetworkObjFindByName(driver->networks, iface->data.network.name); + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, + _("no network with matching name '%s'"), + iface->data.network.name); + return -1; + } + netdef = network->def; + + if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) || + (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) || + (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) { + /* for these forward types, the actual net type really *is* + * NETWORK; we just keep the info from the portgroup in + * iface->data.network.actual + */ + ret = VIR_DOMAIN_NET_TYPE_NETWORK; + + } else if ((netdef->forward.type == VIR_NETWORK_FORWARD_BRIDGE) && + netdef->bridge) { + + /* <forward type='bridge'/> <bridge name='xxx'/> + * is VIR_DOMAIN_NET_TYPE_BRIDGE + */ + + ret = VIR_DOMAIN_NET_TYPE_BRIDGE; + + } else if (netdef->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) { + + ret = VIR_DOMAIN_NET_TYPE_HOSTDEV; + + } else if ((netdef->forward.type == VIR_NETWORK_FORWARD_BRIDGE) || + (netdef->forward.type == VIR_NETWORK_FORWARD_PRIVATE) || + (netdef->forward.type == VIR_NETWORK_FORWARD_VEPA) || + (netdef->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH)) { + + /* <forward type='bridge|private|vepa|passthrough'> are all + * VIR_DOMAIN_NET_TYPE_DIRECT. + */ + + ret = VIR_DOMAIN_NET_TYPE_DIRECT; + + } + + virNetworkObjEndAPI(&network); + return ret; +} + + /** * networkCheckBandwidth: * @net: network QoS diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 7db2c90..f0cac5d 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -1,7 +1,7 @@ /* * bridge_driver.h: core driver methods for managing networks * - * Copyright (C) 2006-2015 Red Hat, Inc. + * Copyright (C) 2006-2016 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -47,6 +47,9 @@ int networkReleaseActualDevice(virDomainDefPtr dom, int networkGetNetworkAddress(const char *netname, char **netaddr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int networkGetActualType(virDomainNetDefPtr iface) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, char **configstr, @@ -64,6 +67,7 @@ int networkBandwidthUpdate(virDomainNetDefPtr iface, # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(dom, iface) 0 +# define networkGetActualType(iface) (iface->type) # define networkGetNetworkAddress(netname, netaddr) (-2) # define networkDnsmasqConfContents(network, pidfile, configstr, \ dctx, caps) 0 -- 2.5.5

Starting with commit f8e712fe, if you start a domain that has an <interface type='hostdev' (or that has <interface type='network'> where the network is a pool of devices for hostdev assignment), when you later try to add *another* interface (of any kind) with hotplug, the function qemuAssignDeviceNetAlias() fails as soon as it sees a "hostdevN" alias in the list of interfaces), causing the attach to fail. This is because (starting with f8e712fe) the device alias names are assigned during the new function qemuProcessPrepareDomain(), which is called *before* networkAllocateActualDevice() (which is called from qemuProcessPrepareHost(), which is called from qemuProcessLaunch()). Prior to that commit, networkAllocateActualDevice() was called first. The problem with this is that the alias for interfaces that are really a hostdev (<interface type='hostdev'>) is of the form "hostdevN" (just like other hostdevs), while other interfaces are "netN". But if you don't know that the interface is going to be a hostdev at the time you assign the alias name, you can't name it differently. (As far as I've seen so far, the change in name by itself wouldn't have been a problem (other than just an outwardly noticeable change in behavior) except for the abovementioned failure to attach/detach new interfaces. Rather than take the chance that there may be other not-yet-revealed problems associated with changing the alias name, this patch changes the way that aliases are assigned to restore the old behavior. Old: In the past, assigning an alias to an interface was skipped if it was seen that the interface was type='hostdev' - we knew that the hostdev part of the interface was also in the list of hostdevs (that's part of what happens in networkAllocateActualDevice()) and it would be assigned when all the other hostdev aliases were assigned. New: When assigning an alias to an interface, we haven't yet called networkAllocateActualDevice() to construct the hostdev part of the interface, so we can't just wait for the loop that creates aliases for all the hostdevs (there's nothing on that list for this device yet!). Instead we handle it immediately in the loop creating interface aliases, by calling the new function networkGetActualType() to determine if it is going to be hostdev, and if so calling qemuAssignDeviceHostdevAlias() instead. Some adjustments have to be made to both qemuAssignDeviceHostdevAlias() and to qemuAssignDeviceNetAlias() to accommodate this. In both of them, an error return from qemuDomainDeviceAliasIndex() is no longer considered an error; instead it's just ignored (because it almost certainly means that the alias string for the device was "net" when we expected "hostdev" or vice versa). in qemuAssignDeviceHostdevAlias() we have to look at all interface aliases for hostdevN in addition to looking at all hostdev aliases (this wasn't necessary in the past, because both the interface entry and the hostdev entry for the device already pointed at the device info; no longer the case since the hostdev entry hasn't yet been setup). Fortunately the buggy behavior hasn't yet been in any official release of libvirt. --- src/qemu/qemu_alias.c | 56 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index fc0653c..010d6b9 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -27,6 +27,7 @@ #include "viralloc.h" #include "virlog.h" #include "virstring.h" +#include "network/bridge_driver.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -293,14 +294,22 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, { if (idx == -1) { size_t i; + idx = 0; for (i = 0; i < def->nhostdevs; i++) { int thisidx; - if ((thisidx = qemuDomainDeviceAliasIndex(def->hostdevs[i]->info, "hostdev")) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to determine device index for hostdev device")); - return -1; - } + + if ((thisidx = qemuDomainDeviceAliasIndex(def->hostdevs[i]->info, "hostdev")) < 0) + continue; /* error just means the alias wasn't "hostdevN", but something else */ + if (thisidx >= idx) + idx = thisidx + 1; + } + /* network interfaces can also have a hostdevN alias */ + for (i = 0; i < def->nnets; i++) { + int thisidx; + + if ((thisidx = qemuDomainDeviceAliasIndex(&def->nets[i]->info, "hostdev")) < 0) + continue; if (thisidx >= idx) idx = thisidx + 1; } @@ -318,22 +327,23 @@ qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx) { + + /* <interface type='hostdev'> uses "hostdevN" as the alias + * We must use "-1" as the index because the caller doesn't know + * that we're now looking for a unique hostdevN rather than netN + */ + if (networkGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) + return qemuAssignDeviceHostdevAlias(def, &net->info.alias, -1); + if (idx == -1) { size_t i; + idx = 0; for (i = 0; i < def->nnets; i++) { int thisidx; - if (virDomainNetGetActualType(def->nets[i]) - == VIR_DOMAIN_NET_TYPE_HOSTDEV) { - /* type='hostdev' interfaces have a hostdev%d alias */ - continue; - } - if ((thisidx = qemuDomainDeviceAliasIndex(&def->nets[i]->info, "net")) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to determine device index for network device")); - return -1; - } + if ((thisidx = qemuDomainDeviceAliasIndex(&def->nets[i]->info, "net")) < 0) + continue; /* failure could be due to "hostdevN" */ if (thisidx >= idx) idx = thisidx + 1; } @@ -392,14 +402,8 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nnets; i++) { - /* type='hostdev' interfaces are also on the hostdevs list, - * and will have their alias assigned with other hostdevs. - */ - if (virDomainNetGetActualType(def->nets[i]) - != VIR_DOMAIN_NET_TYPE_HOSTDEV && - qemuAssignDeviceNetAlias(def, def->nets[i], i) < 0) { + if (qemuAssignDeviceNetAlias(def, def->nets[i], -1) < 0) return -1; - } } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) @@ -414,7 +418,13 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nhostdevs; i++) { - if (qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, i) < 0) + /* we can't start assigning at 0, since netdevs may have used + * up some hostdevN entries already. Also if the HostdevDef is + * linked to a NetDef, they will share an info and the alias + * will already be set, so don't try to set it again. + */ + if (!def->hostdevs[i]->info->alias && + qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0) return -1; } for (i = 0; i < def->nredirdevs; i++) { -- 2.5.5