Re: [libvirt] [PATCH 18/16] snapshot: Consolidate writing snapshot state to disk

Remove a now-unused parameter from qemuDomainSnapshotWriteMetadata(). Then, instead of calling it after every individual change to a given snapshot, call it only once at the end of the API function that resulted in any overall changes. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain.c | 13 ++--------- src/qemu/qemu_driver.c | 50 ++++++++++++++---------------------------- 3 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ca24de15e5..5014f62463 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -681,7 +681,6 @@ int qemuDomainLogAppendMessage(virQEMUDriverPtr driver, const char *qemuFindQemuImgBinary(virQEMUDriverPtr driver); int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, - virDomainMomentObjPtr snapshot, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, const char *snapshotDir); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 424f839a00..e8756a0cea 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8448,7 +8448,6 @@ qemuFindQemuImgBinary(virQEMUDriverPtr driver) int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, - virDomainMomentObjPtr snapshot ATTRIBUTE_UNUSED, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, const char *snapshotDir) @@ -8597,19 +8596,11 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver, if (update_parent && snap->def->parent) { parentsnap = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); - if (!parentsnap) { + if (!parentsnap) VIR_WARN("missing parent snapshot matching name '%s'", snap->def->parent); - } else { + else virDomainSnapshotSetCurrent(vm->snapshots, parentsnap); - if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, driver->caps, - driver->xmlopt, - cfg->snapshotDir) < 0) { - VIR_WARN("failed to set parent snapshot '%s' as current", - snap->def->parent); - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - } - } } } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 018d1cdc87..862d832598 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -553,7 +553,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, * directory if successful. */ if (qemuDomainSnapshotLoadLegacy(vm, snapDir, caps) < 0) goto cleanup; - if (qemuDomainSnapshotWriteMetadata(vm, NULL, caps, + if (qemuDomainSnapshotWriteMetadata(vm, caps, qemu_driver->xmlopt, baseDir) < 0) goto cleanup; if (virFileDeleteTree(snapDir) < 0) @@ -15919,13 +15919,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!redefine && VIR_STRDUP(snap->def->parent, current->def->name) < 0) goto endjob; - if (update_current) { + if (update_current) virDomainSnapshotSetCurrent(vm->snapshots, NULL); - if (qemuDomainSnapshotWriteMetadata(vm, current, - driver->caps, driver->xmlopt, - cfg->snapshotDir) < 0) - goto endjob; - } } /* actually do the snapshot */ @@ -15972,7 +15967,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { if (update_current) virDomainSnapshotSetCurrent(vm->snapshots, snap); - if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps, + if (qemuDomainSnapshotWriteMetadata(vm, driver->caps, driver->xmlopt, cfg->snapshotDir) < 0) { /* if writing of metadata fails, error out rather than trying @@ -16499,10 +16494,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, current = virDomainSnapshotGetCurrent(vm->snapshots); if (current) { virDomainSnapshotSetCurrent(vm->snapshots, NULL); - if (qemuDomainSnapshotWriteMetadata(vm, current, - driver->caps, driver->xmlopt, - cfg->snapshotDir) < 0) - goto endjob; /* XXX Should we restore the current snapshot after this point * in the failure cases where we know there was no change? */ } @@ -16783,10 +16774,12 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, cleanup: if (ret == 0) { virDomainSnapshotSetCurrent(vm->snapshots, snap); - if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps, - driver->xmlopt, + if (qemuDomainSnapshotWriteMetadata(vm, driver->caps, driver->xmlopt, cfg->snapshotDir) < 0) { + /* We've already reverted; not much we can do now */ virDomainSnapshotSetCurrent(vm->snapshots, NULL); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to save metadata for snapshots")); ret = -1; } } else if (snap) { @@ -16839,14 +16832,9 @@ qemuDomainSnapshotReparentChildren(void *payload, VIR_FREE(snap->def->parent); if (rep->parent->def && - VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) { + VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) rep->err = -1; - return 0; - } - rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap, - rep->caps, rep->xmlopt, - rep->cfg->snapshotDir); return 0; } @@ -16912,20 +16900,8 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, &rem); if (rem.err < 0) goto endjob; - if (rem.current) { + if (rem.current) virDomainSnapshotSetCurrent(vm->snapshots, snap); - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps, - driver->xmlopt, - cfg->snapshotDir) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to set snapshot '%s' as current"), - snap->def->name); - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - goto endjob; - } - } - } } else if (snap->nchildren) { rep.cfg = cfg; rep.parent = snap->parent; @@ -16949,6 +16925,14 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, } endjob: + if (ret == 0 && + qemuDomainSnapshotWriteMetadata(vm, driver->caps, driver->xmlopt, + cfg->snapshotDir) < 0) { + /* We've already deleted everything; not much we can do now */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to save metadata for snapshots")); + ret = -1; + } qemuDomainObjEndJob(driver, vm); cleanup: