On 03/04/2015 11:25 AM, Peter Krempa wrote:
> Add code to hot-add memory devices to running qemu instances.
> ---
> src/qemu/qemu_command.c | 4 +--
> src/qemu/qemu_command.h | 15 +++++++++
> src/qemu/qemu_driver.c | 5 ++-
> src/qemu/qemu_hotplug.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_hotplug.h | 3 ++
> 5 files changed, 109 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f1fca02..883c3d1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4546,7 +4546,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,
> @@ -4819,7 +4819,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 6b10e96..b917d76 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7095,8 +7095,11 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> dev->data.rng = NULL;
> break;
>
> - /*TODO: implement later */
> case VIR_DOMAIN_DEVICE_MEMORY:
> + ret = qemuDomainAttachMemory(driver, vm,
> + dev->data.memory);
Shouldn't there be a :
if (!ret)
prior to the following line (like the other cases in the switch)
> + dev->data.memory = NULL;
> + break;
>
> 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 cb2270e..967b678 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1672,6 +1672,91 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
> }
>
>
> +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)
> + goto cleanup;
> +
> + if (virDomainMemoryInsert(vm->def, mem) < 0) {
> + virJSONValueFree(props);
> + goto cleanup;
> + }
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> + if (qemuMonitorAddObject(priv->mon, backendType, objalias, props) < 0)
I see from the AddObject comments, props is consumed upon success, but
if we hit the else of mon->json, then we don't free it which we should -
not related to this particular code, but something that should be fixed...
> + 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;
Since both the Exit error path and this path set mem = NULL, why not
just set it before the Exit and comment that it'll either consumed by
vm->def on success or might be freed on failure...
> +
> + /* this step is best effort, removing the device would be so much trouble */
> + ignore_value(qemuDomainUpdateMemoryDeviceInfo(driver, vm,
> + QEMU_ASYNC_JOB_NONE));
> +
BTW: This is perhaps what I was thinking of in that migration path patch
(#6) where the code fails if not defined.
ACK - with the slight adjustment above for the first thing I noted.
John
> + 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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list