[PATCH 0/7] qemu: snapshot: Fix removal of output files on failure
Peter Krempa (7): qemuSnapshotPrepareDiskExternal: Move temp variables into the block using them qemuSnapshotPrepareDiskExternal: Avoid condition squashing qemuSnapshotPrepareDiskExternal: Reject creation of block devices sooner qemuSnapshotPrepareDiskExternal: Enforce match between snapshot type and existing file type qemuSnapshotPrepareDiskExternal: Refactor existing file check conf: snapshot: rename variable holding memory snapshot file location qemuSnapshotCreateActiveExternal: Don't unlink memory snapshot image if it was existing before src/conf/snapshot_conf.c | 10 +++---- src/conf/snapshot_conf.h | 2 +- src/qemu/qemu_snapshot.c | 56 +++++++++++++++++++++++++++++----------- 3 files changed, 47 insertions(+), 21 deletions(-) -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 29e86342d6..67180a2b10 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -541,9 +541,6 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm, bool reuse, bool blockdev) { - struct stat st; - int err; - int rc; if (disk->src->readonly && !(reuse || blockdev)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -567,6 +564,10 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm, } if (virStorageSourceIsLocalStorage(snapdisk->src)) { + struct stat st; + int err; + int rc; + if (virStorageSourceInit(snapdisk->src) < 0) return -1; -- 2.31.1
Separate the 'else if' branches into nested conditions so that it's more obvious when we'll be adding additional checks later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 67180a2b10..81085bf4bd 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -582,18 +582,22 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm, _("unable to stat for disk %s: %s"), snapdisk->name, snapdisk->src->path); return -1; - } else if (reuse) { + } + + if (reuse) { virReportSystemError(err, _("missing existing file for disk %s: %s"), snapdisk->name, snapdisk->src->path); return -1; } - } else if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("external snapshot file for disk %s already " - "exists and is not a block device: %s"), - snapdisk->name, snapdisk->src->path); - return -1; + } else { + if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot file for disk %s already " + "exists and is not a block device: %s"), + snapdisk->name, snapdisk->src->path); + return -1; + } } } -- 2.31.1
In case when the snapshot target is of VIR_STORAGE_TYPE_BLOCK type and doesn't exist libvirt won't be able to create it. Reject such a config sooner. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 81085bf4bd..f06d0ada38 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -589,6 +589,13 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm, _("missing existing file for disk %s: %s"), snapdisk->name, snapdisk->src->path); return -1; + } else { + if (snapdisk->src->type == VIR_STORAGE_TYPE_BLOCK) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("block device snapshot target '%s' doesn't exist"), + snapdisk->src->path); + return -1; + } } } else { if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) { -- 2.31.1
The code executed later when creating a snapshot makes all decisions based on the configured type rather than the actual type of the existing file, while the check whether the file exists is based solely on the on-disk type. Since a block device is allowed to exist even when not reusing existing files in contrast to regular files this creates a potential for a block device to squeak past the check but then be influenced by other code executed later. Specifically this is a problem when creating a snapshot with the following XML: <domainsnapshot> <disks> <disk name='vdb' type='file'> <source file='/dev/sdb'/> </disk> </disks> </domainsnapshot> If the snapshot creation fails, '/dev/sdb' will be removed because it's considered to be a regular file by the cleanup code. Add a check that will force that the configured type matches the on-disk state. Additional supporting reason is that qemu stopped to accept block devices with the 'file' backend, thus the above configuration will not work any more. This allows us to fail sooner. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1972145 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f06d0ada38..96c43f69e7 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -598,6 +598,15 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm, } } } else { + /* at this point VIR_STORAGE_TYPE_DIR was already rejected */ + if ((snapdisk->src->type == VIR_STORAGE_TYPE_BLOCK && !S_ISBLK(st.st_mode)) || + (snapdisk->src->type == VIR_STORAGE_TYPE_FILE && !S_ISREG(st.st_mode))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("mismatch between configured type for snapshot disk '%s' and the type of existing file '%s'"), + snapdisk->name, snapdisk->src->path); + return -1; + } + if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external snapshot file for disk %s already " -- 2.31.1
Use the snapshot disk type from the definition now that we validate that it matches. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 96c43f69e7..dab386e455 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -607,10 +607,11 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm, return -1; } - if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) { + if (!reuse && + snapdisk->src->type == VIR_STORAGE_TYPE_FILE && + st.st_size > 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("external snapshot file for disk %s already " - "exists and is not a block device: %s"), + _("external snapshot file for disk %s already exists and is not a block device: %s"), snapdisk->name, snapdisk->src->path); return -1; } -- 2.31.1
'file' is too generic to know what's going on. Rename it to 'memorysnapshotfile'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 10 +++++----- src/conf/snapshot_conf.h | 2 +- src/qemu/qemu_snapshot.c | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index df3f2a4c63..0592640dd9 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -125,7 +125,7 @@ virDomainSnapshotDefDispose(void *obj) virDomainSnapshotDef *def = obj; size_t i; - g_free(def->file); + g_free(def->memorysnapshotfile); for (i = 0; i < def->ndisks; i++) virDomainSnapshotDiskDefClear(&def->disks[i]); g_free(def->disks); @@ -360,13 +360,13 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, "disk-only snapshot")); goto cleanup; } - def->file = g_steal_pointer(&memoryFile); + def->memorysnapshotfile = g_steal_pointer(&memoryFile); /* verify that memory path is absolute */ - if (def->file && !g_path_is_absolute(def->file)) { + if (def->memorysnapshotfile && !g_path_is_absolute(def->memorysnapshotfile)) { virReportError(VIR_ERR_XML_ERROR, _("memory snapshot file path (%s) must be absolute"), - def->file); + def->memorysnapshotfile); goto cleanup; } @@ -863,7 +863,7 @@ virDomainSnapshotDefFormatInternal(virBuffer *buf, if (def->memory) { virBufferAsprintf(buf, "<memory snapshot='%s'", virDomainSnapshotLocationTypeToString(def->memory)); - virBufferEscapeString(buf, " file='%s'", def->file); + virBufferEscapeString(buf, " file='%s'", def->memorysnapshotfile); virBufferAddLit(buf, "/>\n"); } diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 7d49a555cd..a3ec0cd410 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -82,7 +82,7 @@ struct _virDomainSnapshotDef { int state; /* virDomainSnapshotState */ int memory; /* virDomainMemorySnapshot */ - char *file; /* memory state file when snapshot is external */ + char *memorysnapshotfile; /* memory state file when snapshot is external */ size_t ndisks; /* should not exceed dom->ndisks */ virDomainSnapshotDiskDef *disks; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dab386e455..99fc4b836f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1451,9 +1451,9 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, goto cleanup; xml = NULL; - if ((ret = qemuSaveImageCreate(driver, vm, snapdef->file, data, - compressor, 0, - QEMU_ASYNC_JOB_SNAPSHOT)) < 0) + if ((ret = qemuSaveImageCreate(driver, vm, snapdef->memorysnapshotfile, + data, compressor, 0, + QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; /* the memory image was created, remove it on errors */ @@ -1522,7 +1522,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, virQEMUSaveDataFree(data); if (memory_unlink && ret < 0) - unlink(snapdef->file); + unlink(snapdef->memorysnapshotfile); return ret; } -- 2.31.1
When writing the memory snapshot into an existing file don't remove it if the snapshot fails later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 99fc4b836f..4e74ddd7f8 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1356,6 +1356,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); bool memory = snapdef->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; bool memory_unlink = false; + bool memory_existing = false; bool thaw = false; bool pmsuspended = false; int compressed; @@ -1451,13 +1452,16 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, goto cleanup; xml = NULL; + memory_existing = virFileExists(snapdef->memorysnapshotfile); + if ((ret = qemuSaveImageCreate(driver, vm, snapdef->memorysnapshotfile, data, compressor, 0, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; /* the memory image was created, remove it on errors */ - memory_unlink = true; + if (!memory_existing) + memory_unlink = true; /* forbid any further manipulation */ qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_DEFAULT_MASK); -- 2.31.1
On 6/16/21 5:05 PM, Peter Krempa wrote:
Peter Krempa (7): qemuSnapshotPrepareDiskExternal: Move temp variables into the block using them qemuSnapshotPrepareDiskExternal: Avoid condition squashing qemuSnapshotPrepareDiskExternal: Reject creation of block devices sooner qemuSnapshotPrepareDiskExternal: Enforce match between snapshot type and existing file type qemuSnapshotPrepareDiskExternal: Refactor existing file check conf: snapshot: rename variable holding memory snapshot file location qemuSnapshotCreateActiveExternal: Don't unlink memory snapshot image if it was existing before
src/conf/snapshot_conf.c | 10 +++---- src/conf/snapshot_conf.h | 2 +- src/qemu/qemu_snapshot.c | 56 +++++++++++++++++++++++++++++----------- 3 files changed, 47 insertions(+), 21 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jano Tomko -
Peter Krempa