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

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

 



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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]