On 01/12/2015 08:02 PM, John Ferlan wrote:

On 01/07/2015 10:42 AM, Ján Tomko wrote:

...

The domain might disappear during the time in monitor when the virDomainObjPtr is unlocked, so the caller needs to check if it's still alive.

Since most of the callers are going to need it, put the check inside qemuDomainObjExitMonitor and return -1 if the domain died in the meantime. --- src/qemu/THREADS.txt | 5 +++++ src/qemu/qemu_domain.c | 16 ++++++++++++++-- src/qemu/qemu_domain.h | 4 ++-- 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt index add2a35..dfa372b 100644 --- a/src/qemu/THREADS.txt +++ b/src/qemu/THREADS.txt @@ -160,6 +160,11 @@ To acquire the QEMU monitor lock - Acquires the virDomainObjPtr lock

These functions must not be used by an asynchronous job. + Note that the virDomainObj is unlocked during the time in + monitor and it can be changed, e.g. if QEMU dies, qemuProcessStop + may free the live domain definition and put the persistent + definition back in vm->def. The callers should check the return + value of ExitMonitor to see if the domain is still alive.

To acquire the QEMU monitor lock as part of an asynchronous job diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d4023c..c942b02 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1586,11 +1586,23 @@ void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver, /* obj must NOT be locked before calling * * Should be paired with an earlier qemuDomainObjEnterMonitor() call + * + * Returns -1 if the domain is no longer alive after exiting the monitor. + * In that case, the caller should be careful when using obj's data, + * e.g. the live definition in vm->def has been freed by qemuProcessStop + * and replaced by the persistent definition, so pointers stolen + * from the live definition could no longer be valid. */ -void qemuDomainObjExitMonitor(virQEMUDriverPtr driver, - virDomainObjPtr obj) +int qemuDomainObjExitMonitor(virQEMUDriverPtr driver, + virDomainObjPtr obj) { qemuDomainObjExitMonitorInternal(driver, obj); + if (!virDomainObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("domain is no longer running")); + return -1; + } + return 0;

Do we have to worry about caller paths perhaps using virReportError and this overwriting the other message for the last message? Does it really matter?

Well, we'd overwrite 'end of file from monitor' by 'domain is no longer running' and personally I don't think it matters. Jan

ACK

John

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++; } }

On 01/12/2015 10:58 PM, John Ferlan wrote:

On 01/07/2015 10:42 AM, Ján Tomko wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1161024

Skip audit and removing the device from live def because it has already been cleaned up. --- src/qemu/qemu_hotplug.c | 59 ++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 23 deletions(-)

Similar Audit concerns w/ 3/14...

...

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ce05f80..c480dcd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2986,19 +2986,22 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } } else { if (qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci) < 0) { - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) @@ -3038,11 +3041,13 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,

qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) @@ -3219,17 +3224,20 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; goto cleanup; } } else { if (qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci) < 0) { - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; goto cleanup; } } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) @@ -3274,7 +3282,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver, } else { ret = qemuMonitorRemovePCIDevice(priv->mon, &detach->info->addr.pci); } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

return ret;

Or just ret qemuDomainObjExitMonitor(driver, vm);

That would return 0 in the case of RemovePCIDevice failing, but not crashing the domain.

...

@@ -3374,7 +3381,8 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, }

if (ret < 0) { - virDomainAuditHostdev(vm, detach, "detach", false); + if (virDomainObjIsActive(vm)) + virDomainAuditHostdev(vm, detach, "detach", false);

Hmm.... well this makes my comments in 3/14 appear to be unnecessary, since there's an explicit check for active vm... Although the Vcpu failure will still call virDomainAuditVcpu

'detach' here is a pointer to the device in the vm->def, so it could be freed in the meantime if the domain crashed. Jan

On 01/07/2015 10:42 AM, Ján Tomko wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1161024

If the domain crashed while we were in monitor, we cannot rely on the REALLOC done on live definition, since vm->def now points to the persistent definition. Skip adding the attached devices to domain definition if the domain crashed.

In AttachChrDevice, the chardev was already added to the live definition and freed by qemuProcessStop in the case of a crash. Skip the device removal in that case. --- src/qemu/qemu_hotplug.c | 54 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c480dcd..a4e4d6b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -391,7 +391,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); } } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + releaseaddr = false; + ret = -1; + }

So here's another case where will will do the Audit even though ExitMonitor fails - seems to contract the check in 4/14...

virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);

@@ -477,7 +480,10 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, type, &controller->info.addr.pci); } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + releaseaddr = false; + ret = -1; + }

No Audit for AttachController, but detach and remove don't have one either...

if (ret == 0) { if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) @@ -628,7 +634,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, disk->info.addr.drive.unit = driveAddr.unit; } } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

Again here we do the Audit...

virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);

@@ -708,7 +715,8 @@ qemuDomainAttachUSBMassstorageDevice(virConnectPtr conn, } else { ret = qemuMonitorAddUSBDisk(priv->mon, src); } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

Again the Audit is done

virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);

@@ -1272,7 +1280,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, configfd, configfd_name); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

after the else is the Audit

} else { virDevicePCIAddressPtr guestAddr = &hostdev->info->addr.pci; virDevicePCIAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr; @@ -1288,7 +1297,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorAddPCIHostDevice(priv->mon, hostAddr, guestAddr); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

same function, same Audit

hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } @@ -1352,12 +1362,11 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, goto error;

qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) - ret = qemuMonitorAddDevice(priv->mon, devstr); - else - goto error; + ret = qemuMonitorAddDevice(priv->mon, devstr); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

Here we Audit again

- qemuDomainObjExitMonitor(driver, vm); virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0); if (ret < 0) goto error; @@ -1479,17 +1488,22 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,

qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + need_remove = false; goto audit;

Auditing here too.

}

if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { /* detach associated chardev on error */ qemuMonitorDetachCharDev(priv->mon, charAlias); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + need_remove = false; + goto audit; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + need_remove = false; goto audit;

Same...

} - qemuDomainObjExitMonitor(driver, vm);

ret = 0; audit: @@ -1545,7 +1559,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, ret = qemuMonitorAddUSBDeviceExact(priv->mon, hostdev->source.subsys.u.usb.bus, hostdev->source.subsys.u.usb.device); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

Same

virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) goto cleanup; @@ -1648,7 +1663,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, } } } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

Same John

virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) @@ -1848,7 +1864,8 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver, dev->linkstate = linkstate;

cleanup: - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1;

return ret; } @@ -3633,7 +3650,8 @@ qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver, }

end_job: - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; cleanup: virObjectUnref(cfg); return ret;

On 01/07/2015 10:42 AM, Ján Tomko wrote:

Exit the monitor right after we've done with it to get the virDomainObjPtr lock back, otherwise we might be accessing vm->def while it's being cleaned up by qemuProcessStop.

If the domain crashed while we were in the monitor, exit early instead of changing vm->def which is now the persistent definition. --- src/qemu/qemu_driver.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f7c9219..1275ba4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4367,7 +4367,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, if (rc == 0) goto unsupported; if (rc < 0) - goto cleanup; + goto exit_monitor;

vcpus++; } @@ -4378,7 +4378,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, if (rc == 0) goto unsupported; if (rc < 0) - goto cleanup; + goto exit_monitor;

vcpus--; } @@ -4395,6 +4395,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, * fatal */ if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) { virResetLastError(); + goto exit_monitor; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; goto cleanup; }

@@ -4515,10 +4519,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, cpupids = NULL;

cleanup: - qemuDomainObjExitMonitor(driver, vm); - vm->def->vcpus = vcpus; VIR_FREE(cpupids); VIR_FREE(mem_mask); + if (virDomainObjIsActive(vm)) + vm->def->vcpus = vcpus; virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);

NOTE: We'll audit regardless of ExitMonitor status here. ACK in general, but the Audit stuff needs to be handled in the same manner as other calls. John

if (cgroup_vcpu) virCgroupFree(&cgroup_vcpu); @@ -4527,6 +4531,8 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, unsupported: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change vcpu count of this domain")); + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; }

<No commit message>? Seems we're fixing two issues here #1. The ExitMonitor issue #2. Somehow the disk->info.alias becomes NULL and thus we're making a local copy to avoid that. On 01/07/2015 10:42 AM, Ján Tomko wrote:

--- src/qemu/qemu_driver.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1275ba4..30c9017 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10114,6 +10114,7 @@ qemuDomainBlockStats(virDomainPtr dom, virDomainObjPtr vm; virDomainDiskDefPtr disk = NULL; qemuDomainObjPrivatePtr priv; + char *diskAlias = NULL;

if (!*path) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -10149,11 +10150,14 @@ qemuDomainBlockStats(virDomainPtr dom, goto endjob; }

+ if (VIR_STRDUP(diskAlias, disk->info.alias) < 0) + goto endjob; + priv = vm->privateData;

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockStatsInfo(priv->mon, - disk->info.alias, + diskAlias, &stats->rd_req, &stats->rd_bytes, NULL, @@ -10163,13 +10167,15 @@ qemuDomainBlockStats(virDomainPtr dom, NULL, NULL, &stats->errs); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

endjob: qemuDomainObjEndJob(driver, vm);

cleanup: qemuDomObjEndAPI(&vm); + VIR_FREE(diskAlias); return ret; }

@@ -10185,11 +10191,11 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, int idx; int tmp, ret = -1; virDomainObjPtr vm; - virDomainDiskDefPtr disk = NULL; qemuDomainObjPrivatePtr priv; long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times; long long wr_total_times, flush_req, flush_total_times, errs; virTypedParameterPtr param; + char *diskAlias = NULL;

virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);

@@ -10218,6 +10224,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, }

if (*nparams != 0) { + virDomainDiskDefPtr disk = NULL; if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); @@ -10231,6 +10238,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, disk->dst); goto endjob; } + if (VIR_STRDUP(diskAlias, disk->info.alias) < 0) + goto endjob;

so diskAlias is only set if *nparams != 0, but...

}

priv = vm->privateData; @@ -10241,12 +10250,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);

if (tmp == 0 || ret < 0) { - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm));

