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

Re: [libvirt] [PATCH] qemu: Don't parse device twice in attach/detach



On 03/01/2012 01:56 PM, Michal Privoznik wrote:
> Some nits are generated during XML parse (e.g. MAC address of
> an interface); However, with current implementation, if we
> are plugging a device both to persistent and live config,
> we parse given XML twice: first time for live, second for config.
> This is wrong then as the second time we are not guaranteed
> to generate same values as we did for the first time.
> To prevent that we need to create a copy of DeviceDefPtr;
> This is done through format/parse process instead of writing
> functions for deep copy as it is easier to maintain:
> adding new field to any virDomain*DefPtr doesn't require change
> of copying function.
> ---
>  src/conf/domain_conf.c   |   94 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |    3 +
>  src/libvirt_private.syms |    1 +
>  src/qemu/qemu_driver.c   |   40 +++++++++++--------
>  4 files changed, 121 insertions(+), 17 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f9654f1..f001319 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14118,3 +14118,97 @@ virDomainNetFind(virDomainDefPtr def, const char *device)
>  
>      return net;
>  }
> +
> +#define VIR_DOMAIN_DEVICE_FORMAT(func) \
> +    if (func < 0) \
> +        goto cleanup; \
> +    xmlStr = virBufferContentAndReset(&buf); \
> +    ret = virDomainDeviceDefParse(caps, def, xmlStr, flags)
> +
> +virDomainDeviceDefPtr
> +virDomainDeviceDefCopy(virCapsPtr caps,
> +                       const virDomainDefPtr def,
> +                       virDomainDeviceDefPtr src)
> +{
> +    virDomainDeviceDefPtr ret = NULL;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int flags = VIR_DOMAIN_XML_INACTIVE;


I'm concerned that this function has the appearance of being general
purpose, but actually isn't. There are many bits of data in
virDomainNetDef that won't be copied, because they're only
parsed/formatted if extra flags are given in the calls to Parse and
Format (VIR_DOMAIN_XML_INTERNAL_STATUS,
VIR_DOMAIN_XML_INTERNAL_STATUS_ACTUAL_NET, etc).

For the current use case, it should work adequately, since we know that
we just created the object by parsing with a limited flag set, but I can
imagine the day 6 months from now when somebody who doesn't know the
history/limitations of ths new function finds it and mistakenly believes
it's the answer to their current (probably very different) problem (and
then commits code with a bug that doesn't show up until it's in the field)

It's probably too much trouble in the short term to try and make this
function completely general purpose, but I think at least the function
name should reflect the limited nature and/or both the declaration and
definition of the function should be accompanied by a comment noting the
limitations.

Beyond that, this looks like the right way to fix the problem. Note that
libxl_driver.c:libxmlDomainModifyDeviceFlags() has exactly the same
problem, so maybe its fix can be included in this patch.

