[libvirt] [PATCHv3 11/12] qemu: Implement memory device hotplug

[John Ferlan]

On 03/17/2015 10:20 AM, Peter Krempa wrote:
> Add code to hot-add memory devices to running qemu instances.
> ---
> 
> Notes:
>     Version 3:
>     - added comment to clarify that @mem is always consumed by qemuDomainAttachMemory
>     Version 2:
>     - no change
>     
>     Version 2:
>     - no change
> 
>  src/qemu/qemu_command.c |  4 +--
>  src/qemu/qemu_command.h | 15 ++++++++
>  src/qemu/qemu_driver.c  |  5 ++-
>  src/qemu/qemu_hotplug.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_hotplug.h |  3 ++
>  5 files changed, 119 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2d85567..1f72437 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4599,7 +4599,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
>   * other configuration was used (to detect legacy configurations). Returns
>   * -1 in case of an error.
>   */
> -static int
> +int
>  qemuBuildMemoryBackendStr(unsigned long long size,
>                            unsigned long long pagesize,
>                            int guestNode,
> @@ -4872,7 +4872,7 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
>  }
> 
> 
> -static char *
> +char *
>  qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
>                           virQEMUCapsPtr qemuCaps)
>  {
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index ee81f92..a29db41 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -162,6 +162,21 @@ char *qemuBuildSoundDevStr(virDomainDefPtr domainDef,
>                             virDomainSoundDefPtr sound,
>                             virQEMUCapsPtr qemuCaps);
> 
> +int qemuBuildMemoryBackendStr(unsigned long long size,
> +                              unsigned long long pagesize,
> +                              int guestNode,
> +                              virBitmapPtr userNodeset,
> +                              virBitmapPtr autoNodeset,
> +                              virDomainDefPtr def,
> +                              virQEMUCapsPtr qemuCaps,
> +                              virQEMUDriverConfigPtr cfg,
> +                              const char **backendType,
> +                              virJSONValuePtr *backendProps,
> +                              bool force);
> +
> +char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
> +                               virQEMUCapsPtr qemuCaps);
> +
>  /* Legacy, pre device support */
>  char *qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev,
>                                     virQEMUCapsPtr qemuCaps);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e948cca..cbdf279 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7649,8 +7649,11 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>              dev->data.rng = NULL;
>          break;
> 
> -    /*TODO: implement later */
>      case VIR_DOMAIN_DEVICE_MEMORY:
> +        ret = qemuDomainAttachMemory(driver, vm,
> +                                     dev->data.memory);
> +        dev->data.memory = NULL;
> +        break;

FWIW: Although there is a note in the AttachMemory code about
consumption it may be worth noting here too just so no one in the future
"sees" it missing and adds it thinking it's necessary

> 
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_FS:
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 5c5ad0e..88c5e3c 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1672,6 +1672,101 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
>  }
> 
> 
> +/**
> + * qemuDomainAttachMemory:
> + * @driver: qemu driver data
> + * @vm: VM object
> + * @mem: Definition of the memory device to be attached. @mem is always consumed
> + *
> + * Attaches memory device described by @mem to domain @vm.
> + *
> + * Returns 0 on success -1 on error.
> + */
> +int
> +qemuDomainAttachMemory(virQEMUDriverPtr driver,
> +                       virDomainObjPtr vm,
> +                       virDomainMemoryDefPtr mem)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    char *devstr = NULL;
> +    char *objalias = NULL;
> +    const char *backendType;
> +    virJSONValuePtr props = NULL;
> +    int id;
> +    int ret = -1;
> +
> +    if (virAsprintf(&mem->info.alias, "dimm%zu", vm->def->nmems) < 0)
> +        goto cleanup;
> +
> +    if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0)
> +        goto cleanup;
> +
> +    if (!(devstr = qemuBuildMemoryDeviceStr(mem, priv->qemuCaps)))
> +        goto cleanup;
> +
> +    qemuDomainMemoryDeviceAlignSize(mem);
> +
> +    if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize,
> +                                  mem->targetNode, mem->sourceNodes, NULL,
> +                                  vm->def, priv->qemuCaps, cfg,
> +                                  &backendType, &props, true) < 0)

Coverity determines that qemuBuildMemoryBackendStr can return props here
with a -1 return and thus leak props

That's because qemuBuildMemoryBackendStr sets the returned *backendProps
and sets the local props to NULL before the (!hugepages) code which if
it fails won't cause 'props' to be free'd properly

Adding the virJSONValueFree(props); makes Coverity happy again.

John
> +        goto cleanup;
> +
> +    if (virDomainMemoryInsert(vm->def, mem) < 0) {
> +        virJSONValueFree(props);
> +        goto cleanup;
> +    }
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    if (qemuMonitorAddObject(priv->mon, backendType, objalias, props) < 0)
> +        goto removedef;
> +
> +    if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
> +        virErrorPtr err = virSaveLastError();
> +        ignore_value(qemuMonitorDelObject(priv->mon, objalias));
> +        virSetError(err);
> +        virFreeError(err);
> +        goto removedef;
> +    }
> +
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> +        /* we shouldn't touch mem now, as the def might be freed */
> +        mem = NULL;
> +        goto cleanup;
> +    }
> +
> +    /* mem is consumed by vm->def */
> +    mem = NULL;
> +
> +    /* this step is best effort, removing the device would be so much trouble */
> +    ignore_value(qemuDomainUpdateMemoryDeviceInfo(driver, vm,
> +                                                  QEMU_ASYNC_JOB_NONE));
> +
> +    ret = 0;
> +
> + cleanup:
> +    virObjectUnref(cfg);
> +    VIR_FREE(devstr);
> +    VIR_FREE(objalias);
> +    virDomainMemoryDefFree(mem);
> +    return ret;
> +
> + removedef:
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> +        mem = NULL;
> +        goto cleanup;
> +    }
> +
> +    if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
> +        mem = virDomainMemoryRemove(vm->def, id);
> +    else
> +        mem = NULL;
> +
> +    goto cleanup;
> +}
> +
> +
>  static int
>  qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
>                                virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 8a0b313..ad4ff38 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -57,6 +57,9 @@ int qemuDomainAttachHostDevice(virConnectPtr conn,
>                                 virDomainHostdevDefPtr hostdev);
>  int qemuDomainFindGraphicsIndex(virDomainDefPtr def,
>                                  virDomainGraphicsDefPtr dev);
> +int qemuDomainAttachMemory(virQEMUDriverPtr driver,
> +                           virDomainObjPtr vm,
> +                           virDomainMemoryDefPtr mem);
>  int qemuDomainChangeGraphics(virQEMUDriverPtr driver,
>                               virDomainObjPtr vm,
>                               virDomainGraphicsDefPtr dev);
>