Can tmp == 0 and ret == 0? Thus when go to endjob and eventually exit we don't a failure? Does it matter? I suppose for consistency it does.

goto endjob; }

ret = qemuMonitorGetBlockStatsInfo(priv->mon, - disk->info.alias, + diskAlias,

...used here regardless of *nparams... Can we get here with diskAlias == NULL?

&rd_req, &rd_bytes, &rd_total_times, @@ -10257,7 +10266,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, &flush_total_times, &errs);

- qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

if (ret < 0) goto endjob; @@ -10342,6 +10352,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, qemuDomainObjEndJob(driver, vm);

cleanup: + VIR_FREE(diskAlias); qemuDomObjEndAPI(&vm); return ret; } @@ -16853,7 +16864,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

Potentially leaving ret = 0 from the IoThrottle call... Should just set ret = -1 (liek done in qemuDomainBlockStatsFlags above) ACK with a commit message and the issues resolved. John

if (ret < 0) goto endjob; vm->def->disks[idx]->blkdeviotune = info;

On 01/13/2015 12:54 AM, John Ferlan wrote:

On 01/07/2015 10:42 AM, Ján Tomko wrote:

...

Error out if the domain has disappeared in the meantime.

This prevents writing the persistent definition as the domain status and calling qemuDomainRemoveDevice on devices that already have been dealt with in qemuProcessStop. --- src/qemu/qemu_domain.c | 9 +++++---- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++------------------- 3 files changed, 26 insertions(+), 25 deletions(-)

...

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a4e4d6b..6b108bf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -193,7 +193,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

if "ret == 0" we get to cleanup... with ejected media, no Audit and the rest of this not done...

...

I've added ret = -1 in front of that. Both audit and qemuDomainPrepareDisk on the error path would need access to the disk that's now freed. Jan

<no commit message> On 01/07/2015 10:42 AM, Ján Tomko wrote:

--- src/qemu/qemu_migration.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 77e0b35..31494c8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1663,7 +1663,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm);

Missing the error check here (yes, it's in 11/14)

goto cleanup; } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; }

priv->nbdPort = port; @@ -1860,7 +1861,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, }

-static void +static int qemuMigrationStopNBDServer(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuMigrationCookiePtr mig) @@ -1868,22 +1869,23 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData;

if (!mig->nbd) - return; + return 0;

if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) - return; + return -1;

if (qemuMonitorNBDServerStop(priv->mon) < 0) VIR_WARN("Unable to stop NBD server");

- qemuDomainObjExitMonitor(driver, vm); -

Why not ret = qemuDomainObjExitMonitor(), then return ret later on... Holding the lock while virPortAllocatorRelease executes seems unnecessary...

virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); priv->nbdPort = 0; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + return 0; }

-static void +static int qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, virQEMUDriverPtr driver, virDomainObjPtr vm) @@ -1891,6 +1893,7 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, qemuDomainObjPrivatePtr priv = vm->privateData; size_t i; char *diskAlias = NULL; + int ret = 0;

VIR_DEBUG("mig=%p nbdPort=%d", mig->nbd, priv->nbdPort);

@@ -1914,12 +1917,15 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, BLOCK_JOB_ABORT, true) < 0) VIR_WARN("Unable to stop block job on %s", diskAlias); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto cleanup; + } }

cleanup: VIR_FREE(diskAlias); - return; + return ret; }

/* Validate whether the domain is safe to migrate. If vm is NULL, @@ -2125,7 +2131,8 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver, state);

cleanup: - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

or ret = qemuDomainObjExitMonitor(); return ret; (your choice here and the next few) The rest seems OK... Might be nice to mention in a commit message why failure to cancel drive mirror is ignorable while failure to stop nbd server is not. ACK, but it might be nice to pull in part of 11/14 since you're changing ExitMonitor checks anyway John

return ret; }

@@ -2164,7 +2171,8 @@ qemuMigrationSetAutoConverge(virQEMUDriverPtr driver, state);

cleanup: - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; return ret; }

@@ -2210,7 +2218,8 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver, state);

cleanup: - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; return ret; }

@@ -2476,7 +2485,8 @@ qemuDomainMigrateGraphicsRelocate(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) { ret = qemuMonitorGraphicsRelocate(priv->mon, type, listenAddress, port, tlsPort, tlsSubject); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; }

cleanup: @@ -4000,7 +4010,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,

/* cancel any outstanding NBD jobs */ if (mig) - qemuMigrationCancelDriveMirror(mig, driver, vm); + ignore_value(qemuMigrationCancelDriveMirror(mig, driver, vm));

if (spec->fwdType != MIGRATION_FWD_DIRECT) { if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0) @@ -5142,7 +5152,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver, VIR_WARN("unable to provide network data for relocation"); }

- qemuMigrationStopNBDServer(driver, vm, mig); + if (qemuMigrationStopNBDServer(driver, vm, mig) < 0) + goto endjob;

