On Tue, Jun 27, 2023 at 17:07:15 +0200, Pavel Hrdina wrote:
> When reverting to external snapshot we need to create new overlay qcow2
> files from the disk files the VM had when the snapshot was taken.
>
> There are some specifics and limitations when reverting to a snapshot:
>
> 1) When reverting to last snapshot we need to first create new overlay
> files before we can safely delete the old overlay files in case the
> creation fails so we have still recovery option when we error out.
>
> These new files will not have the suffix as when the snapshot was
> created as renaming the original files in order to use the same file
> names as when the snapshot was created would add unnecessary
> complexity to the code.
>
> 2) When reverting to any snapshot we will always create overlay files
> for every disk the VM had when the snapshot was done. Otherwise we
> would have to figure out if there is any other qcow2 image already
> using any of the VM disks as backing store and that itself might be
> extremely complex and in some cases impossible.
>
> 3) When reverting from any state the current overlay files will be
> always removed as that VM state is not meant to be saved. It's the
> same as with internal snapshots. If user want's to keep the current
> state before reverting they need to create a new snapshot. For now
> this will only work if the current snapshot is the last.
>
> Signed-off-by: Pavel Hrdina <[email protected]>
> ---
> src/qemu/qemu_snapshot.c | 232 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 228 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 1cb0ea55de..dbf2cdd5db 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -18,6 +18,8 @@
>
> #include <config.h>
>
> +#include <fcntl.h>
> +
> #include "qemu_snapshot.h"
>
> #include "qemu_monitor.h"
> @@ -1975,6 +1977,183 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm,
> }
Please document all new functions both any non-obvious parameter and
what it is supposed to do.
> +static int
> +qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
> + virDomainSnapshotDef *tmpsnapdef,
> + virDomainMomentObj *snap,
> + virDomainDef *config,
> + virDomainDef *inactiveConfig,
> + int *memsnapFD,
> + char **memsnapPath)
> +{
> + ssize_t i;
Normally we declare 'i' as unsigned.
> + bool active = virDomainObjIsActive(vm);
> + virDomainDef *domdef = NULL;
> + virDomainSnapshotLocation location =
> VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
This constant is used in one place only.
> + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> +
> + if (config) {
> + domdef = config;
> + } else {
> + domdef = inactiveConfig;
> + }
> +
> + /* We need this to generate creation timestamp that is used as default
> + * snapshot name. */
> + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0)
> + return -1;
> +
> + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false,
> true) < 0)
> + return -1;
> +
> + for (i = 0; i < tmpsnapdef->ndisks; i++) {
> + virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i];
> + virDomainDiskDef *domdisk = domdef->disks[i];
> +
> + if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active,
> false) < 0)
> + return -1;
> + }
> +
> + if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) {
> + virQEMUDriver *driver = ((qemuDomainObjPrivate *)
> vm->privateData)->driver;
> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> +
> + *memsnapPath = snapdef->memorysnapshotfile;
> + *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY,
> NULL);
Since the caller inherits the FD this must be documented.
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> +qemuSnapshotRevertExternalActive(virDomainObj *vm,
> + virDomainSnapshotDef *tmpsnapdef)
> +{
> + ssize_t i;
'size_t'
> + g_autoptr(GHashTable) blockNamedNodeData = NULL;
> + g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
> +
> + snapctxt = qemuSnapshotDiskContextNew(tmpsnapdef->ndisks, vm,
> VIR_ASYNC_JOB_SNAPSHOT);
> +
> + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm,
> VIR_ASYNC_JOB_SNAPSHOT)))
> + return -1;
> +
> + for (i = 0; i < tmpsnapdef->ndisks; i++) {
> + if (qemuSnapshotDiskPrepareOne(snapctxt,
> + vm->def->disks[i],
> + tmpsnapdef->disks + i,
> + blockNamedNodeData,
> + false,
> + true) < 0)
> + return -1;
> + }
> +
> + if (qemuSnapshotDiskCreate(snapctxt) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +static int
> +qemuSnapshotRevertExternalInactive(virDomainObj *vm,
> + virDomainSnapshotDef *tmpsnapdef,
> + virDomainDef *config,
> + virDomainDef *inactiveConfig)
This function is named '..Inactive'
> +{
> + virDomainDef *domdef = NULL;
> + g_autoptr(virBitmap) created = NULL;
> +
> + created = virBitmapNew(tmpsnapdef->ndisks);
> +
> + if (config) {
> + domdef = config;
> + } else {
> + domdef = inactiveConfig;
> + }
So this logic is weird. Inactive VMs have only one definition.
> +
> + if (config) {
> + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0)
> + return -1;
> + }
> +
> + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) <
> 0)
> + return -1;
> +
> + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false)
> < 0) {
> + ssize_t bit = -1;
Consider storing the error here ...
> +
> + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) {
> + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]);
> +
> + if (virStorageSourceInit(snapdisk->src) < 0 ||
> + virStorageSourceUnlink(snapdisk->src) < 0) {
... so that this doesn't overwrite it.
> + VIR_WARN("Failed to remove snapshot image '%s'",
> + snapdisk->src->path);
> + }
> + }
> +
> + return -1;
> + }
> +
> + return 0;
> +}
The rest seems to make sense:
Reviewed-by: Peter Krempa <[email protected]>
