On 12.10.2012 11:58, Laine Stump wrote:

This function really should have been taking virDevicePCIAddress* instead of the inefficient virDevicePCIAddress (results in copying two entire structs onto the stack rather than just two pointers), and returning a bool true/false (not matching is not necessarily a "failure", as a -1 return would imply, and also using "if (!virDevicePCIAddressEqual(x, y))" to mean "if x == y" is just a bit counterintuitive). --- src/conf/device_conf.c | 21 +++++++++------------ src/conf/device_conf.h | 4 ++-- src/network/bridge_driver.c | 8 ++++---- 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index a852960..7b97f45 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -130,18 +130,15 @@ virDevicePCIAddressFormat(virBufferPtr buf, return 0; }

-int -virDevicePCIAddressEqual(virDevicePCIAddress addr1, - virDevicePCIAddress addr2) +bool +virDevicePCIAddressEqual(virDevicePCIAddress *addr1, + virDevicePCIAddress *addr2) { - int ret = -1; - - if (addr1.domain == addr2.domain && - addr1.bus == addr2.bus && - addr1.slot == addr2.slot && - addr1.function == addr2.function) { - ret = 0; + if (addr1->domain == addr2->domain && + addr1->bus == addr2->bus && + addr1->slot == addr2->slot && + addr1->function == addr2->function) { + return true; } - - return ret; + return false; } diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 26d5637..5318738 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -59,8 +59,8 @@ int virDevicePCIAddressFormat(virBufferPtr buf, virDevicePCIAddress addr, bool includeTypeInAddr);

-int virDevicePCIAddressEqual(virDevicePCIAddress addr1, - virDevicePCIAddress addr2); +bool virDevicePCIAddressEqual(virDevicePCIAddress *addr1, + virDevicePCIAddress *addr2);

VIR_ENUM_DECL(virDeviceAddressPciMulti) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a41c49c..917dd34 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3820,8 +3820,8 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (netdef->forwardIfs[ii].type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI && - (virDevicePCIAddressEqual(hostdev->source.subsys.u.pci, - netdef->forwardIfs[ii].device.pci) == 0)) { + virDevicePCIAddressEqual(&hostdev->source.subsys.u.pci, + &netdef->forwardIfs[ii].device.pci)) { dev = &netdef->forwardIfs[ii]; break; } @@ -3972,8 +3972,8 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (netdef->forwardIfs[ii].type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI && - (virDevicePCIAddressEqual(hostdev->source.subsys.u.pci, - netdef->forwardIfs[ii].device.pci) == 0)) { + virDevicePCIAddressEqual(&hostdev->source.subsys.u.pci, + &netdef->forwardIfs[ii].device.pci)) { dev = &netdef->forwardIfs[ii]; break; }

ACK Michal

On 12.10.2012 11:58, Laine Stump wrote:

This does a shallow copy of all the bits, then strdups the two items that are actually allocated separately. --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 27 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9bcd47b..629c0bf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2018,6 +2018,30 @@ virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info, unsigned int flags) return false; }

+int +virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, + virDomainDeviceInfoPtr src) +{ + /* Assume that dst is already cleared */ + + /* first a shallow copy of *everything* */ + *dst = *src; + + /* then redo the two fields that are pointers */ + dst->alias = NULL; + dst->romfile = NULL; + + if (src->alias && !(dst->alias = strdup(src->alias))) { + virReportOOMError(); + return -1; + } + if (src->romfile && !(dst->romfile = strdup(src->romfile))) { + virReportOOMError(); + return -1; + } + return 0; +} + void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) { VIR_FREE(info->alias); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 06bb96c..f6f4739 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1885,6 +1885,8 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virCapsPtr caps, virDomainDeviceDefPtr src); int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); +int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, + virDomainDeviceInfoPtr src); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 50e2720..3bf1a08 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -328,6 +328,7 @@ virDomainDeviceAddressTypeToString; virDomainDeviceDefCopy; virDomainDeviceDefFree; virDomainDeviceDefParse; +virDomainDeviceInfoCopy; virDomainDeviceInfoIterate; virDomainDeviceTypeToString; virDomainDiskBusTypeToString;

ACK Michal

On 12.10.2012 11:58, Laine Stump wrote:

* qemuDomainChangeNet is going to need 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

Note that this function will refuse to make any change that requires the *guest* size 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.

This patch *does not* implement the "reconnect" code - there is a placeholder that turns that into an error as well. 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. ---

Note that the function qemuDomainChangeNet has essentially been totally rewritten, so don't waste time trying to correlate between the "-" and "+" lines in that part of the diff.

