[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/4] qemu: reorganize qemuDomainChangeNet



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]