[libvirt] [PATCH 00/13] qemu: External disk snapshot code refactors/cleanups (blockdev-add saga)
Few cleanups to the snapshot code extracted from my blockdev branch. Peter Krempa (13): qemu: snapshot: Pass 'cfg' to external snapshot functions qemu: Use VIR_AUTOPTR in qemuDomainSnapshotCreateDiskActive qemu: Use virErrorPreserveLast in qemuDomainSnapshotCreateDiskActive qemu: Use VIR_AUTO* in qemuDomainSnapshotCreateActiveExternal qemu: snapshot: Remove unused cleanup section in qemuDomainSnapshotCreateSingleDiskActive qemu: snapshot: Always save status and config after qemuDomainSnapshotCreateDiskActive qemu: snapshot: Always save config XML after qemuDomainSnapshotCreateDiskActive qemu: snapshot: Densely pack data in qemuDomainSnapshotDiskDataCollect qemu: snapshot: Move all cleanup of snapshot disk data to qemuDomainSnapshotDiskDataFree qemu: snapshot: Don't overload 'ret' in qemuDomainSnapshotCreateDiskActive qemu: snapshot: Unify 'cleanup' and 'error' in qemuDomainSnapshotCreateDiskActive qemu: snapshot: Return early if there's nothing to snapshot qemu: snapshot: Remove unnecessary 'do_transaction' logic in qemuDomainSnapshotCreateDiskActive src/qemu/qemu_driver.c | 189 +++++++++++++++++------------------------ 1 file changed, 79 insertions(+), 110 deletions(-) -- 2.21.0
The caller has it so there's no point in getting it again. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42b1ce2521..9c6221d3b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15231,6 +15231,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMomentObjPtr snap, unsigned int flags, + virQEMUDriverConfigPtr cfg, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -15240,7 +15241,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, size_t i; bool persist = false; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; - virQEMUDriverConfigPtr cfg = NULL; qemuDomainSnapshotDiskDataPtr diskdata = NULL; virErrorPtr orig_err = NULL; virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); @@ -15256,8 +15256,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (!(diskdata = qemuDomainSnapshotDiskDataCollect(driver, vm, snap, reuse))) goto cleanup; - cfg = virQEMUDriverGetConfig(driver); - /* Based on earlier qemuDomainSnapshotPrepare, all disks in this list are * now either VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, or * VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and @@ -15344,7 +15342,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, cleanup: qemuDomainSnapshotDiskDataFree(diskdata, snapdef->ndisks, driver, vm); virJSONValueFree(actions); - virObjectUnref(cfg); if (orig_err) { virSetError(orig_err); @@ -15359,6 +15356,7 @@ static int qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMomentObjPtr snap, + virQEMUDriverConfigPtr cfg, unsigned int flags) { virObjectEventPtr event; @@ -15371,7 +15369,6 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, bool memory_unlink = false; int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool pmsuspended = false; - virQEMUDriverConfigPtr cfg = NULL; int compressed; char *compressedpath = NULL; virQEMUSaveDataPtr data = NULL; @@ -15442,7 +15439,6 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, JOB_MASK(QEMU_JOB_SUSPEND) | JOB_MASK(QEMU_JOB_MIGRATION_OP))); - cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->snapshotImageFormat, &compressedpath, "snapshot", false)) < 0) @@ -15473,7 +15469,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, /* the domain is now paused if a memory snapshot was requested */ - if ((ret = qemuDomainSnapshotCreateDiskActive(driver, vm, snap, flags, + if ((ret = qemuDomainSnapshotCreateDiskActive(driver, vm, snap, flags, cfg, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; @@ -15532,7 +15528,6 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, virQEMUSaveDataFree(data); VIR_FREE(xml); VIR_FREE(compressedpath); - virObjectUnref(cfg); if (memory_unlink && ret < 0) unlink(snapdef->file); @@ -15771,7 +15766,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotObjGetDef(snap)->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { /* external full system or disk snapshot */ if (qemuDomainSnapshotCreateActiveExternal(driver, - vm, snap, flags) < 0) + vm, snap, cfg, flags) < 0) goto endjob; } else { /* internal full system */ -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:23PM +0200, Peter Krempa wrote:
The caller has it so there's no point in getting it again.
Unless the point is to reduce argument count.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c6221d3b2..8093b27656 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15235,7 +15235,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - virJSONValuePtr actions = NULL; + VIR_AUTOPTR(virJSONValue) actions = NULL; bool do_transaction = false; int ret = 0; size_t i; @@ -15341,7 +15341,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, cleanup: qemuDomainSnapshotDiskDataFree(diskdata, snapdef->ndisks, driver, vm); - virJSONValueFree(actions); if (orig_err) { virSetError(orig_err); -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:24PM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8093b27656..65b54b7511 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15301,7 +15301,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, error: if (ret < 0) { - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); for (i = 0; i < snapdef->ndisks; i++) { if (!diskdata[i].src) continue; @@ -15341,11 +15341,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, cleanup: qemuDomainSnapshotDiskDataFree(diskdata, snapdef->ndisks, driver, vm); - - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); return ret; } -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:25PM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65b54b7511..fad96053db 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15358,14 +15358,14 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, bool resume = false; int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - char *xml = NULL; + VIR_AUTOFREE(char *) xml = NULL; virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); bool memory = snapdef->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; bool memory_unlink = false; int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool pmsuspended = false; int compressed; - char *compressedpath = NULL; + VIR_AUTOFREE(char *) compressedpath = NULL; virQEMUSaveDataPtr data = NULL; /* If quiesce was requested, then issue a freeze command, and a @@ -15521,8 +15521,6 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, } virQEMUSaveDataFree(data); - VIR_FREE(xml); - VIR_FREE(compressedpath); if (memory_unlink && ret < 0) unlink(snapdef->file); -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:26PM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
After getting rid of pre-transaction qemu support the cleanup section is unused. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fad96053db..97f7ad16bf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15197,31 +15197,26 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, virJSONValuePtr actions, bool reuse) { - int ret = -1; - if (qemuBlockSnapshotAddLegacy(actions, dd->disk, dd->src, reuse) < 0) - goto cleanup; + return -1; /* pre-create the image file so that we can label it before handing it to qemu */ if (!reuse && dd->src->type != VIR_STORAGE_TYPE_BLOCK) { if (virStorageFileCreate(dd->src) < 0) { virReportSystemError(errno, _("failed to create image file '%s'"), NULLSTR(dd->src->path)); - goto cleanup; + return -1; } dd->created = true; } /* set correct security, cgroup and locking options on the new image */ if (qemuDomainStorageSourceAccessAllow(driver, vm, dd->src, false, true) < 0) - goto cleanup; + return -1; dd->prepared = true; - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:27PM +0200, Peter Krempa wrote:
After getting rid of pre-transaction qemu support the cleanup section is unused.
Yay for progress.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The error path is unlikely thus saving the status XML even if we didn't modify it does not add much burden. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 97f7ad16bf..40ba50c8c0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15327,12 +15327,10 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } } - if (ret == 0 || !do_transaction) { - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0 || - (persist && virDomainSaveConfig(cfg->configDir, driver->caps, - vm->newDef) < 0)) - ret = -1; - } + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0 || + (persist && virDomainSaveConfig(cfg->configDir, driver->caps, + vm->newDef) < 0)) + ret = -1; cleanup: qemuDomainSnapshotDiskDataFree(diskdata, snapdef->ndisks, driver, vm); -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:28PM +0200, Peter Krempa wrote:
The error path is unlikely thus saving the status XML even if we didn't modify it does not add much burden.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
If there's a offline config definition save it unconditionally even if it was not modified. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 40ba50c8c0..40182c84e5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15157,13 +15157,11 @@ qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src) /** * qemuDomainSnapshotUpdateDiskSources: * @dd: snapshot disk data object - * @persist: set to true if persistent config of the VM was changed * * Updates disk definition after a successful snapshot. */ static void -qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd, - bool *persist) +qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd) { if (!dd->src) return; @@ -15185,7 +15183,6 @@ qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd, if (dd->persistdisk) { VIR_STEAL_PTR(dd->persistsrc->backingStore, dd->persistdisk->src); VIR_STEAL_PTR(dd->persistdisk->src, dd->persistsrc); - *persist = true; } } @@ -15234,7 +15231,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, bool do_transaction = false; int ret = 0; size_t i; - bool persist = false; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; qemuDomainSnapshotDiskDataPtr diskdata = NULL; virErrorPtr orig_err = NULL; @@ -15287,7 +15283,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", ret >= 0); if (ret == 0) - qemuDomainSnapshotUpdateDiskSources(dd, &persist); + qemuDomainSnapshotUpdateDiskSources(dd); } if (ret < 0) @@ -15328,8 +15324,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0 || - (persist && virDomainSaveConfig(cfg->configDir, driver->caps, - vm->newDef) < 0)) + (vm->newDef && virDomainSaveConfig(cfg->configDir, driver->caps, + vm->newDef) < 0)) ret = -1; cleanup: -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:29PM +0200, Peter Krempa wrote:
If there's a offline config definition save it unconditionally even if
*an offline
it was not modified.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function skips disks which are not selected for snapshot. Rather than creating a sparse array and check whether the given field is filled compress the entries. Note that this does not allocate a smaller array, but the memory allocation is short-lived. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 69 +++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 40182c84e5..9736933143 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15071,37 +15071,42 @@ qemuDomainSnapshotDiskDataFree(qemuDomainSnapshotDiskDataPtr data, * Collects and prepares a list of structures that hold information about disks * that are selected for the snapshot. */ -static qemuDomainSnapshotDiskDataPtr +static int qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMomentObjPtr snap, - bool reuse) + bool reuse, + qemuDomainSnapshotDiskDataPtr *rdata, + size_t *rndata) { size_t i; - qemuDomainSnapshotDiskDataPtr ret; + qemuDomainSnapshotDiskDataPtr data; + size_t ndata = 0; qemuDomainSnapshotDiskDataPtr dd; char *backingStoreStr; virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); + int ret = -1; - if (VIR_ALLOC_N(ret, snapdef->ndisks) < 0) - return NULL; + if (VIR_ALLOC_N(data, snapdef->ndisks) < 0) + return -1; for (i = 0; i < snapdef->ndisks; i++) { if (snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; - dd = ret + i; + ndata++; + dd = data + ndata; dd->disk = vm->def->disks[i]; if (!(dd->src = virStorageSourceCopy(snapdef->disks[i].src, false))) - goto error; + goto cleanup; if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) - goto error; + goto cleanup; if (qemuDomainStorageFileInit(driver, vm, dd->src, NULL) < 0) - goto error; + goto cleanup; dd->initialized = true; @@ -15109,7 +15114,7 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, * block commit still works */ if (reuse) { if (virStorageFileGetBackingStoreStr(dd->src, &backingStoreStr) < 0) - goto error; + goto cleanup; if (backingStoreStr != NULL) { if (virStorageIsRelative(backingStoreStr)) VIR_STEAL_PTR(dd->relPath, backingStoreStr); @@ -15127,19 +15132,21 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, false))) { if (!(dd->persistsrc = virStorageSourceCopy(dd->src, false))) - goto error; + goto cleanup; if (virStorageSourceInitChainElement(dd->persistsrc, dd->persistdisk->src, false) < 0) - goto error; + goto cleanup; } } - return ret; + VIR_STEAL_PTR(*rdata, data); + *rndata = ndata; + ret = 0; - error: - qemuDomainSnapshotDiskDataFree(ret, snapdef->ndisks, driver, vm); - return NULL; + cleanup: + qemuDomainSnapshotDiskDataFree(data, ndata, driver, vm); + return ret; } @@ -15163,9 +15170,6 @@ qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src) static void qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd) { - if (!dd->src) - return; - /* storage driver access won'd be needed */ if (dd->initialized) virStorageFileDeinit(dd->src); @@ -15233,8 +15237,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, size_t i; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; qemuDomainSnapshotDiskDataPtr diskdata = NULL; + size_t ndiskdata = 0; virErrorPtr orig_err = NULL; - virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); if (virDomainObjCheckActive(vm) < 0) return -1; @@ -15244,17 +15248,15 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, /* prepare a list of objects to use in the vm definition so that we don't * have to roll back later */ - if (!(diskdata = qemuDomainSnapshotDiskDataCollect(driver, vm, snap, reuse))) + if (qemuDomainSnapshotDiskDataCollect(driver, vm, snap, reuse, + &diskdata, &ndiskdata) < 0) goto cleanup; /* Based on earlier qemuDomainSnapshotPrepare, all disks in this list are * now either VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, or * VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and * qcow2 format. */ - for (i = 0; i < snapdef->ndisks; i++) { - if (!diskdata[i].src) - continue; - + for (i = 0; i < ndiskdata; i++) { ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &diskdata[i], actions, reuse); @@ -15274,12 +15276,9 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - for (i = 0; i < snapdef->ndisks; i++) { + for (i = 0; i < ndiskdata; i++) { qemuDomainSnapshotDiskDataPtr dd = &diskdata[i]; - if (!dd->src) - continue; - virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", ret >= 0); if (ret == 0) @@ -15293,10 +15292,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, error: if (ret < 0) { virErrorPreserveLast(&orig_err); - for (i = 0; i < snapdef->ndisks; i++) { - if (!diskdata[i].src) - continue; - + for (i = 0; i < ndiskdata; i++) { if (diskdata[i].prepared) qemuDomainStorageSourceAccessRevoke(driver, vm, diskdata[i].src); @@ -15310,10 +15306,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, * stopped using them*/ bool paused = virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING; - for (i = 0; i < snapdef->ndisks; i++) { - if (!diskdata[i].disk) - continue; - + for (i = 0; i < ndiskdata; i++) { if (paused) virDomainLockImageDetach(driver->lockManager, vm, diskdata[i].disk->src); @@ -15329,7 +15322,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, ret = -1; cleanup: - qemuDomainSnapshotDiskDataFree(diskdata, snapdef->ndisks, driver, vm); + qemuDomainSnapshotDiskDataFree(diskdata, ndiskdata, driver, vm); virErrorRestore(&orig_err); return ret; -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:30PM +0200, Peter Krempa wrote:
The function skips disks which are not selected for snapshot. Rather than creating a sparse array and check whether the given field is filled compress the entries.
Note that this does not allocate a smaller array, but the memory allocation is short-lived.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 69 +++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 38 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuDomainSnapshotDiskDataFree also removes the resources associated with the disk data. Move the unlinking of the just-created file so that we can unify the cleanup paths. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9736933143..220a5846d8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15049,6 +15049,12 @@ qemuDomainSnapshotDiskDataFree(qemuDomainSnapshotDiskDataPtr data, /* on success of the snapshot the 'src' and 'persistsrc' properties will * be set to NULL by qemuDomainSnapshotUpdateDiskSources */ if (data[i].src) { + if (data[i].created && + virStorageFileUnlink(data[i].src) < 0) { + VIR_WARN("Unable to remove just-created %s", + NULLSTR(data[i].src->path)); + } + if (data[i].initialized) virStorageFileDeinit(data[i].src); @@ -15292,14 +15298,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, error: if (ret < 0) { virErrorPreserveLast(&orig_err); - for (i = 0; i < ndiskdata; i++) { - if (diskdata[i].prepared) - qemuDomainStorageSourceAccessRevoke(driver, vm, diskdata[i].src); - - if (diskdata[i].created && - virStorageFileUnlink(diskdata[i].src) < 0) - VIR_WARN("Unable to remove just-created %s", diskdata[i].src->path); - } } else { /* on successful snapshot we need to remove locks from the now-old * disks and if the VM is paused release locks on the images since qemu -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:31PM +0200, Peter Krempa wrote:
qemuDomainSnapshotDiskDataFree also removes the resources associated with the disk data. Move the unlinking of the just-created file so that we can unify the cleanup paths.
Pre-exisiting since commit cbb4d229, but touching the disk state from a *Free function feels wrong. Can you rename it first?
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Introduce 'rc' for collecting state from monitor commands so that we can initialize 'ret' to -1. This also fixes few cases which could return 0 from the fucntion despite an error condition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 220a5846d8..17270d5f83 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15239,7 +15239,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; VIR_AUTOPTR(virJSONValue) actions = NULL; bool do_transaction = false; - int ret = 0; + int rc; + int ret = -1; size_t i; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; qemuDomainSnapshotDiskDataPtr diskdata = NULL; @@ -15263,11 +15264,9 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, * VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and * qcow2 format. */ for (i = 0; i < ndiskdata; i++) { - ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, - &diskdata[i], - actions, reuse); - - if (ret < 0) + if (qemuDomainSnapshotCreateSingleDiskActive(driver, vm, + &diskdata[i], + actions, reuse) < 0) goto error; do_transaction = true; @@ -15277,24 +15276,26 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; - ret = qemuMonitorTransaction(priv->mon, &actions); + rc = qemuMonitorTransaction(priv->mon, &actions); if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; + rc = -1; for (i = 0; i < ndiskdata; i++) { qemuDomainSnapshotDiskDataPtr dd = &diskdata[i]; - virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", ret >= 0); + virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0); - if (ret == 0) + if (rc == 0) qemuDomainSnapshotUpdateDiskSources(dd); } - if (ret < 0) + if (rc < 0) goto error; } + ret = 0; + error: if (ret < 0) { virErrorPreserveLast(&orig_err); -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:32PM +0200, Peter Krempa wrote:
Introduce 'rc' for collecting state from monitor commands so that we can initialize 'ret' to -1. This also fixes few cases which could return 0 from the fucntion despite an error condition.
function
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
All cases taking the 'cleanup' path can take the original 'error' path. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 17270d5f83..5b6d3bb795 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15267,7 +15267,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &diskdata[i], actions, reuse) < 0) - goto error; + goto cleanup; do_transaction = true; } @@ -15291,12 +15291,12 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } if (rc < 0) - goto error; + goto cleanup; } ret = 0; - error: + cleanup: if (ret < 0) { virErrorPreserveLast(&orig_err); } else { @@ -15320,7 +15320,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, vm->newDef) < 0)) ret = -1; - cleanup: qemuDomainSnapshotDiskDataFree(diskdata, ndiskdata, driver, vm); virErrorRestore(&orig_err); -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:33PM +0200, Peter Krempa wrote:
All cases taking the 'cleanup' path can take the original 'error' path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Skip actual snapshot creation code if we have 0 disks to snapshot. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5b6d3bb795..b7f5142605 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15259,6 +15259,12 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, &diskdata, &ndiskdata) < 0) goto cleanup; + /* check whether there's anything to do */ + if (ndiskdata == 0) { + ret = 0; + goto cleanup; + } + /* Based on earlier qemuDomainSnapshotPrepare, all disks in this list are * now either VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, or * VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:34PM +0200, Peter Krempa wrote:
Skip actual snapshot creation code if we have 0 disks to snapshot.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Now that we never get to the actual snapshot code if there's nothing to do we can remove the variable and surrounding logic. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b7f5142605..69aefcf659 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15238,7 +15238,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; VIR_AUTOPTR(virJSONValue) actions = NULL; - bool do_transaction = false; int rc; int ret = -1; size_t i; @@ -15274,32 +15273,28 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, &diskdata[i], actions, reuse) < 0) goto cleanup; - - do_transaction = true; } - if (do_transaction) { - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - goto cleanup; - - rc = qemuMonitorTransaction(priv->mon, &actions); + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + goto cleanup; - if (qemuDomainObjExitMonitor(driver, vm) < 0) - rc = -1; + rc = qemuMonitorTransaction(priv->mon, &actions); - for (i = 0; i < ndiskdata; i++) { - qemuDomainSnapshotDiskDataPtr dd = &diskdata[i]; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + rc = -1; - virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0); + for (i = 0; i < ndiskdata; i++) { + qemuDomainSnapshotDiskDataPtr dd = &diskdata[i]; - if (rc == 0) - qemuDomainSnapshotUpdateDiskSources(dd); - } + virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0); - if (rc < 0) - goto cleanup; + if (rc == 0) + qemuDomainSnapshotUpdateDiskSources(dd); } + if (rc < 0) + goto cleanup; + ret = 0; cleanup: -- 2.21.0
On Wed, Jun 05, 2019 at 05:28:35PM +0200, Peter Krempa wrote:
Now that we never get to the actual snapshot code if there's nothing to do we can remove the variable and surrounding logic.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa