Re: [libvirt] [PATCHv2.5 09/10] qemu: Implement memory device hotplug

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)

> + 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...