[libvirt] [PATCH 3/4] qemu: reorganize qemuDomainChangeNet
Michal Privoznik
mprivozn at redhat.com
Fri Oct 12 13:58:53 UTC 2012
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
More information about the libvir-list
mailing list