src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 360 +++++++++++++++++++++++++++++++++++------------- src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 271 insertions(+), 95 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee84b27..323d11e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5984,7 +5984,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 a738b19..5284eb7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1236,14 +1236,14 @@ 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; @@ -1327,124 +1327,300 @@ 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 ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev) { - virDomainNetDefPtr olddev = qemuDomainFindNet(vm, dev); - int ret = 0; + virDomainNetDefPtr newdev = dev->data.net; + virDomainNetDefPtr *olddevslot = qemuDomainFindNet(vm, newdev); + virDomainNetDefPtr olddev; + int oldType, newType; + bool needReconnect = false; + bool needChangeBridge = false; + bool needLinkStateChange = false; + int ret = -1;

- if (!olddev) { + if (!olddevslot || !(olddev = *olddevslot)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("cannot find existing network device to modify")); - return -1; + goto cleanup; }

- if (olddev->type != dev->type) { + 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; + } + + /* 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 */

- break; + /* 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; + }

- case VIR_DOMAIN_NET_TYPE_BRIDGE: - /* allow changing brname */ - break; + newType = virDomainNetGetActualType(newdev);

- 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; + 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; + }

- 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; + if (olddev->type != newdev->type || oldType != newType) { + /* this will certainly require a total remake of the host + * side of the device + */ + needReconnect = true; + } else {

- default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to change config on '%s' network type"), - virDomainNetTypeToString(dev->type)); + /* if the type hasn't changed, check the relevant fields for the + * type = maybe we don't need to reconnect + */ + switch (olddev->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_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;

- /* 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; - } + case VIR_DOMAIN_NET_TYPE_NETWORK: + /* other things handled in common code directly below this switch */ + if (STRNEQ(olddev->data.network.name, newdev->data.network.name)) + needReconnect = true; + break;

- /* 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; - } + case VIR_DOMAIN_NET_TYPE_BRIDGE: + /* maintain previous behavior */ + if (STRNEQ(olddev->data.bridge.brname, olddev->data.bridge.brname)) + needChangeBridge = true; + break;

- 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; + 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; + + } + } + /* 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)) || + 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; + } + + if (olddev->linkstate != newdev->linkstate) + needLinkStateChange = true; + + /* FINALLY - actual perform the required actions */ + if (needReconnect) { + virReportError(VIR_ERR_NO_SUPPORT, + _("unable to change config on '%s' network type"), + virDomainNetTypeToString(newdev->type)); + goto cleanup; }