if (flags & VIR_MIGRATE_PERSIST_DEST) { virDomainDefPtr vmdef;

<no commit message> On 01/07/2015 10:42 AM, Ján Tomko wrote:

--- src/qemu/qemu_process.c | 102 +++++++++++++++++++++++++----------------------- 1 file changed, 53 insertions(+), 49 deletions(-)

Hmm.. miscounted in a couple of comments in previous changes where I reference 11/14 - guess that's 12/14 <sigh> it's late.

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3d383ef..e6c5bc6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -571,7 +571,7 @@ qemuProcessFakeReboot(void *opaque) virObjectEventPtr event = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainRunningReason reason = VIR_DOMAIN_RUNNING_BOOTED; - int ret = -1; + int ret = -1, rc;

VIR_DEBUG("vm=%p", vm); virObjectLock(vm); @@ -585,17 +585,13 @@ qemuProcessFakeReboot(void *opaque) }

qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorSystemReset(priv->mon) < 0) { - qemuDomainObjExitMonitor(driver, vm); + rc = qemuMonitorSystemReset(priv->mon); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto endjob; - } - qemuDomainObjExitMonitor(driver, vm);

- if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); + if (rc < 0) goto endjob; - }

if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_CRASHED) reason = VIR_DOMAIN_RUNNING_CRASHED; @@ -1662,7 +1658,8 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, if (ret == 0 && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON)) ret = virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1;

error:

@@ -2118,7 +2115,8 @@ qemuProcessReconnectRefreshChannelVirtioState(virQEMUDriverPtr driver,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetChardevInfo(priv->mon, &info); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; ^^ But if ret == 0, then we may get erroneous report of success.

if (ret < 0) goto cleanup; @@ -2171,7 +2169,8 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; ret = qemuMonitorGetChardevInfo(priv->mon, &info); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

If ret == 0, then could we get erroneous report of success?

VIR_DEBUG("qemuMonitorGetChardevInfo returned %i", ret);

NIT: This could move up a couple lines right after the call...

if (ret == 0) { @@ -2264,18 +2263,19 @@ qemuProcessDetectVcpuPIDs(virQEMUDriverPtr driver,

if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; + ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; /* failure to get the VCPU<-> PID mapping or to execute the query * command will not be treated fatal as some versions of qemu don't * support this command */ - if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) { - qemuDomainObjExitMonitor(driver, vm); + if (ncpupids <= 0) { virResetLastError();

priv->nvcpupids = 0; priv->vcpupids = NULL; return 0; } - qemuDomainObjExitMonitor(driver, vm);

if (ncpupids != vm->def->vcpus) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2310,7 +2310,8 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; niothreads = qemuMonitorGetIOThreads(priv->mon, &iothreads); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; if (niothreads < 0) goto cleanup;

@@ -3026,11 +3027,13 @@ qemuProcessInitPCIAddresses(virQEMUDriverPtr driver, return -1; naddrs = qemuMonitorGetAllPCIAddresses(priv->mon, &addrs); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

if (naddrs > 0) ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs);

+ cleanup: VIR_FREE(addrs);

return ret; @@ -3180,7 +3183,8 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, goto release;

ret = qemuMonitorStartCPUs(priv->mon, conn); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto release;

if ret == 0, then we spew that nasty message but return success.

if (ret < 0) goto release; @@ -3213,7 +3217,8 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver, goto cleanup;

ret = qemuMonitorStopCPUs(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

If ret == 0, then we return success which is wrong. The rest seems reasonable ACK with changes John

if (ret < 0) goto cleanup; @@ -3282,9 +3287,10 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm)

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetStatus(priv->mon, &running, &reason); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1;

- if (ret < 0 || !virDomainObjIsActive(vm)) + if (ret < 0) return -1;

state = virDomainObjGetState(vm, NULL); @@ -3396,7 +3402,8 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, vm->def->name); qemuDomainObjEnterMonitor(driver, vm); ignore_value(qemuMonitorMigrateCancel(priv->mon)); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; /* resume the domain but only if it was paused as a result of * migration */ if (state == VIR_DOMAIN_PAUSED && @@ -3465,7 +3472,8 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver, case QEMU_ASYNC_JOB_SNAPSHOT: qemuDomainObjEnterMonitor(driver, vm); ignore_value(qemuMonitorMigrateCancel(priv->mon)); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; /* resume the domain but only if it was paused as a result of * running a migration-to-file operation. Although we are * recovering an async job, this function is run at startup @@ -3990,7 +3998,8 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return false; rc = qemuMonitorGetGuestCPU(priv->mon, arch, &guestcpu); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return false;

if (rc < 0) { if (rc == -2) @@ -4777,12 +4786,10 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Setting network link states"); if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; - if (qemuProcessSetLinkStates(vm) < 0) { - qemuDomainObjExitMonitor(driver, vm); + if (qemuProcessSetLinkStates(vm) < 0) + goto exit_monitor; + if (qemuDomainObjExitMonitor(driver, vm)) goto cleanup; - } - - qemuDomainObjExitMonitor(driver, vm);

VIR_DEBUG("Fetching list of active devices"); if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0) @@ -4806,10 +4813,8 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; if (period) qemuMonitorSetMemoryStatsPeriod(priv->mon, period); - if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; - } + if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) + goto exit_monitor; if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;

@@ -4882,6 +4887,10 @@ int qemuProcessStart(virConnectPtr conn, virObjectUnref(caps);

return -1; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; }

@@ -5388,21 +5397,13 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,

VIR_DEBUG("Getting initial memory amount"); qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorGetBalloonInfo(priv->mon, &vm->def->mem.cur_balloon) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto error; - } - if (qemuMonitorGetStatus(priv->mon, &running, &reason) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto error; - } - if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto error; - } - qemuDomainObjExitMonitor(driver, vm); - - if (!virDomainObjIsActive(vm)) + if (qemuMonitorGetBalloonInfo(priv->mon, &vm->def->mem.cur_balloon) < 0) + goto exit_monitor; + if (qemuMonitorGetStatus(priv->mon, &running, &reason) < 0) + goto exit_monitor; + if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0) + goto exit_monitor; + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto error;

if (running) { @@ -5412,7 +5413,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, qemuDomainObjEnterMonitor(driver, vm); qemuMonitorSetMemoryStatsPeriod(priv->mon, vm->def->memballoon->period); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto error; } } else { virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason); @@ -5447,6 +5449,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,

return 0;

+ exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); error: /* We jump here if we failed to attach to the VM for any reason. * Leave the domain running, but pretend we never attempted to

<no commit message>? On 01/07/2015 10:42 AM, Ján Tomko wrote:

--- src/qemu/qemu_driver.c | 131 +++++++++++++++++++++++++++++----------------- src/qemu/qemu_migration.c | 98 +++++++++++++++------------------- 2 files changed, 125 insertions(+), 104 deletions(-)

Should qemu_migration move to 10/14?

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1781ea9..55d6fb3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1978,7 +1978,8 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags)

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSystemPowerdown(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; }

endjob: @@ -2073,7 +2074,8 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) } else { qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSystemPowerdown(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

if (ret == 0) qemuDomainSetFakeReboot(driver, vm, isReboot); @@ -2116,7 +2118,8 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags) priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSystemReset(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

priv->fakeReboot = false;

@@ -2333,7 +2336,8 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetBalloon(priv->mon, newmem); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

Here we have another Audit issue...

virDomainAuditMemory(vm, vm->def->mem.cur_balloon, newmem, "update", r == 1); if (r < 0) @@ -2415,7 +2419,8 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,

qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; if (r < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unable to set balloon driver collection period")); @@ -2479,7 +2484,8 @@ static int qemuDomainInjectNMI(virDomainPtr domain, unsigned int flags)

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorInjectNMI(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

if ret == 0, then @ cleanup we return success

endjob: qemuDomainObjEndJob(driver, vm);

Hmmm... do we need to call DomainObjEndJob if ExitMonitor fails? I don't even remember if this was an issue for previous such exits...

@@ -2541,7 +2547,8 @@ static int qemuDomainSendKey(virDomainPtr domain,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

again, if ret == 0, then we return wrong status possibly... Also I see another EndJob below...

endjob: qemuDomainObjEndJob(driver, vm); @@ -2596,7 +2603,10 @@ static int qemuDomainGetInfo(virDomainPtr dom, } else { qemuDomainObjEnterMonitor(driver, vm); err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + qemuDomainObjEndJob(driver, vm);

Well, I think I have my answer. You may want to go back through all the changes to see if any are missed...

+ goto cleanup; + } } qemuDomainObjEndJob(driver, vm);

@@ -3492,7 +3502,7 @@ static int qemuDumpToFd(virQEMUDriverPtr driver, virDomainObjPtr vm, ret = qemuMonitorDumpToFd(priv->mon, fd, dumpformat);

cleanup: - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm));

return ret; } @@ -3691,7 +3701,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSystemReset(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); }

if (resume && virDomainObjIsActive(vm)) { @@ -3788,10 +3798,11 @@ qemuDomainScreenshot(virDomainPtr dom,

qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorScreendump(priv->mon, tmp) < 0) { - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto endjob; } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

if (VIR_CLOSE(tmp_fd) < 0) { virReportSystemError(errno, _("unable to close %s"), tmp); @@ -4216,7 +4227,8 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &guestFilter); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

Even though it's a void function, I think instead of goto endjob, it should be ret = -1...

if (ret < 0) goto endjob;

@@ -6227,7 +6239,8 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom,

qemuDomainObjEnterMonitor(driver, vm); err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + err = -1;

endjob: qemuDomainObjEndJob(driver, vm); @@ -10083,10 +10096,11 @@ qemuDomainBlockResize(virDomainPtr dom,

qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorBlockResize(priv->mon, device, size) < 0) { - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto endjob; } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

ret = 0;

@@ -10740,7 +10754,8 @@ qemuDomainMemoryStats(virDomainPtr dom, qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

if (ret >= 0 && ret < nr_stats) { long rss; @@ -10874,16 +10889,17 @@ qemuDomainMemoryPeek(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); if (flags == VIR_MEMORY_VIRTUAL) { if (qemuMonitorSaveVirtualMemory(priv->mon, offset, size, tmp) < 0) { - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto endjob; } } else { if (qemuMonitorSavePhysicalMemory(priv->mon, offset, size, tmp) < 0) { - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto endjob; } } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

/* Read the memory file into buffer. */ if (saferead(fd, buffer, size) == (ssize_t) -1) { @@ -11121,7 +11137,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom, ret = qemuMonitorGetBlockExtent(priv->mon, disk->info.alias, &src->allocation); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; }

if (ret == 0) { @@ -12361,7 +12378,8 @@ static int qemuDomainAbortJob(virDomainPtr dom) qemuDomainObjAbortAsyncJob(vm); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorMigrateCancel(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

endjob: qemuDomainObjEndJob(driver, vm); @@ -12404,7 +12422,8 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom, VIR_DEBUG("Setting migration downtime to %llums", downtime); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetMigrationDowntime(priv->mon, downtime); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

endjob: qemuDomainObjEndJob(driver, vm); @@ -12457,7 +12476,8 @@ qemuDomainMigrateGetCompressionCache(virDomainPtr dom, ret = qemuMonitorGetMigrationCacheSize(priv->mon, cacheSize); }

- qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

endjob: qemuDomainObjEndJob(driver, vm); @@ -12511,7 +12531,8 @@ qemuDomainMigrateSetCompressionCache(virDomainPtr dom, ret = qemuMonitorSetMigrationCacheSize(priv->mon, cacheSize); }

- qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

endjob: qemuDomainObjEndJob(driver, vm); @@ -12561,7 +12582,8 @@ qemuDomainMigrateSetMaxSpeed(virDomainPtr dom, VIR_DEBUG("Setting migration bandwidth to %luMbs", bandwidth); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetMigrationSpeed(priv->mon, bandwidth); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

if (ret == 0) priv->migMaxBandwidth = bandwidth; @@ -15006,7 +15028,10 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorArbitraryCommand(priv->mon, cmd, result, hmp); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto endjob;

This goto seems superfluous... especially compared to other recent changes. Although seemingly correct, it's handled differently and it's so much easier when things are more consistent... Of course each of them could just have ret = qemuDomainobjExitMonitor() - or at least these inline cases where endjob: is almost always the next line.

+ }

endjob: qemuDomainObjEndJob(driver, vm); @@ -15328,14 +15353,10 @@ qemuDomainBlockPivot(virConnectPtr conn, if (!disk->mirrorState) { qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorBlockJobInfo(priv->mon, device, &info, NULL); - qemuDomainObjExitMonitor(driver, vm); - if (rc < 0) + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); + if (rc < 0) goto cleanup; - } if (rc == 1 && info.cur == info.end && info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; @@ -15412,7 +15433,8 @@ qemuDomainBlockPivot(virConnectPtr conn, disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path, format); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

In this case, if ret == 0, then we return success incorrectly. Perhaps we should set ret = qemuDomainObjExitMonitor here since the next if handles < 0, although after that there's a SaveStatus, which I wonder if we really want to do in the case where ret < 0.

if (ret < 0) { /* On failure, qemu abandons the mirror, and reverts back to @@ -15615,8 +15637,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, speed, mode, async); - qemuDomainObjExitMonitor(driver, vm); - if (ret < 0) { + if (qemuDomainObjExitMonitor(driver, vm) < 0 || + ret < 0) {

Again - if ret == 0, then what? FWIW: this is a construct I was going to suggest for many of the other failure checks where both would fall into an error path, until I started thinking about the ret == 0 and then Exit == failure case...

if (mode == BLOCK_JOB_ABORT && disk->mirror) disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; goto endjob; @@ -15660,7 +15682,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJobInfo(priv->mon, device, &dummy, NULL); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + break;

Here again, ret == 0 and we're breaking - although the next check makes me wonder... Furthermore about 10 lines or so dow there's a virDomainObjIsActive check - is that now not necessary like other places?

if (ret <= 0) break; @@ -15764,7 +15787,8 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJobInfo(priv->mon, device, info, &bandwidth); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

Another ret == 0 possibility

if (ret < 0) goto endjob;

@@ -15982,8 +16006,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format, bandwidth, granularity, buf_size, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);

OK - here we Audit right after the DriveMirror, then exit the Monitor...

- qemuDomainObjExitMonitor(driver, vm); - if (ret < 0) { + if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) {

Another place where ret == 0 could be true...

qemuDomainPrepareDiskChainElement(driver, vm, mirror, VIR_DISK_CHAIN_NO_ACCESS); goto endjob; @@ -16379,7 +16402,8 @@ qemuDomainBlockCommit(virDomainPtr dom, ret = qemuMonitorBlockCommit(priv->mon, device, topPath, basePath, backingPath, speed); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

Againt ret == 0 possible.

if (mirror) { if (ret == 0) { @@ -16473,7 +16497,10 @@ qemuDomainOpenGraphics(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorOpenGraphics(priv->mon, protocol, fd, "graphicsfd", (flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH) != 0); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto cleanup;

We're not going call ObjEndJob?

+ } qemuDomainObjEndJob(driver, vm);

cleanup: @@ -16542,7 +16569,10 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorOpenGraphics(priv->mon, protocol, pair[1], "graphicsfd", (flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH)); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto cleanup;

Again no ObjEndJob?

+ } qemuDomainObjEndJob(driver, vm); if (ret < 0) goto cleanup; @@ -16985,7 +17015,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, goto endjob; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

We could have ret == 0 and goto endjob.

if (ret < 0) goto endjob; } @@ -17157,7 +17188,8 @@ qemuDomainGetDiskErrors(virDomainPtr dom,

qemuDomainObjEnterMonitor(driver, vm); table = qemuMonitorGetBlockInfo(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; if (!table) goto endjob;

@@ -17436,7 +17468,8 @@ qemuDomainPMWakeup(virDomainPtr dom,

qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSystemWakeup(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;

endjob: qemuDomainObjEndJob(driver, vm); @@ -17867,7 +17900,8 @@ qemuDomainSetTime(virDomainPtr dom,

qemuDomainObjEnterMonitor(driver, vm); rv = qemuMonitorRTCResetReinjection(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;

if (rv < 0) goto endjob; @@ -18512,7 +18546,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver, visitBacking); ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, visitBacking)); - qemuDomainObjExitMonitor(driver, dom); + if (qemuDomainObjExitMonitor(driver, dom) < 0) + goto cleanup;

if (rc < 0) { virResetLastError(); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 31494c8..87b843b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -550,7 +550,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, qemuDomainObjPrivatePtr priv = vm->privateData; virHashTablePtr stats = NULL; size_t i; - int ret = -1; + int ret = -1, rc;

/* It is not a bug if there already is a NBD data */ if (!mig->nbd && @@ -571,18 +571,11 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, goto cleanup;

qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, - false) < 0) { - qemuDomainObjExitMonitor(driver, vm); + rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false); + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - } - qemuDomainObjExitMonitor(driver, vm); - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("domain exited meanwhile")); + if (rc < 0)

This is the kind of construct where I thought a : if (qemuDomainObjExitMonitor(driver,vm) < 0 || rc < 0) would fit... It doesn't matter to me - although consistency is nice too. It's almost like a macro could be generated that would check the status of qemuMonitor* call and the status of *ExitMonitor using another 'goto' type parameter in order to consistently handle each of these...

goto cleanup; - } }

if (!disk->info.alias || @@ -1655,14 +1648,11 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver, if (!port && ((virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) || (qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; + goto exit_monitor; }

- if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; - } + if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0) + goto exit_monitor; if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; } @@ -1675,6 +1665,10 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver, if (ret < 0) virPortAllocatorRelease(driver->migrationPorts, port); return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; }

/** @@ -1764,7 +1758,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, goto error; mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest, NULL, speed, 0, 0, mirror_flags); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto error;

if (mon_ret < 0) goto error; @@ -1785,7 +1780,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, /* explicitly do this *after* we entered the monitor, * as this is a critical section so we are guaranteed * priv->job.asyncAbort will not change */ - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED; virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), qemuDomainAsyncJobTypeToString(priv->job.asyncJob), @@ -1794,7 +1789,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, } mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info, NULL); - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm));

