[libvirt] [PATCHv2 03/14] Fix vmdef usage after domain crash in monitor on device removal
John Ferlan
jferlan at redhat.com
Mon Jan 12 21:44:53 UTC 2015
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++;
> }
> }
>
More information about the libvir-list
mailing list