- if (olddev->linkstate != dev->linkstate) { - if ((ret = qemuDomainChangeNetLinkState(driver, vm, olddev, dev->linkstate)) < 0) - return ret; + if (needChangeBridge && qemuDomainChangeNetBridge(vm, olddev, newdev) < 0) + goto cleanup; + + if (needLinkStateChange && + qemuDomainChangeNetLinkState(driver, vm, olddev, newdev->linkstate) < 0) { + goto cleanup; }

+ 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,

ACK Michal

On 12.10.2012 11:58, Laine Stump wrote:

This is the final patch to resolve:

https://bugzilla.redhat.com/show_bug.cgi?id=805071

This patch finalizes support live change of any/all hostside config of a network device as part of the public virDomainUpdateDeviceFlags API. The changes are done without detaching the device from the guest, so hopefully disruptions are kept to a minimum.

Two new functions are created for the task - qemuDomainDetachNetHostSide() and qemuDomainAttachHostSide() - which detach and reattach just the host side of a network device, while leaving the PCI device on the guest side still attached. Calling these two functions in sequence allows us to completely change the host side of the device, including type of tap device, where the tap is connected, bandwidth/vlan/virtualport settings, etc.

These functions had their starts as cut-pastes of qemuDomainDetachNetDevice and qemuDomainAttachNetDevice. To avoid the headaches of duplicated code we may later want those older functions to call the new functions, but that should wait until the new code has had some real world testing - better to keep it in a lesser role for now, with the existing code path unchanged.

For now, the two new functions are only used by qemuDomainChangeNet() in circumstances that previously would have resulted in an UNSUPPORTED error, but now will instead perform a device reconnect using the modified netdev object.

One potentially ugly bit is that if the Attach of the new config fails, we will have already detached the old config; in order to recover as well as possible under the circumstances, we try to re-attach the old config, which could fail for the same (or similar) reason, leaving a discrepancy between the domain object and the actual state of the domain. There really isn't any good way out of this unfortunately. --- src/qemu/qemu_hotplug.c | 338 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 332 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5284eb7..57a2392 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" @@ -646,7 +647,6 @@ error: return -1; }

- /* XXX conn required for network -> bridge resolution */ int qemuDomainAttachNetDevice(virConnectPtr conn, struct qemud_driver *driver, @@ -1249,6 +1249,304 @@ static virDomainNetDefPtr *qemuDomainFindNet(virDomainObjPtr vm, return NULL; }

+/* qemuDomainDetachNetHostSide: + * + * detach only the Host side of the netdev. Leave the Guest side + * attached. The intent is for this to *only* be called just prior to + * calling qemuDomainAttachNetHostSide() to reconnect to a different + * host side source (or with different settings). + * + * The device is *not* removed from the domain's list of nets, and + * the "actual device" is not released. + * + * Returns 0 on success, -1 on failure. + */ +static int +qemuDomainDetachNetHostSide(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainNetDefPtr detach) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + int vlan; + char *hostnet_name = NULL; + virNetDevVPortProfilePtr vport = NULL; + + if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* it's not possible to detach just the host side of a hostdev */ + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot detach only host side of a hostdev network device")); + goto cleanup; + } + + if ((vlan = qemuDomainNetVLAN(detach)) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("unable to determine original VLAN")); + goto cleanup; + } + + if (virAsprintf(&hostnet_name, "host%s", detach->info.alias) < 0) { + virReportOOMError(); + goto cleanup; + } + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (qemuCapsGet(priv->caps, QEMU_CAPS_NETDEV) && + qemuCapsGet(priv->caps, QEMU_CAPS_DEVICE)) { + ret = qemuMonitorRemoveNetdev(priv->mon, hostnet_name); + } else { + ret = qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name); + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + virDomainAuditNet(vm, detach, NULL, "detach", ret == 0); + if (ret < 0) + goto cleanup; + + virDomainConfNWFilterTeardown(detach); + + if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_DIRECT) { + ignore_value(virNetDevMacVLanDeleteWithVPortProfile( + detach->ifname, &detach->mac, + virDomainNetGetActualDirectDev(detach), + virDomainNetGetActualDirectMode(detach), + virDomainNetGetActualVirtPortProfile(detach), + driver->stateDir)); + } + + if ((driver->macFilter) && (detach->ifname != NULL)) { + if ((errno = networkDisallowMacOnPort(driver, + detach->ifname, + &detach->mac))) { + virReportSystemError(errno, + _("failed to remove ebtables rule on '%s'"), + detach->ifname); + } + } + + vport = virDomainNetGetActualVirtPortProfile(detach); + if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) + ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(detach), + detach->ifname)); + ret = 0; +cleanup: + if (!ret) + networkReleaseActualDevice(detach); + VIR_FREE(hostnet_name); + return ret; +} + +/* qemuDomainAttachNetHostSide: + * + * attach only the Host side of the netdev. Assume that the Guest side + * is already attached. The intent is for this to *only* be called + * just after calling qemuDomainDetachNetHostSide() to reconnect to a + * different host side source (or with different settings). + * + * The device is assumed to be already in the domain's list of nets, + * and the "actualDevice" is already allocated. Also, it isn't removed + * from the domain nets list in case of failure. + * + * Returns 0 on success, -1 on failure. + * + * XXX: conn required for network -> bridge resolution +*/ +static int +qemuDomainAttachNetHostSide(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainNetDefPtr net) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *tapfd_name = NULL; + int tapfd = -1; + char *vhostfd_name = NULL; + int vhostfd = -1; + char *netstr = NULL; + virNetDevVPortProfilePtr vport = NULL; + int ret = -1; + int vlan; + bool iface_connected = false; + int actualType; + char *netdev_name = NULL; + + actualType = virDomainNetGetActualType(net); + + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* We can't attach just one side of one of these devices */ + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot attach only host side of a hostdev network device")); + goto cleanup; + } + + if (!qemuCapsGet(priv->caps, QEMU_CAPS_HOST_NET_ADD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("installed qemu version does not support host_net_add")); + goto cleanup; + } + + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { + /* + * If type=bridge then we attempt to allocate the tap fd here + * only if running as root or if "-netdev bridge" (using the + * qemu setuid network helper) is not supported. + */ + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + driver->privileged || + (!qemuCapsGet (priv->caps, QEMU_CAPS_NETDEV_BRIDGE))) { + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, + net, priv->caps)) < 0) { + goto cleanup; + } + iface_connected = true; + if (qemuOpenVhostNet(vm->def, net, priv->caps, &vhostfd) < 0) + goto cleanup; + } + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, priv->caps, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) + goto cleanup; + iface_connected = true; + if (qemuOpenVhostNet(vm->def, net, priv->caps, &vhostfd) < 0) + goto cleanup; + } + + if (qemuCapsGet(priv->caps, QEMU_CAPS_NETDEV) && + qemuCapsGet(priv->caps, QEMU_CAPS_DEVICE)) { + vlan = -1; + } else { + vlan = qemuDomainNetVLAN(net); + if (vlan < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unable to attach network devices without vlan")); + goto cleanup; + } + } + + if (tapfd != -1) { + if (virAsprintf(&tapfd_name, "fd-%s", net->info.alias) < 0) + goto no_memory; + } + + if (vhostfd != -1) { + if (virAsprintf(&vhostfd_name, "vhostfd-%s", net->info.alias) < 0) + goto no_memory; + } + + if (qemuCapsGet(priv->caps, QEMU_CAPS_NETDEV) && + qemuCapsGet(priv->caps, QEMU_CAPS_DEVICE)) { + if (!(netstr = qemuBuildHostNetStr(net, driver, priv->caps, + ',', -1, tapfd_name, + vhostfd_name))) + goto cleanup; + } else { + if (!(netstr = qemuBuildHostNetStr(net, driver, priv->caps, + ' ', vlan, tapfd_name, + vhostfd_name))) + goto cleanup; + } + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (qemuCapsGet(priv->caps, QEMU_CAPS_NETDEV) && + qemuCapsGet(priv->caps, QEMU_CAPS_DEVICE)) { + if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfd_name, + vhostfd, vhostfd_name) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto cleanup; + } + } else { + if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfd_name, + vhostfd, vhostfd_name) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto cleanup; + } + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + + VIR_FORCE_CLOSE(tapfd); + VIR_FORCE_CLOSE(vhostfd); + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + + /* set link state */ + if (net->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) { + if (!net->info.alias) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device alias not found: cannot set link state to down")); + } else { + qemuDomainObjEnterMonitorWithDriver(driver, vm); + + if (qemuCapsGet(priv->caps, QEMU_CAPS_NETDEV)) { + if (qemuMonitorSetLink(priv->mon, net->info.alias, + VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto try_remove; + } + } else { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("setting of link state not supported: Link is up")); + } + + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + /* link set to down */ + } + + virDomainAuditNet(vm, NULL, net, "attach", true); + + ret = 0; +cleanup: + if (ret < 0 && iface_connected) { + virDomainConfNWFilterTeardown(net); + + vport = virDomainNetGetActualVirtPortProfile(net); + if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) + ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(net), net->ifname)); + } + + VIR_FREE(netstr); + VIR_FREE(tapfd_name); + VIR_FORCE_CLOSE(tapfd); + VIR_FREE(vhostfd_name); + VIR_FORCE_CLOSE(vhostfd); + VIR_FREE(netdev_name); + + return ret; + +try_remove: + if (!virDomainObjIsActive(vm)) + goto cleanup; + + if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) + goto no_memory; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (vlan < 0) { + if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) + VIR_WARN("Failed to remove network backend for netdev %s", + netdev_name); + } else { + if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, netdev_name) < 0) + VIR_WARN("Failed to remove network backend for vlan %d, net %s", + vlan, netdev_name); + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + goto cleanup; + +no_memory: + virReportOOMError(); + goto cleanup; +} + static int qemuDomainChangeNetBridge(virDomainObjPtr vm, virDomainNetDefPtr olddev, @@ -1330,7 +1628,7 @@ cleanup: int qemuDomainChangeNet(struct qemud_driver *driver, virDomainObjPtr vm, - virDomainPtr dom ATTRIBUTE_UNUSED, + virDomainPtr dom, virDomainDeviceDefPtr dev) { virDomainNetDefPtr newdev = dev->data.net; @@ -1584,10 +1882,38 @@ qemuDomainChangeNet(struct qemud_driver *driver,

/* FINALLY - actual perform the required actions */ if (needReconnect) { - virReportError(VIR_ERR_NO_SUPPORT, - _("unable to change config on '%s' network type"), - virDomainNetTypeToString(newdev->type)); - goto cleanup; + /* disconnect using the old config */ + if (qemuDomainDetachNetHostSide(driver, vm, olddev) < 0) + goto cleanup; + + /* reconnect using the new config (note that the new + * actualDevice was already allocated above) + */ + if (qemuDomainAttachNetHostSide(dom->conn, driver, vm, newdev) < 0) { + virErrorPtr err; + + /* try to reconnect olddev */ + err = virSaveLastError(); + ignore_value(qemuDomainAttachNetHostSide(dom->conn, driver, vm, newdev)); + virSetError(err); + virFreeError(err); + goto cleanup; + } + + /* we're successfully attached using the new config, so put it + * in place in the domain nets list, and discard the old config + */ + 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. */ + *olddevslot = newdev; + newdev = dev->data.net = NULL; + dev->type = VIR_DOMAIN_DEVICE_NONE; + + needChangeBridge = false; + needLinkStateChange = false; }

if (needChangeBridge && qemuDomainChangeNetBridge(vm, olddev, newdev) < 0)

ACK Michal