if (mon_ret < 0) goto error; @@ -1849,7 +1844,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, BLOCK_JOB_ABORT, true) < 0) { VIR_WARN("Unable to cancel block-job on '%s'", diskAlias); } - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm));

The context of this is within a while (lastGood) loop in an error path - theoretically the failure is dead guest, so what good is contining the loop? I do agree other paths are mainly ignoring errors too, but it doesn't seem necessary to continue if the guest died anyway.

} else { VIR_WARN("Unable to enter monitor. No block job cancelled"); } @@ -2231,6 +2226,7 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver, bool wait_for_spice = false; bool spice_migrated = false; size_t i = 0; + int rc;

if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { for (i = 0; i < vm->def->ngraphics; i++) { @@ -2252,12 +2248,11 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) return -1;

- if (qemuMonitorGetSpiceMigrationStatus(priv->mon, - &spice_migrated) < 0) { - qemuDomainObjExitMonitor(driver, vm); + rc = qemuMonitorGetSpiceMigrationStatus(priv->mon, &spice_migrated); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + if (rc < 0) return -1; - } - qemuDomainObjExitMonitor(driver, vm); virObjectUnlock(vm); nanosleep(&ts, NULL); virObjectLock(vm); @@ -2287,7 +2282,8 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver, } ret = qemuMonitorGetMigrationStatus(priv->mon, &status);

- qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1;

if (ret < 0 || qemuDomainJobInfoUpdateTime(priv->job.current) < 0) @@ -3884,7 +3880,7 @@ qemuMigrationRun(virQEMUDriverPtr driver, /* explicitly do this *after* we entered the monitor, * as this is a critical section so we are guaranteed * priv->job.asyncAbort will not change */ - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED; virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), qemuDomainAsyncJobTypeToString(priv->job.asyncJob), @@ -3892,24 +3888,20 @@ qemuMigrationRun(virQEMUDriverPtr driver, goto cleanup; }

- if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; - } + if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0) + goto exit_monitor;

/* connect to the destination qemu if needed */ if (spec->destType == MIGRATION_DEST_CONNECT_HOST && qemuMigrationConnect(driver, vm, spec) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; + goto exit_monitor; }

switch (spec->destType) { case MIGRATION_DEST_HOST: if (STREQ(spec->dest.host.protocol, "rdma") && virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; + goto exit_monitor; } ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags, spec->dest.host.protocol, @@ -3943,17 +3935,12 @@ qemuMigrationRun(virQEMUDriverPtr driver, VIR_FORCE_CLOSE(spec->dest.fd.qemu); break; } - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

Here is another case where if ret == 0, then goto cleanup doesn't seem right.

if (ret < 0) goto cleanup; ret = -1;

- if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto cleanup; - } - /* From this point onwards we *must* call cancel to abort the * migration on source if anything goes wrong */

@@ -4045,6 +4032,10 @@ qemuMigrationRun(virQEMUDriverPtr driver,

return ret;

+ exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm));

should a "ret = -1;" be added here? I think the rest seem to be OK - my eyes got weary and I hope I didn't miss any cases. It may be worth going back through the patches 9-13 when you've made all the changes in order to ensure things are more consistent and that nothing was missed. ACK with changes - John

+ goto cleanup; + cancel: orig_err = virSaveLastError();

@@ -4052,7 +4043,7 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) { qemuMonitorMigrateCancel(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); } } goto cleanup; @@ -5330,7 +5321,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuMonitorSetMigrationSpeed(priv->mon, QEMU_DOMAIN_MIG_BANDWIDTH_MAX); priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; }

if (!virDomainObjIsActive(vm)) { @@ -5406,11 +5398,11 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, if (virSetCloseExec(pipeFD[1]) < 0) { virReportSystemError(errno, "%s", _("Unable to set cloexec flag")); - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } if (virCommandRunAsync(cmd, NULL) < 0) { - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } rc = qemuMonitorMigrateToFd(priv->mon, @@ -5425,14 +5417,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, args, path, offset); } } - qemuDomainObjExitMonitor(driver, vm); - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - } - if (rc < 0) goto cleanup;

@@ -5445,7 +5431,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, if (virDomainObjIsActive(vm) && qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { qemuMonitorMigrateCancel(priv->mon); - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); } } goto cleanup; @@ -5465,7 +5451,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { qemuMonitorSetMigrationSpeed(priv->mon, saveMigBandwidth); priv->migMaxBandwidth = saveMigBandwidth; - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); }

VIR_FORCE_CLOSE(pipeFD[0]);

<no commit message> On 01/07/2015 10:42 AM, Ján Tomko wrote:

--- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 18 ++++++------------ 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 82d0c91..3d56eb8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2313,7 +2313,7 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); /* we continue on even in the face of error */ qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitor(driver, vm); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); } }

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 55d6fb3..a3d8983 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12896,7 +12896,8 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, }

ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; if (ret < 0) goto cleanup;

@@ -13417,12 +13418,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source, formatStr, reuse); if (!actions) { - qemuDomainObjExitMonitor(driver, vm); - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("domain crashed while taking the snapshot")); + if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - } }

In this case we do fall through to Audit - what the "strange" part of the Audit is though is we've changed the value of 'ret' if the ExitMonitor fails. Could a 'successful' DiskSnapshot ret value be changed into a failure because the guest died for some reason. Could anything else jump in (I've totally lost track now :-)) If that's expected, fine, but I thought it worthy to point out. Although I do "suspect" it could be reasonable to expect that the crash was because of taking the snapshot, but who knows for sure. ACK in general... Now that I'm done with the various callers. It's been hard to keep track in my own mind of the consistency between each routine and what I may have caught along the way. The following 4 do spring to mind though 1. Audit I think the Audits should occur. Whether they "all" happen right after the qemuMonitor* call while we still have the lock or whether they're done afterwards doesn't matter to me. 2. EndJob I started noticing some cases where EndJob was called and other times where it was missed. Of course I didn't go back to earlier patches to check all cases, but that might be something you want to do to ensure we're not leaving somewhere without the EndJob 3. Possibility of ret = qemuMonitor* == 0 and ExitMonitor == -1 While I agree it's highly likely that ret == -1 prior to ExitMonitor calls where sometimes ret = -1 is done and other times it's not. Still, if it is possible, then it needs to be accounted for. It doesn't seem so, but to have some call sequences do things one way while others do it differently makes it difficult to keep track. 4. Consistency I realize the inconsistencies existed before these patches, but since you're tackling all the callers in this series of patches - it may be nice to make things consistent. That way the next person who copies what some command does will then hopefully create consistent code. Another option is to make a macro that handles the ExitMonitor and goto's... John

virDomainAuditDisk(vm, disk->src, snap->src, "snapshot", ret >= 0); @@ -13560,12 +13557,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (ret == 0) { if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { ret = qemuMonitorTransaction(priv->mon, actions); - qemuDomainObjExitMonitor(driver, vm); - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("domain crashed while taking the snapshot")); + if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - } } else { /* failed to enter monitor, clean stuff up and quit */ ret = -1; @@ -14656,7 +14649,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitor(driver, vm); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; if (rc < 0) { /* XXX resume domain if it was running before the * failed loadvm attempt? */