On 01/07/2015 10:42 AM, Ján Tomko wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1161024 > > In the device type-specific functions, exit early > if the domain has disappeared, because the cleanup > should have been done by qemuProcessStop. > > Check the return value in processDeviceDeletedEvent > and qemuProcessUpdateDevices. > --- > src/qemu/qemu_driver.c | 3 ++- > src/qemu/qemu_hotplug.c | 32 ++++++++++++++++++++------------ > src/qemu/qemu_hotplug.h | 6 +++--- > src/qemu/qemu_process.c | 6 ++++-- > 4 files changed, 29 insertions(+), 18 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index cdf4173..f7c9219 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -4044,7 +4044,8 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver, > if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0) > goto endjob; > > - qemuDomainRemoveDevice(driver, vm, &dev); > + if (qemuDomainRemoveDevice(driver, vm, &dev) < 0) > + goto endjob; > > if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) > VIR_WARN("unable to save domain status after removing device %s", > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 1714341..ce05f80 100644
The following generally applies to patches 3-5... For hotplug, the ExitMonitor failures are done prior to auditing or sending events. Although not all the code handles failures and auditing quite the same - I would think we still want to Audit the qemuMonitor* calls regardless of whether the process dies or not. The audit code uses vm->def->{uuid|name|virtType} to format the message it generates, so is that "safe" according to what happens on the ExitMonitor failure? One reason this springs to mind is some of the code will Audit on the qemuMonitor* call success/failure and I would think perhaps one of the failures is that the vm died and having that Audit trail could be a good thing. Beyond that knowing that the qemuMonitor* call passed (because Audit told us so), but then finding the vm crashed (because ExitMonitor told us so) could narrow some other bizarre window. So while perhaps slightly outside the scope of these changes - what affect does exiting prior to Audit really have... and yes, event is different can of worms. As I go deeper into the patches I see the HotplugVcpus will call virDomainAuditVcpu even after the ignored ExitMonitor, so it seems safe... > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -2491,8 +2491,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, > > qemuDomainObjEnterMonitor(driver, vm); > qemuMonitorDriveDel(priv->mon, drivestr); > - qemuDomainObjExitMonitor(driver, vm); > VIR_FREE(drivestr); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + return -1; > > virDomainAuditDisk(vm, disk->src, NULL, "detach", true); Missed audit call... I think the last parameter should true/false based on whether DriveDel succeeds, right? Although I do have a recollection of some interaction with DelDevice while working through the hostdev changes. > > @@ -2607,7 +2608,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, > > qemuDomainObjEnterMonitor(driver, vm); > qemuMonitorDriveDel(priv->mon, drivestr); > - qemuDomainObjExitMonitor(driver, vm); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + goto cleanup; > } hmm... the Audit call quite a few lines later and it assumes success. > > event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias); > @@ -2701,7 +2703,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && > virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { > - qemuDomainObjExitMonitor(driver, vm); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + goto cleanup; > virDomainAuditNet(vm, net, NULL, "detach", false); > goto cleanup; > } > @@ -2713,12 +2716,14 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, > virReportError(VIR_ERR_OPERATION_FAILED, "%s", > _("unable to determine original VLAN")); > } > - qemuDomainObjExitMonitor(driver, vm); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + goto cleanup; NOTE: There's a virReportError before us in this path... > virDomainAuditNet(vm, net, NULL, "detach", false); > goto cleanup; > } > } > - qemuDomainObjExitMonitor(driver, vm); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + goto cleanup; > For each of these 3 changes - the AuditNet will not occur... > virDomainAuditNet(vm, net, NULL, "detach", true); > > @@ -2788,7 +2793,8 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, > > qemuDomainObjEnterMonitor(driver, vm); > rc = qemuMonitorDetachCharDev(priv->mon, charAlias); > - qemuDomainObjExitMonitor(driver, vm); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + goto cleanup; > Missed Audit call Weak ACK for what's here and if it's felt Auditing isn't necessary, then I have no other objections... John > virDomainAuditChardev(vm, chr, NULL, "detach", rc == 0); > > @@ -2809,27 +2815,28 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, > } > > > -void > +int > qemuDomainRemoveDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virDomainDeviceDefPtr dev) > { > + int ret = -1; > switch ((virDomainDeviceType) dev->type) { > case VIR_DOMAIN_DEVICE_DISK: > - qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk); > + ret = qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk); > break; > case VIR_DOMAIN_DEVICE_CONTROLLER: > - qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller); > + ret = qemuDomainRemoveControllerDevice(driver, vm, > dev->data.controller); > break; > case VIR_DOMAIN_DEVICE_NET: > - qemuDomainRemoveNetDevice(driver, vm, dev->data.net); > + ret = qemuDomainRemoveNetDevice(driver, vm, dev->data.net); > break; > case VIR_DOMAIN_DEVICE_HOSTDEV: > - qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev); > + ret = qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev); > break; > > case VIR_DOMAIN_DEVICE_CHR: > - qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); > + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); > break; > > case VIR_DOMAIN_DEVICE_NONE: > @@ -2855,6 +2862,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, > virDomainDeviceTypeToString(dev->type)); > break; > } > + return ret; > } > > > diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h > index d13c532..19ab9a0 100644 > --- a/src/qemu/qemu_hotplug.h > +++ b/src/qemu/qemu_hotplug.h > @@ -107,9 +107,9 @@ virDomainChrDefPtr > qemuDomainChrRemove(virDomainDefPtr vmdef, > virDomainChrDefPtr chr); > > -void qemuDomainRemoveDevice(virQEMUDriverPtr driver, > - virDomainObjPtr vm, > - virDomainDeviceDefPtr dev); > +int qemuDomainRemoveDevice(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainDeviceDefPtr dev); > > bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, > const char *devAlias); > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index c18204b..4528b58 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3547,8 +3547,10 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver, > if ((tmp = old)) { > while (*tmp) { > if (!virStringArrayHasString(priv->qemuDevices, *tmp) && > - virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0) > - qemuDomainRemoveDevice(driver, vm, &dev); > + virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0 && > + qemuDomainRemoveDevice(driver, vm, &dev) < 0) { > + goto cleanup; > + } > tmp++; > } > } > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list