Re: [libvirt] [PATCHv2 03/14] Fix vmdef usage after domain crash in monitor on device removal

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...