[libvirt] [PATCH 2/2] Emit VIR_DOMAIN_EVENT_ID_DEVICE_ADDED in the QEMU driver

[Ján Tomko]

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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150414/dd341149/attachment-0001.sig>