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

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?

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!

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.

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