> +    char *xmlStr = NULL;
> +
> +    switch(src->type) {
> +    case VIR_DOMAIN_DEVICE_DISK:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainDiskDefFormat(&buf,
> +                                                        src->data.disk,
> +                                                        flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_LEASE:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainLeaseDefFormat(&buf,
> +                                                         src->data.lease));
> +        break;
> +    case VIR_DOMAIN_DEVICE_FS:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainFSDefFormat(&buf,
> +                                                      src->data.fs,
> +                                                      flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_NET:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainNetDefFormat(&buf,
> +                                                       src->data.net,
> +                                                       flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_INPUT:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainInputDefFormat(&buf,
> +                                                         src->data.input,
> +                                                         flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_SOUND:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainSoundDefFormat(&buf,
> +                                                         src->data.sound,
> +                                                         flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_VIDEO:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainVideoDefFormat(&buf,
> +                                                         src->data.video,
> +                                                         flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_HOSTDEV:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainHostdevDefFormat(&buf,
> +                                                           src->data.hostdev,
> +                                                           flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainWatchdogDefFormat(&buf,
> +                                                            src->data.watchdog,
> +                                                            flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_CONTROLLER:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainControllerDefFormat(&buf,
> +                                                              src->data.controller,
> +                                                              flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_GRAPHICS:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainGraphicsDefFormat(&buf,
> +                                                            src->data.graphics,
> +                                                            flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_HUB:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainHubDefFormat(&buf,
> +                                                       src->data.hub,
> +                                                       flags));
> +        break;
> +    case VIR_DOMAIN_DEVICE_REDIRDEV:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainRedirdevDefFormat(&buf,
> +                                                            src->data.redirdev,
> +                                                            flags));
> +        break;
> +    default:
> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                             _("Copying definition of '%d' type "
> +                               "is not implemented yet."),
> +                             src->type);
> +        break;
> +    }
> +
> +cleanup:
> +    VIR_FREE(xmlStr);
> +    return ret;
> +}
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 596be4d..7e4a464 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1750,6 +1750,9 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def);
>  void virDomainHubDefFree(virDomainHubDefPtr def);
>  void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def);
>  void virDomainDeviceDefFree(virDomainDeviceDefPtr def);
> +virDomainDeviceDefPtr virDomainDeviceDefCopy(virCapsPtr caps,
> +                                             const virDomainDefPtr def,
> +                                             virDomainDeviceDefPtr src);
>  int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
>                                    int type);
>  int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a104e70..53f27e7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -286,6 +286,7 @@ virDomainDeviceAddressIsValid;
>  virDomainDeviceAddressPciMultiTypeFromString;
>  virDomainDeviceAddressPciMultiTypeToString;
>  virDomainDeviceAddressTypeToString;
> +virDomainDeviceDefCopy;
>  virDomainDeviceDefFree;
>  virDomainDeviceDefParse;
>  virDomainDeviceInfoIterate;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c6bdd29..3a52ded 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5561,8 +5561,9 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>      struct qemud_driver *driver = dom->conn->privateData;
>      virDomainObjPtr vm = NULL;
>      virDomainDefPtr vmdef = NULL;
> -    virDomainDeviceDefPtr dev = NULL;
> +    virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
>      bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
> +    bool free_dev_copy = false;
>      int ret = -1;
>      unsigned int affect;
>  
> @@ -5608,12 +5609,24 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>           goto endjob;
>      }
>  
> -    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> -        dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
> -                                      VIR_DOMAIN_XML_INACTIVE);
> -        if (dev == NULL)
> +    dev = dev_copy = virDomainDeviceDefParse(driver->caps, vm->def, xml,
> +                                             VIR_DOMAIN_XML_INACTIVE);
> +    if (dev == NULL)
> +        goto endjob;
> +
> +    if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> +        flags & VIR_DOMAIN_AFFECT_LIVE) {
> +        /* If we are affecting both CONFIG and LIVE
> +         * create a deep copy of device as adding
> +         * to CONFIG takes one instance.
> +         */
> +        dev_copy = virDomainDeviceDefCopy(driver->caps, vm->def, dev);
> +        if (!dev_copy)
>              goto endjob;
> +        free_dev_copy = true;
> +    }
>  
> +    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>          /* Make a copy for updated domain. */
>          vmdef = virDomainObjCopyPersistentDef(driver->caps, vm);
>          if (!vmdef)
> @@ -5639,24 +5652,15 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>      }
>  
>      if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> -        /* If dev exists it was created to modify the domain config. Free it. */
> -        virDomainDeviceDefFree(dev);
> -        dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
> -                                      VIR_DOMAIN_XML_INACTIVE);
> -        if (dev == NULL) {
> -            ret = -1;
> -            goto endjob;
> -        }
> -
>          switch (action) {
>          case QEMU_DEVICE_ATTACH:
> -            ret = qemuDomainAttachDeviceLive(vm, dev, dom);
> +            ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom);
>              break;
>          case QEMU_DEVICE_DETACH:
> -            ret = qemuDomainDetachDeviceLive(vm, dev, dom);
> +            ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom);
>              break;
>          case QEMU_DEVICE_UPDATE:
> -            ret = qemuDomainUpdateDeviceLive(vm, dev, dom, force);
> +            ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force);
>              break;
>          default:
>              qemuReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -5694,6 +5698,8 @@ endjob:
>  cleanup:
>      virDomainDefFree(vmdef);
>      virDomainDeviceDefFree(dev);
> +    if (free_dev_copy)
> +        virDomainDeviceDefFree(dev_copy);
>      if (vm)
>          virDomainObjUnlock(vm);
>      qemuDriverUnlock(driver);


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