On Mon, Apr 13, 2015 at 02:03:57PM -0400, John Ferlan wrote:
>
>
> On 04/04/2015 01:16 PM, Ján Tomko wrote:
> > Only for devices that have an alias.
> > ---
> > src/qemu/qemu_driver.c | 17 ++++++++++++++++-
> > src/qemu/qemu_hotplug.c | 5 +++++
> > 2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 6132674..c13f22b 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -7586,17 +7586,20 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> > {
> > virQEMUDriverPtr driver = dom->conn->privateData;
> > int ret = -1;
> > + const char *alias = NULL;
> >
> > switch ((virDomainDeviceType) dev->type) {
> > case VIR_DOMAIN_DEVICE_DISK:
> > qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1);
> > ret = qemuDomainAttachDeviceDiskLive(dom->conn, driver, vm, dev);
> > + alias = dev->data.disk->info.alias;
> > if (!ret)
> > dev->data.disk = NULL;
> > break;
> >
> > case VIR_DOMAIN_DEVICE_CONTROLLER:
> > ret = qemuDomainAttachDeviceControllerLive(driver, vm, dev);
> > + alias = dev->data.controller->info.alias;
>
> The one concern I'd have for all of these is - if (ret != 0) - is there
> any path that free's anything along the way that you're using a pointer
> in the alias fetching?
All the functions except AttachMemory should only add the device to the
domain definition right after setting ret to 0, so if ret == -1, it
should still be owned by the caller.
>
> Additionally of course, since the only way to print the alias is if (ret
> == 0) later, one could point out that setting it when ret != 0 is
> pointless; however, at least if ret == 0, you should be able to assume
> no one as deleted the alias!
>
Actually, this is wrong. When ret == 0, the device is already a part of
the domain definition. And the domain object is unlocked when we enter
the monitor in qemuDomainUpdateDeviceList.
So the Event should be queued before the call to UpdateDeviceList, or a
local copy of the alias is needed. Either way, another version of this
patch is needed.
> Perhaps it's best to only get the alias if (!ret)
>
> Your call if you want to add a "note" for case VIR_DOMAIN_DEVICE_MEMORY
> that the event is elicited inside the call since the call consumes
> dev->data.memory and hence the alias.
Good idea.
Jan
>
> I think with the alias setting inside !ret I'd feel comfortable giving
> an ACK - although I suspect in the other case it's not deleted until
> after this call exits
>
> John
>
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list