[PATCH 0/5] qemu: Don't abort() when creating internal snapshot with <disk type='volume'
See patch 5/5 Peter Krempa (5): virCommandAddArg: Don't abort on invalid input virDomainDiskTranslateSourcePool: Don't break error message in half qemuDomainSnapshotForEachQcow2Raw: Avoid a level of indentation qemuDomainSnapshotForEachQcow2Raw: Lock out operation on unsupported storage qemuDomainSnapshotForEachQcow2: Pass in 'def' rather than selecting it internally src/conf/domain_conf.c | 3 +- src/qemu/qemu_domain.c | 102 +++++++++++++++++++++------------------ src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_snapshot.c | 16 ++++-- src/util/vircommand.c | 2 +- 5 files changed, 70 insertions(+), 55 deletions(-) -- 2.28.0
Commit 912c6b22fc622cd7c7d29c7f8eaeb816b266daac added abort() when the 'val' parameter is NULL along with setting the error variable for the command. We don't want to abort in this case, just set the error. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 9a4f3d7f32..5117467c1d 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1503,7 +1503,7 @@ virCommandAddArg(virCommandPtr cmd, const char *val) if (val == NULL) { cmd->has_error = EINVAL; - abort(); + return; } arg = g_strdup(val); -- 2.28.0
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2393bf6a37..b1534dcc1e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -32847,8 +32847,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) if (def->startupPolicy != 0 && virStorageSourceGetActualType(def->src) != VIR_STORAGE_TYPE_FILE) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("'startupPolicy' is only valid for " - "'file' type volume")); + _("'startupPolicy' is only valid for 'file' type volume")); return -1; } -- 2.28.0
'continue' the loop if the device is not a disk. Saving the level makes one of the error messages fit on a single line. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 73 +++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1a80aa4c69..c8f721b00d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6501,49 +6501,48 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriverPtr driver, for (i = 0; i < ndisks; i++) { g_autoptr(virCommand) cmd = virCommandNewArgList(qemuimgbin, "snapshot", op, name, NULL); + int format = virDomainDiskGetFormat(def->disks[i]); /* FIXME: we also need to handle LVM here */ - if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - int format = virDomainDiskGetFormat(def->disks[i]); + if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK) + continue; - if (format > 0 && format != VIR_STORAGE_FILE_QCOW2) { - if (try_all) { - /* Continue on even in the face of error, since other - * disks in this VM may have the same snapshot name. - */ - VIR_WARN("skipping snapshot action on %s", - def->disks[i]->dst); - skipped = true; - continue; - } else if (STREQ(op, "-c") && i) { - /* We must roll back partial creation by deleting - * all earlier snapshots. */ - qemuDomainSnapshotForEachQcow2Raw(driver, def, name, - "-d", false, i); - } - virReportError(VIR_ERR_OPERATION_INVALID, - _("Disk device '%s' does not support" - " snapshotting"), - def->disks[i]->dst); - return -1; + if (format > 0 && format != VIR_STORAGE_FILE_QCOW2) { + if (try_all) { + /* Continue on even in the face of error, since other + * disks in this VM may have the same snapshot name. + */ + VIR_WARN("skipping snapshot action on %s", + def->disks[i]->dst); + skipped = true; + continue; + } else if (STREQ(op, "-c") && i) { + /* We must roll back partial creation by deleting + * all earlier snapshots. */ + qemuDomainSnapshotForEachQcow2Raw(driver, def, name, + "-d", false, i); } + virReportError(VIR_ERR_OPERATION_INVALID, + _("Disk device '%s' does not support snapshotting"), + def->disks[i]->dst); + return -1; + } - virCommandAddArg(cmd, virDomainDiskGetSource(def->disks[i])); - - if (virCommandRun(cmd, NULL) < 0) { - if (try_all) { - VIR_WARN("skipping snapshot action on %s", - def->disks[i]->dst); - skipped = true; - continue; - } else if (STREQ(op, "-c") && i) { - /* We must roll back partial creation by deleting - * all earlier snapshots. */ - qemuDomainSnapshotForEachQcow2Raw(driver, def, name, - "-d", false, i); - } - return -1; + virCommandAddArg(cmd, virDomainDiskGetSource(def->disks[i])); + + if (virCommandRun(cmd, NULL) < 0) { + if (try_all) { + VIR_WARN("skipping snapshot action on %s", + def->disks[i]->dst); + skipped = true; + continue; + } else if (STREQ(op, "-c") && i) { + /* We must roll back partial creation by deleting + * all earlier snapshots. */ + qemuDomainSnapshotForEachQcow2Raw(driver, def, name, + "-d", false, i); } + return -1; } } -- 2.28.0
Don't try to manipulate snapshots on network or unresolved volume backed storage. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c8f721b00d..c782810839 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6507,6 +6507,13 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriverPtr driver, if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK) continue; + if (!virStorageSourceIsLocalStorage(def->disks[i]->src)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't manipulate inactive snapshots of disk '%s'"), + def->disks[i]->dst); + return -1; + } + if (format > 0 && format != VIR_STORAGE_FILE_QCOW2) { if (try_all) { /* Continue on even in the face of error, since other -- 2.28.0
In some cases such as when creating an internal inactive snapshot we know that the domain definition in the snapshot is equivalent to the current definition. Additionally we set up the current definition for the snapshotting but not the one contained in the snapshot. Thus in some cases the caller knows better which def to use. Make qemuDomainSnapshotForEachQcow2 take the definition by the caller and copy the logic for selecting the definition to callers where we don't know for sure that the above claim applies. This fixes internal inactive snapshots when <disk type='volume'> is used as we translate the pool/vol combo only in the current def. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/97 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 22 +++++++++++----------- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_snapshot.c | 16 +++++++++++++--- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c782810839..663c0af867 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6560,18 +6560,11 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriverPtr driver, * failure, 1 if we skipped a disk due to try_all. */ int qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver, - virDomainObjPtr vm, + virDomainDefPtr def, virDomainMomentObjPtr snap, const char *op, bool try_all) { - /* Prefer action on the disks in use at the time the snapshot was - * created; but fall back to current definition if dealing with a - * snapshot created prior to libvirt 0.9.5. */ - virDomainDefPtr def = snap->def->dom; - - if (!def) - def = vm->def; return qemuDomainSnapshotForEachQcow2Raw(driver, def, snap->def->name, op, try_all, def->ndisks); } @@ -6592,9 +6585,16 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver, if (!metadata_only) { if (!virDomainObjIsActive(vm)) { /* Ignore any skipped disks */ - if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d", - true) < 0) - return -1; + + /* Prefer action on the disks in use at the time the snapshot was + * created; but fall back to current definition if dealing with a + * snapshot created prior to libvirt 0.9.5. */ + virDomainDefPtr def = snap->def->dom; + + if (!def) + def = vm->def; + + return qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true); } else { priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6b75b828c0..010bae285d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -613,7 +613,7 @@ int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, const char *snapshotDir); int qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver, - virDomainObjPtr vm, + virDomainDefPtr def, virDomainMomentObjPtr snap, const char *op, bool try_all); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 057b94e5b8..1c58ac8acf 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -138,7 +138,7 @@ qemuSnapshotCreateInactiveInternal(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMomentObjPtr snap) { - return qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-c", false); + return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); } @@ -1813,9 +1813,19 @@ qemuSnapshotRevertInactive(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMomentObjPtr snap) { + /* Prefer action on the disks in use at the time the snapshot was + * created; but fall back to current definition if dealing with a + * snapshot created prior to libvirt 0.9.5. */ + virDomainDefPtr def = snap->def->dom; + + if (!def) + def = vm->def; + /* Try all disks, but report failure if we skipped any. */ - int ret = qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-a", true); - return ret > 0 ? -1 : ret; + if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-a", true) != 0) + return -1; + + return 0; } -- 2.28.0
On a Monday in 2020, Peter Krempa wrote:
See patch 5/5
Peter Krempa (5): virCommandAddArg: Don't abort on invalid input virDomainDiskTranslateSourcePool: Don't break error message in half qemuDomainSnapshotForEachQcow2Raw: Avoid a level of indentation qemuDomainSnapshotForEachQcow2Raw: Lock out operation on unsupported storage qemuDomainSnapshotForEachQcow2: Pass in 'def' rather than selecting it internally
src/conf/domain_conf.c | 3 +- src/qemu/qemu_domain.c | 102 +++++++++++++++++++++------------------ src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_snapshot.c | 16 ++++-- src/util/vircommand.c | 2 +- 5 files changed, 70 insertions(+), 55 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa