<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? */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list