On Fri, Mar 13, 2015 at 20:48:39 -0400, John Ferlan wrote:
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_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)
No as qemuDomainAttachMemory always consumes the memory device definition. I'll add a comment in the next version to clarify that.
+ 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...
I'll fixed that as a separate patch that will be part of the next posting.
+ 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...
It's always consumed, either freed or added to the definition. In some cases it's necessary to track it separately as the @vm was unlocked during monitor access. Peter