- Devel - libvirt List Archives

[libvirt] [PATCHv2 3/4] qemu: reorganize qemuDomainChangeNet
by Laine Stump 15 Oct '12

15 Oct '12

(V1 of this patch is in the following thread: https://www.redhat.com/archives/libvir-list/2012-October/msg00542.html I'm posting it because PATCH 4/4 of that series attempts to use apparently non-existent/non-working functionality in qemu; modifying PATCH 3/4 slightly gives back some of the functionality lost by not having 4/4. Once this is ACKed, I will push 1-3 only, and leave 4/4 untilits problem is discovered/resolved) **** This patch resolves: https://bugzilla.redhat.com/show_bug.cgi?id=805071 to the extent that it can be resolved with current qemu functionality. It attempts to detect as many situations as possible when the simple operation of disconnecting an existing tap device from one bridge and attaching it to another will satisfy the change requested in virDomainUpdateDeviceFlags() for a network device. Before this patch, that situation could only be detected if the pre-change interface *and* the post-change interface definition were both "type='bridge'". After this patch, it can also be detected if the before or after interfaces are any combination of type='bridge' and type='network' (the networks can be <forward mode='nat|route|bridge'>, as long as they use a Linux host bridge and not macvtap connections). This extra effort is especially useful since the recent discovery that a netdev_del+netdev_add combo (to reconnect the network device with completely different hostside configuration) doesn't work properly with current qemu (1.2) unless it is accompanied by the matching device_del+device_add - see this mailing list message for details: http://lists.nongnu.org/archive/html/qemu-devel/2012-10/msg02355.html (A slight modification of the patch referenced there has been prepared to apply on top of this patch, but won't be pushed until qemu can be made to work with it.) * qemuDomainChangeNet needs access to the virDomainDeviceDef that holds the new netdef (so that it can clear out the virDomainDeviceDef if it ends up using the NetDef to replace the original), so the virDomainNetDefPtr arg is replaced with a virDomainDeviceDefPtr. * qemuDomainChangeNet previously checked for *some* changes to the interface config, but this check was by no means complete. It was also a bit disorganized. This refactoring of the code is (I believe) complete in its check of all NetDef attributes that might be changed, and either returns a failure (for changes that are simply impossible), or sets one of three flags: needLinkStateChange - if the device link state needs to go up/down needBridgeChange - if everything else is the same, but it needs to be connected to a difference linux host bridge needReconnect - if the entire host side of the device needs to be torn down and reconstructed (currently non-working, as mentioned above) Note that this function will refuse to make any change that requires the *guest* side of the device to be detached (e.g. changing the PCI address or mac address). Those would be disruptive enough to the guest that it's reasonable to require an explicit detach/attach sequence from the management application. * As mentioned above, qemuDomainChangeNet also does its best to understand when a simple change in attached bridge for the existing tap device will work vs. the need to completely tear down/reconstruct the host side of the device (including tap device). This patch *does not* implement the "reconnect" code anyway - there is a placeholder that turns that into an error. Rather, the purpose of this patch is to replicate existing behavior with code that is ready to have that functionality plugged in in a later patch. * The expanded uses for qemuDomainChangeNetBridge meant that it needed to be enhanced as well - it no longer replaces the original brname string in olddev with the new brname; instead, it relies on the caller to replace the *entire* olddev with newdev (since we've gone to great lengths to assure they are functionally identical other than the name of the bridge, this is now not only safe, but more correct). Additionally, qemuDomainNetChangeBridge can now set the bridge for type='network' interfaces as well as plain type='bridge' interfaces. (Note that I had to make this change simultaneous to the reorganization of qemuDomainChangeNet because the two are too closely intertwined to separate). --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 502 ++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 401 insertions(+), 107 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0787039..3a78465 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5987,7 +5987,7 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm, ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics); break; case VIR_DOMAIN_DEVICE_NET: - ret = qemuDomainChangeNet(driver, vm, dom, dev->data.net) + ret = qemuDomainChangeNet(driver, vm, dom, dev); break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 78f1278..0fe236f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -33,6 +33,7 @@ #include "domain_audit.h" #include "domain_nwfilter.h" #include "logging.h" +#include "datatypes.h" #include "virterror_internal.h" #include "memory.h" #include "pci.h" @@ -1209,27 +1210,88 @@ cleanup: return -1; } -static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, - virDomainNetDefPtr dev) +static virDomainNetDefPtr *qemuDomainFindNet(virDomainObjPtr vm, + virDomainNetDefPtr dev) { int i; for (i = 0; i < vm->def->nnets; i++) { if (virMacAddrCmp(&vm->def->nets[i]->mac, &dev->mac) == 0) - return vm->def->nets[i]; + return &vm->def->nets[i]; } return NULL; } -static -int qemuDomainChangeNetBridge(virDomainObjPtr vm, - virDomainNetDefPtr olddev, - virDomainNetDefPtr newdev) +static char * +qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) +{ + char *brname = NULL; + int actualType = virDomainNetGetActualType(net); + + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + const char *tmpbr = virDomainNetGetActualBridgeName(net); + if (!tmpbr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("interface is missing bridge name")); + goto cleanup; + } + /* we need a copy, not just a pointer to the original */ + if (!(brname = strdup(tmpbr))) { + virReportOOMError(); + goto cleanup; + } + } else if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { + int active; + virErrorPtr errobj; + virNetworkPtr network; + + if (!(network = virNetworkLookupByName(conn, net->data.network.name))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Couldn't find network '%s'"), + net->data.network.name); + goto cleanup; + } + + active = virNetworkIsActive(network); + if (active == 1) { + brname = virNetworkGetBridgeName(network); + } else if (active == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Network '%s' is not active."), + net->data.network.name); + } + + /* Make sure any above failure is preserved */ + errobj = virSaveLastError(); + virNetworkFree(network); + virSetError(errobj); + virFreeError(errobj); + goto cleanup; + + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Network type %d is not supported"), + virDomainNetGetActualType(net)); +cleanup: + return brname; +} + +static int +qemuDomainChangeNetBridge(virConnectPtr conn, + virDomainObjPtr vm, + virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev) { int ret = -1; - char *oldbridge = olddev->data.bridge.brname; - char *newbridge = newdev->data.bridge.brname; + char *oldbridge = NULL, *newbridge = NULL; + + if (!(oldbridge = qemuDomainNetGetBridgeName(conn, olddev))) + goto cleanup; + + if (!(newbridge = qemuDomainNetGetBridgeName(conn, newdev))) + goto cleanup; VIR_DEBUG("Change bridge for interface %s: %s -> %s", olddev->ifname, oldbridge, newbridge); @@ -1237,23 +1299,19 @@ int qemuDomainChangeNetBridge(virDomainObjPtr vm, if (virNetDevExists(newbridge) != 1) { virReportError(VIR_ERR_OPERATION_FAILED, _("bridge %s doesn't exist"), newbridge); - return -1; + goto cleanup; } if (oldbridge) { ret = virNetDevBridgeRemovePort(oldbridge, olddev->ifname); virDomainAuditNet(vm, olddev, NULL, "detach", ret == 0); if (ret < 0) - return -1; + goto cleanup; } - /* move newbridge into olddev now so Audit log is correct */ - olddev->data.bridge.brname = newbridge; ret = virNetDevBridgeAddPort(newbridge, olddev->ifname); - virDomainAuditNet(vm, NULL, olddev, "attach", ret == 0); + virDomainAuditNet(vm, NULL, newdev, "attach", ret == 0); if (ret < 0) { - /* restore oldbridge to olddev */ - olddev->data.bridge.brname = oldbridge; ret = virNetDevBridgeAddPort(oldbridge, olddev->ifname); virDomainAuditNet(vm, NULL, olddev, "attach", ret == 0); if (ret < 0) { @@ -1261,12 +1319,14 @@ int qemuDomainChangeNetBridge(virDomainObjPtr vm, _("unable to recover former state by adding port " "to bridge %s"), oldbridge); } - return -1; + goto cleanup; } - /* oldbridge no longer needed, and newbridge moved to olddev */ + /* caller will replace entire olddev with newdev in domain nets list */ + ret = 0; +cleanup: VIR_FREE(oldbridge); - newdev->data.bridge.brname = NULL; - return 0; + VIR_FREE(newbridge); + return ret; } int qemuDomainChangeNetLinkState(struct qemud_driver *driver, @@ -1300,124 +1360,358 @@ cleanup: return ret; } -int qemuDomainChangeNet(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainPtr dom ATTRIBUTE_UNUSED, - virDomainNetDefPtr dev) - +int +qemuDomainChangeNet(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainPtr dom, + virDomainDeviceDefPtr dev) { - virDomainNetDefPtr olddev = qemuDomainFindNet(vm, dev); - int ret = 0; + virDomainNetDefPtr newdev = dev->data.net; + virDomainNetDefPtr *devslot = qemuDomainFindNet(vm, newdev); + virDomainNetDefPtr olddev; + int oldType, newType; + bool needReconnect = false; + bool needBridgeChange = false; + bool needLinkStateChange = false; + bool needReplaceDevDef = false; + int ret = -1; - if (!olddev) { + if (!devslot || !(olddev = *devslot)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("cannot find existing network device to modify")); - return -1; + goto cleanup; + } + + oldType = virDomainNetGetActualType(olddev); + if (oldType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* no changes are possible to a type='hostdev' interface */ + virReportError(VIR_ERR_NO_SUPPORT, + _("cannot change config of '%s' network type"), + virDomainNetTypeToString(oldType)); + goto cleanup; } - if (olddev->type != dev->type) { + /* Check individual attributes for changes that can't be done to a + * live netdev. These checks *mostly* go in order of the + * declarations in virDomainNetDef in order to assure nothing is + * omitted. (exceptiong where noted in comments - in particular, + * some things require that a new "actual device" be allocated + * from the network driver first, but we delay doing that until + * after we've made as many other checks as possible) + */ + + /* type: this can change (with some restrictions), but the actual + * type of the new device connection isn't known until after we + * allocate the "actual" device. + */ + + if (virMacAddrCmp(&olddev->mac, &newdev->mac)) { + char oldmac[VIR_MAC_STRING_BUFLEN], newmac[VIR_MAC_STRING_BUFLEN]; + + virReportError(VIR_ERR_NO_SUPPORT, + _("cannot change network interface mac address " + "from %s to %s"), + virMacAddrFormat(&olddev->mac, oldmac), + virMacAddrFormat(&newdev->mac, newmac)); + goto cleanup; + } + + if (STRNEQ_NULLABLE(olddev->model, newdev->model)) { + virReportError(VIR_ERR_NO_SUPPORT, + _("cannot modify network device model from %s to %s"), + olddev->model ? olddev->model : "(default)", + newdev->model ? newdev->model : "(default)"); + goto cleanup; + } + + if (olddev->model && STREQ(olddev->model, "virtio") && + (olddev->driver.virtio.name != newdev->driver.virtio.name || + olddev->driver.virtio.txmode != newdev->driver.virtio.txmode || + olddev->driver.virtio.ioeventfd != newdev->driver.virtio.ioeventfd || + olddev->driver.virtio.event_idx != newdev->driver.virtio.event_idx)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot change network interface type")); - return -1; + _("cannot modify virtio network device driver attributes")); + goto cleanup; } - if (!virNetDevVPortProfileEqual(olddev->virtPortProfile, dev->virtPortProfile)) { + /* data: this union will be examined later, after allocating new actualdev */ + /* virtPortProfile: will be examined later, after allocating new actualdev */ + + if (olddev->tune.sndbuf_specified != newdev->tune.sndbuf_specified || + olddev->tune.sndbuf != newdev->tune.sndbuf) { + needReconnect = true; + } + + if (STRNEQ_NULLABLE(olddev->script, newdev->script)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot change <virtualport> settings")); + _("cannot modify network device script attribute")); + goto cleanup; } - switch (olddev->type) { - case VIR_DOMAIN_NET_TYPE_USER: - break; + /* ifname: check if it's set in newdev. If not, retain the autogenerated one */ + if (!(newdev->ifname || + (newdev->ifname = strdup(olddev->ifname)))) { + virReportOOMError(); + goto cleanup; + } + if (STRNEQ_NULLABLE(olddev->ifname, newdev->ifname)) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot modify network device tap name")); + goto cleanup; + } - case VIR_DOMAIN_NET_TYPE_ETHERNET: - if (STRNEQ_NULLABLE(olddev->data.ethernet.dev, dev->data.ethernet.dev) || - STRNEQ_NULLABLE(olddev->script, dev->script) || - STRNEQ_NULLABLE(olddev->data.ethernet.ipaddr, dev->data.ethernet.ipaddr)) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot modify ethernet network device configuration")); - return -1; - } - break; + /* info: if newdev->info is empty, fill it in from olddev, + * otherwise verify that it matches - nothing is allowed to + * change. (There is no helper function to do this, so + * individually check the few feidls of virDomainDeviceInfo that + * are relevant in this case). + */ + if (!virDomainDeviceAddressIsValid(&newdev->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) { + goto cleanup; + } + if (!virDevicePCIAddressEqual(&olddev->info.addr.pci, + &newdev->info.addr.pci)) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot modify network device guest PCI address")); + goto cleanup; + } + /* grab alias from olddev if not set in newdev */ + if (!(newdev->info.alias || + (newdev->info.alias = strdup(olddev->info.alias)))) { + virReportOOMError(); + goto cleanup; + } + if (STRNEQ_NULLABLE(olddev->info.alias, newdev->info.alias)) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot modify network device alias")); + goto cleanup; + } + if (olddev->info.rombar != newdev->info.rombar) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot modify network device rom bar setting")); + goto cleanup; + } + if (STRNEQ_NULLABLE(olddev->info.romfile, newdev->info.romfile)) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot modify network rom file")); + goto cleanup; + } + if (olddev->info.bootIndex != newdev->info.bootIndex) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot modify network device boot index setting")); + goto cleanup; + } + /* (end of device info checks) */ - case VIR_DOMAIN_NET_TYPE_SERVER: - case VIR_DOMAIN_NET_TYPE_CLIENT: - case VIR_DOMAIN_NET_TYPE_MCAST: - if (STRNEQ_NULLABLE(olddev->data.socket.address, dev->data.socket.address) || - olddev->data.socket.port != dev->data.socket.port) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot modify network socket device configuration")); - return -1; - } - break; + if (STRNEQ_NULLABLE(olddev->filter, newdev->filter)) + needReconnect = true; - case VIR_DOMAIN_NET_TYPE_NETWORK: - if (STRNEQ_NULLABLE(olddev->data.network.name, dev->data.network.name) || - STRNEQ_NULLABLE(olddev->data.network.portgroup, dev->data.network.portgroup)) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot modify network device configuration")); - return -1; - } + /* bandwidth can be modified, and will be checked later */ + /* vlan can be modified, and will be checked later */ + /* linkstate can be modified */ + + /* allocate new actual device to compare to old - we will need to + * free it if we fail for any reason + */ + if (newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK && + networkAllocateActualDevice(newdev) < 0) { + goto cleanup; + } + + newType = virDomainNetGetActualType(newdev); + + if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* can't turn it into a type='hostdev' interface */ + virReportError(VIR_ERR_NO_SUPPORT, + _("cannot change network interface type to '%s'"), + virDomainNetTypeToString(newType)); + goto cleanup; + } + + if (olddev->type == newdev->type && oldType == newType) { + /* if type hasn't changed, check the relevant fields for the type */ + switch (newdev->type) { + case VIR_DOMAIN_NET_TYPE_USER: + break; + + case VIR_DOMAIN_NET_TYPE_ETHERNET: + if (STRNEQ_NULLABLE(olddev->data.ethernet.dev, + newdev->data.ethernet.dev) || + STRNEQ_NULLABLE(olddev->script, newdev->script) || + STRNEQ_NULLABLE(olddev->data.ethernet.ipaddr, + newdev->data.ethernet.ipaddr)) { + needReconnect = true; + } break; - case VIR_DOMAIN_NET_TYPE_BRIDGE: - /* allow changing brname */ - break; + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + if (STRNEQ_NULLABLE(olddev->data.socket.address, + newdev->data.socket.address) || + olddev->data.socket.port != newdev->data.socket.port) { + needReconnect = true; + } + break; + + case VIR_DOMAIN_NET_TYPE_NETWORK: + if (STRNEQ(olddev->data.network.name, newdev->data.network.name)) { + if (virDomainNetGetActualVirtPortProfile(newdev)) + needReconnect = true; + else + needBridgeChange = true; + } + /* other things handled in common code directly below this switch */ + break; + + case VIR_DOMAIN_NET_TYPE_BRIDGE: + /* all handled in bridge name checked in common code below */ + break; + + case VIR_DOMAIN_NET_TYPE_INTERNAL: + if (STRNEQ_NULLABLE(olddev->data.internal.name, + newdev->data.internal.name)) { + needReconnect = true; + } + break; + + case VIR_DOMAIN_NET_TYPE_DIRECT: + /* all handled in common code directly below this switch */ + break; + + default: + virReportError(VIR_ERR_NO_SUPPORT, + _("unable to change config on '%s' network type"), + virDomainNetTypeToString(newdev->type)); + break; - case VIR_DOMAIN_NET_TYPE_INTERNAL: - if (STRNEQ_NULLABLE(olddev->data.internal.name, dev->data.internal.name)) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot modify internal network device configuration")); - return -1; } - break; + } else { + /* interface type has changed. There are a few special cases + * where this can only require a minor (or even no) change, + * but in most cases we need to do a full reconnection. + * + * If we switch (in either direction) between type='bridge' + * and type='network' (for a traditional managed virtual + * network that uses a host bridge, i.e. forward + * mode='route|nat'), we just need to change the bridge. + */ + if ((oldType == VIR_DOMAIN_NET_TYPE_NETWORK && + newType == VIR_DOMAIN_NET_TYPE_BRIDGE) || + (oldType == VIR_DOMAIN_NET_TYPE_BRIDGE && + newType == VIR_DOMAIN_NET_TYPE_NETWORK)) { + + needBridgeChange = true; + + } else if (oldType == VIR_DOMAIN_NET_TYPE_DIRECT && + newType == VIR_DOMAIN_NET_TYPE_DIRECT) { + + /* this is the case of switching from type='direct' to + * type='network' for a network that itself uses direct + * (macvtap) devices. If the physical device and mode are + * the same, this doesn't require any actual setup + * change. If the physical device or mode *does* change, + * that will be caught in the common section below */ + + } else { + + /* for all other combinations, we'll need a full reconnect */ + needReconnect = true; - case VIR_DOMAIN_NET_TYPE_DIRECT: - if (STRNEQ_NULLABLE(olddev->data.direct.linkdev, dev->data.direct.linkdev) || - olddev->data.direct.mode != dev->data.direct.mode) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot modify direct network device configuration")); - return -1; } - break; + } - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to change config on '%s' network type"), - virDomainNetTypeToString(dev->type)); - break; + /* now several things that are in multiple (but not all) + * different types, and can be safely compared even for those + * cases where they don't apply to a particular type. + */ + if (STRNEQ_NULLABLE(virDomainNetGetActualBridgeName(olddev), + virDomainNetGetActualBridgeName(newdev))) { + if (virDomainNetGetActualVirtPortProfile(newdev)) + needReconnect = true; + else + needBridgeChange = true; + } + if (STRNEQ_NULLABLE(virDomainNetGetActualDirectDev(olddev), + virDomainNetGetActualDirectDev(newdev)) || + virDomainNetGetActualDirectMode(olddev) != virDomainNetGetActualDirectMode(olddev) || + !virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(olddev), + virDomainNetGetActualVirtPortProfile(newdev)) || + !virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev), + virDomainNetGetActualBandwidth(newdev)) || + !virNetDevVlanEqual(virDomainNetGetActualVlan(olddev), + virDomainNetGetActualVlan(newdev))) { + needReconnect = true; } - /* all other unmodifiable parameters */ - if (STRNEQ_NULLABLE(olddev->model, dev->model) || - STRNEQ_NULLABLE(olddev->filter, dev->filter)) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot modify network device configuration")); - return -1; + if (olddev->linkstate != newdev->linkstate) + needLinkStateChange = true; + + /* FINALLY - actually perform the required actions */ + + if (needReconnect) { + virReportError(VIR_ERR_NO_SUPPORT, + _("unable to change config on '%s' network type"), + virDomainNetTypeToString(newdev->type)); + goto cleanup; } - /* check if device name has been set, if no, retain the autogenerated one */ - if (dev->ifname && - STRNEQ_NULLABLE(olddev->ifname, dev->ifname)) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("cannot modify network device configuration")); - return -1; + if (needBridgeChange) { + if (qemuDomainChangeNetBridge(dom->conn, vm, olddev, newdev) < 0) + goto cleanup; + /* we successfully switched to the new bridge, and we've + * determined that the rest of newdev is equivalent to olddev, + * so move newdev into place, so that the */ + needReplaceDevDef = true; } - if (olddev->type == VIR_DOMAIN_NET_TYPE_BRIDGE - && STRNEQ_NULLABLE(olddev->data.bridge.brname, - dev->data.bridge.brname)) { - if ((ret = qemuDomainChangeNetBridge(vm, olddev, dev)) < 0) - return ret; + if (needLinkStateChange && + qemuDomainChangeNetLinkState(driver, vm, olddev, newdev->linkstate) < 0) { + goto cleanup; } - if (olddev->linkstate != dev->linkstate) { - if ((ret = qemuDomainChangeNetLinkState(driver, vm, olddev, dev->linkstate)) < 0) - return ret; + if (needReplaceDevDef) { + /* the changes above warrant replacing olddev with newdev in + * the domain's nets list. + */ + networkReleaseActualDevice(olddev); + virDomainNetDefFree(olddev); + /* move newdev into the nets list, and NULL it out from the + * virDomainDeviceDef that we were given so that the caller + * won't delete it on return. + */ + *devslot = newdev; + newdev = dev->data.net = NULL; + dev->type = VIR_DOMAIN_DEVICE_NONE; } + ret = 0; +cleanup: + /* When we get here, we will be in one of these two states: + * + * 1) newdev has been moved into the domain's list of nets and + * newdev set to NULL, and dev->data.net will be NULL (and + * dev->type is NONE). olddev will have been completely + * released and freed. (aka success) In this case no extra + * cleanup is needed. + * + * 2) newdev has *not* been moved into the domain's list of nets, + * and dev->data.net == newdev (and dev->type == NET). In this * + * case, we need to at least release the "actual device" from * + * newdev (the caller will free dev->data.net a.k.a. newdev, and + * the original olddev is still in used) + * + * Note that case (2) isn't necessarily a failure. It may just be + * that the changes were minor enough that we didn't need to + * replace the entire device object. + */ + if (newdev) + networkReleaseActualDevice(newdev); + return ret; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 36c0580..a7864c3 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -1,7 +1,7 @@ /* * qemu_hotplug.h: QEMU device hotplug management * - * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -77,7 +77,7 @@ int qemuDomainChangeGraphicsPasswords(struct qemud_driver *driver, int qemuDomainChangeNet(struct qemud_driver *driver, virDomainObjPtr vm, virDomainPtr dom, - virDomainNetDefPtr dev); + virDomainDeviceDefPtr dev); int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, -- 1.7.11.7

2 1

[libvirt] Potential race condition problem
by Benjamin Wang (gendwang) 15 Oct '12

15 Oct '12

Hi, Currently virInitialize() method defined in libvirt.c has the following code: int virInitialize(void) { if (initialized) return 0; initialized = 1; if (virThreadInitialize() < 0 || virErrorInitialize() < 0 || virRandomInitialize(time(NULL) ^ getpid()) || virNodeSuspendInit() < 0) return -1; ...... } When two threads access virInitialize method, there is no lock for the "initialized" parameter. If the first thread enters this method and set "initialized" to 1, the second thread could see that "initialized" is 1(Because initialized is not volatiled, I say could). In some situation, before the first thread finishes all the initialization, the second thread could use some resources which should be initialized in Initialize method. If you have any comments, please let me know. Thanks! B.R. Benjamin Wang

4 7

[libvirt] how to enable ceph-rbd in libvirt.
by yue 15 Oct '12

15 Oct '12

1.my libvirtd version is 0.9.10. i rebuild qemu with --enable-rbd. it seems not work if guest-os has a rbd disk. -----rbd disk info ---- <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <auth username='cloud'> <secret type='ceph' uuid='7a91dc24-b072-43c4-98fb-4b2415322b0f'/> </auth> <source protocol='rbd' name='cloud/testrbd'> <host name='192.168.10.4' port='6789'/> </source> <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> </disk> 2.-----------------------error log--------- LC_ALL=C PATH=/sbin:/usr/sbin:/bin:/usr/bin HOME=/root USER=root LOGNAME=root QEMU_AUDIO_DRV=none /usr/libexec/qemu-kvm -S -M rhel6.2.0 -enable-kvm -m 512 -smp 1,sockets=1,cores=1,threads=1 -name xpSP3 -uuid b45bd66a-6700-4905-c5f4-4c799413d7b7 -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/xpSP3.monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=localtime,driftfix=slew -no-shutdown -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -drive file=/var/lib/libvirt/images/xpSP3.img,if=none,id=drive-ide0-0-0,format=raw,cache=none -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -drive file=rbd:cloud/testrbd:id=cloud:key=AQCGbGRQ+M+NGBAATtylZNiSxqCTQ4uaApd+9w==:auth_supported=cephx none:mon_host=192.168.10.4\:6789,if=none,id=drive-virtio-disk1,format=raw -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk1,id=virtio-disk1 -netdev tap,fd=26,id=hostnet0 -device e! 1000,netdev=hostnet0,id=net0,mac=52:54:00:2a:8c:48,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -device usb-tablet,id=input0 -vnc 0.0.0.0:0 -vga std -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 Domain id=6 is tainted: high-privileges char device redirected to /dev/pts/4 qemu-kvm: -drive file=rbd:cloud/testrbd:id=cloud:key=AQCGbGRQ+M+NGBAATtylZNiSxqCTQ4uaApd+9w==:auth_supported=cephx none:mon_host=192.168.10.4\:6789,if=none,id=drive-virtio-disk1,format=raw: could not open disk image rbd:cloud/testrbd:id=cloud:key=AQCGbGRQ+M+NGBAATtylZNiSxqCTQ4uaApd+9w==:auth_supported=cephx none:mon_host=192.168.10.4\:6789: No such file or directory 2012-10-11 10:37:24.215+0000: shutting down

3 11

[libvirt] [PATCH 0/7 v2] CPU tuning improvements
by Osier Yang 15 Oct '12

15 Oct '12

As proposed in https://www.redhat.com/archives/libvir-list/2012-October/msg00522.html, This patch set is trying to resolve the conflicts between <vcpu> , <vcpupin>, and <emulatorpin>, when doing CPU tuning for domain process or vcpu threads. PATCH 1/7 changes all the doc together, as the 3 elements are related with each other too tightly, splitting the changes in separate patches will just make things obscure and hard to review. See commit log of PATCH 1/7 for overview/details about the solution. Osier Yang (7): doc: Clarify the relationship between <vcpu>, <vcpupin>, and <emulatorpin> conf: Do not allow vcpupin's cpuid exceed current vcpus number conf: Initialize the pinning policy for vcpus qemu: Create or remove cgroup when doing vcpu hotpluging qemu: Initialize cpuset for hotplugged vcpu as def->cpuset conf: Prohibit emulatorpin if vcpu placement is auto qemu: Ignore def->cpumask if emulatorpin is specified docs/formatdomain.html.in | 42 +++++++++++------ src/conf/domain_conf.c | 103 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 110 +++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.c | 3 +- 6 files changed, 243 insertions(+), 19 deletions(-) -- 1.7.7.6 Regards, Osier

2 20

[libvirt] [PATCH] daemon: Use $(AM_V_GEN) in a few more places
by Cole Robinson 14 Oct '12

14 Oct '12

--- daemon/Makefile.am | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 3405c67..1c7505b 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -84,10 +84,10 @@ augeastests_DATA = test_libvirtd.aug CLEANFILES += test_libvirtd.aug libvirtd.8: $(srcdir)/libvirtd.8.in - sed \ + $(AM_V_GEN)sed \ -e 's!SYSCONFDIR!$(sysconfdir)!g' \ -e 's!LOCALSTATEDIR!$(localstatedir)!g' \ - < $< > $@-t + < $< > $@-t && \ mv $@-t $@ libvirtd_SOURCES = $(DAEMON_SOURCES) @@ -215,15 +215,15 @@ LOGROTATE_CONFS = libvirtd.qemu.logrotate libvirtd.lxc.logrotate \ BUILT_SOURCES += $(LOGROTATE_CONFS) libvirtd.logrotate: libvirtd.logrotate.in - sed \ + $(AM_V_GEN)sed \ -e 's![@]localstatedir[@]!$(localstatedir)!g' \ - < $< > $@-t + < $< > $@-t && \ mv $@-t $@ libvirtd.qemu.logrotate: libvirtd.qemu.logrotate.in - sed \ + $(AM_V_GEN)sed \ -e 's![@]localstatedir[@]!$(localstatedir)!g' \ - < $< > $@-t + < $< > $@-t && \ mv $@-t $@ libvirtd.lxc.logrotate: libvirtd.lxc.logrotate.in -- 1.7.11.4

2 2

[libvirt] [PATCH 0/4] qemu: support live change of all netdev hostside config
by Laine Stump 14 Oct '12

14 Oct '12

This patch series resolves: https://bugzilla.redhat.com/show_bug.cgi?id=805071 It enhances virDomainUpdateDeviceFlags for network devices to allow live change of just about everything that doesn't affect what hardware is presented to the guest (e.g. the device model, mac address, PCI address, and other PCI attributes can't be changed, but the type/details of the connection to the host network can be changed, including bandwidth, type of tap device, vlan setting, nwfilter). I haven't tested the new code yet beyond compiling, but wanted to get the patches up on the list since the weekend is coming. I'll be testing this morning and hopefully won't encounter any major problems.

2 10

[libvirt] [PATCH] Properly parse (unsigned) long long
by Guido Günther 13 Oct '12

13 Oct '12

This fixes problems on platforms where sizeof(long) != sizeof(long long) like ia32. --- Cheers, -- Guido python/generator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/generator.py b/python/generator.py index a98a894..1ef76e1 100755 --- a/python/generator.py +++ b/python/generator.py @@ -259,8 +259,8 @@ py_types = { 'double': ('d', None, "double", "double"), 'unsigned int': ('i', None, "int", "int"), 'unsigned long': ('l', None, "long", "long"), - 'long long': ('l', None, "longlong", "long long"), - 'unsigned long long': ('l', None, "longlong", "long long"), + 'long long': ('L', None, "longlong", "long long"), + 'unsigned long long': ('K', None, "longlong", "long long"), 'unsigned char *': ('z', None, "charPtr", "char *"), 'char *': ('z', None, "charPtr", "char *"), 'const char *': ('z', None, "constcharPtr", "const char *"), -- 1.7.10.4

2 2

[libvirt] [PATCH] spec: Add support for libssh2 transport
by Peter Krempa 13 Oct '12

13 Oct '12

Libssh2 transport support was enabled lately but the spec file wasn't updated to take this into account. This caused libvirt to be built without libssh2 support in Red Hat based OSes. --- I'm not sure how the {?rhel} >= 6 macro is evaluated: Is there a possibility to be more specific with the version? libvirt.spec.in | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 318fe92..75623eb 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -108,6 +108,7 @@ %define with_systemd 0%{!?_without_systemd:0} %define with_numad 0%{!?_without_numad:0} %define with_firewalld 0%{!?_without_firewalld:0} +%define with_libssh2_transport 0%{!?_without_libssh2_transport:0} # Non-server/HV driver defaults which are always enabled %define with_python 0%{!?_without_python:1} @@ -229,6 +230,11 @@ %endif %endif +# Enable libssh2 transport for new enough distros +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 6 +%define with_libssh2_transport 0%{!?_without_libssh2_transport:1} +%endif + # Disable some drivers when building without libvirt daemon. # The logic is the same as in configure.ac %if ! %{with_libvirtd} @@ -491,9 +497,13 @@ BuildRequires: numactl-devel %if %{with_capng} BuildRequires: libcap-ng-devel >= 0.5.0 %endif -%if %{with_phyp} +%if %{with_phyp} || %{with_libssh2_transport} +%if %{with_libssh2_transport} +BuildRequires: libssh2-devel >= 1.3.0 +%else BuildRequires: libssh2-devel %endif +%endif %if %{with_netcf} %if 0%{?fedora} >= 18 || 0%{?rhel} >= 7 -- 1.7.12.3

2 2

[libvirt] [PATCH] selinux: Use raw contexts
by Martin Kletzander 12 Oct '12

12 Oct '12

We are currently able to work only with non-translated SELinux contexts, but we are using functions that work with translated contexts throughout the code. This patch swaps all SELinux context translation relative calls with their raw sisters to avoid parsing problems. The problems can be experienced with mcstrans for example. Thanks Laurent Bigonville for finding this out. --- configure.ac | 4 ++-- src/security/security_selinux.c | 26 +++++++++++++------------- src/storage/storage_backend.c | 2 +- tests/securityselinuxhelper.c | 6 +++--- tests/securityselinuxtest.c | 2 +- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/configure.ac b/configure.ac index bcdea9c..08dc63d 100644 --- a/configure.ac +++ b/configure.ac @@ -1440,14 +1440,14 @@ if test "$with_selinux" != "no"; then old_libs="$LIBS" if test "$with_selinux" = "check"; then AC_CHECK_HEADER([selinux/selinux.h],[],[with_selinux=no]) - AC_CHECK_LIB([selinux], [fgetfilecon],[],[with_selinux=no]) + AC_CHECK_LIB([selinux], [fgetfilecon_raw],[],[with_selinux=no]) if test "$with_selinux" != "no"; then with_selinux="yes" fi else fail=0 AC_CHECK_HEADER([selinux/selinux.h],[],[fail=1]) - AC_CHECK_LIB([selinux], [fgetfilecon],[],[fail=1]) + AC_CHECK_LIB([selinux], [fgetfilecon_raw],[],[fail=1]) test $fail = 1 && AC_MSG_ERROR([You must install the libselinux development package in order to compile libvirt with basic SELinux support]) fi diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index d55c60d..10135ed 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -111,7 +111,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) char *sens, *cat, *tmp; int catMin, catMax, catRange; - if (getcon(&ourSecContext) < 0) { + if (getcon_raw(&ourSecContext) < 0) { virReportSystemError(errno, "%s", _("Unable to get current process SELinux context")); goto cleanup; @@ -252,7 +252,7 @@ virSecuritySELinuxGenNewContext(const char *basecontext, VIR_DEBUG("basecontext=%s mcs=%s isObjectContext=%d", basecontext, mcs, isObjectContext); - if (getcon(&ourSecContext) < 0) { + if (getcon_raw(&ourSecContext) < 0) { virReportSystemError(errno, "%s", _("Unable to get current process SELinux context")); goto cleanup; @@ -612,7 +612,7 @@ virSecuritySELinuxReserveSecurityLabel(virSecurityManagerPtr mgr, if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC) return 0; - if (getpidcon(pid, &pctx) == -1) { + if (getpidcon_raw(pid, &pctx) == -1) { virReportSystemError(errno, _("unable to get PID %d security context"), pid); return -1; @@ -713,7 +713,7 @@ virSecuritySELinuxGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN { security_context_t ctx; - if (getpidcon(pid, &ctx) == -1) { + if (getpidcon_raw(pid, &ctx) == -1) { virReportSystemError(errno, _("unable to get PID %d security context"), pid); @@ -753,10 +753,10 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional) VIR_INFO("Setting SELinux context on '%s' to '%s'", path, tcon); - if (setfilecon(path, tcon) < 0) { + if (setfilecon_raw(path, tcon) < 0) { int setfilecon_errno = errno; - if (getfilecon(path, &econ) >= 0) { + if (getfilecon_raw(path, &econ) >= 0) { if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ @@ -818,10 +818,10 @@ virSecuritySELinuxFSetFilecon(int fd, char *tcon) VIR_INFO("Setting SELinux context on fd %d to '%s'", fd, tcon); - if (fsetfilecon(fd, tcon) < 0) { + if (fsetfilecon_raw(fd, tcon) < 0) { int fsetfilecon_errno = errno; - if (fgetfilecon(fd, &econ) >= 0) { + if (fgetfilecon_raw(fd, &econ) >= 0) { if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ @@ -1577,7 +1577,7 @@ virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr, return -1; } - if (setexeccon(secdef->label) == -1) { + if (setexeccon_raw(secdef->label) == -1) { virReportSystemError(errno, _("unable to set security context '%s'"), secdef->label); @@ -1622,7 +1622,7 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr, goto done; } - if (getcon(&scon) == -1) { + if (getcon_raw(&scon) == -1) { virReportSystemError(errno, _("unable to get current process context '%s'"), secdef->label); @@ -1645,7 +1645,7 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr, VIR_DEBUG("Setting VM %s socket context %s", def->name, context_str(proccon)); - if (setsockcreatecon(context_str(proccon)) == -1) { + if (setsockcreatecon_raw(context_str(proccon)) == -1) { virReportSystemError(errno, _("unable to set socket security context '%s'"), context_str(proccon)); @@ -1688,7 +1688,7 @@ virSecuritySELinuxSetSecuritySocketLabel(virSecurityManagerPtr mgr, VIR_DEBUG("Setting VM %s socket context %s", vm->name, secdef->label); - if (setsockcreatecon(secdef->label) == -1) { + if (setsockcreatecon_raw(secdef->label) == -1) { virReportSystemError(errno, _("unable to set socket security context '%s'"), secdef->label); @@ -1728,7 +1728,7 @@ virSecuritySELinuxClearSecuritySocketLabel(virSecurityManagerPtr mgr, return -1; } - if (setsockcreatecon(NULL) == -1) { + if (setsockcreatecon_raw(NULL) == -1) { virReportSystemError(errno, _("unable to clear socket security context '%s'"), secdef->label); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e159bd2..aea70e2 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1227,7 +1227,7 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, #if HAVE_SELINUX /* XXX: make this a security driver call */ - if (fgetfilecon(fd, &filecon) == -1) { + if (fgetfilecon_raw(fd, &filecon) == -1) { if (errno != ENODATA && errno != ENOTSUP) { virReportSystemError(errno, _("cannot get file context of '%s'"), diff --git a/tests/securityselinuxhelper.c b/tests/securityselinuxhelper.c index 43676de..dc63ff3 100644 --- a/tests/securityselinuxhelper.c +++ b/tests/securityselinuxhelper.c @@ -31,7 +31,7 @@ * the process context, where as in fact we're faking it all */ -int getcon(security_context_t *context) +int getcon_raw(security_context_t *context) { if (getenv("FAKE_CONTEXT") == NULL) { *context = NULL; @@ -43,7 +43,7 @@ int getcon(security_context_t *context) return 0; } -int getpidcon(pid_t pid, security_context_t *context) +int getpidcon_raw(pid_t pid, security_context_t *context) { if (pid != getpid()) { *context = NULL; @@ -60,7 +60,7 @@ int getpidcon(pid_t pid, security_context_t *context) return 0; } -int setcon(security_context_t context) +int setcon_raw(security_context_t context) { return setenv("FAKE_CONTEXT", context, 1); } diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index 848a390..8bcf3a1 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -217,7 +217,7 @@ testSELinuxGenLabel(const void *opaque) context_t con = NULL; context_t imgcon = NULL; - if (setcon((security_context_t)data->pidcon) < 0) { + if (setcon_raw((security_context_t)data->pidcon) < 0) { perror("Cannot set process security context"); return -1; } -- 1.7.12.3

2 4

[libvirt] (no subject)
by cbley＠av-test.de 12 Oct '12

12 Oct '12

>From 9c4881c67472575c3f1982f0921a4292025a9620 Mon Sep 17 00:00:00 2001 Message-Id: <9c4881c67472575c3f1982f0921a4292025a9620.1350037981.git.cbley(a)av-test.de> In-Reply-To: <cover.1350037981.git.cbley(a)av-test.de> References: <2DA6F4A3691599408358374D182280F9141CF6(a)xmb-rcd-x04.cisco.com> <cover.1350037981.git.cbley(a)av-test.de> From: cbley(a)av-test.de (Claudio Bley) Date: Fri, 17 Aug 2012 14:45:26 +0200 Subject: [PATCH 01/15] Explicitly set includeAntRuntime to false for javac tasks. To: libvir-list(a)redhat.com Organization: AV-Test GmbH, Germany This prevents warnings like this: warning: 'includeantruntime' was not set, defaulting to build.sysclasspath=last; set to false for repeatable builds --- build.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build.xml b/build.xml index 0b009aa..158a8a5 100644 --- a/build.xml +++ b/build.xml @@ -42,8 +42,8 @@ </target> <target name="build" depends="init" description="builds the code and jar files"> - <javac srcdir="src/main/java" includes="**/*.java" classpathref="compile.classpath" destdir="target/classes" /> - <javac srcdir="src/test/java" includes="**/*.java" classpathref="test.classpath" destdir="target/testclasses" /> + <javac srcdir="src/main/java" debug="${javac.debug}" includes="**/*.java" classpathref="compile.classpath" destdir="target/classes" includeAntRuntime="false" /> + <javac srcdir="src/test/java" debug="${javac.debug}" includes="**/*.java" classpathref="test.classpath" destdir="target/testclasses" includeAntRuntime="false" /> <jar destfile="${jar.file}" basedir="target/classes" /> </target> -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern

3 3