[PATCH 0/3] qemu: Fix corner case in internal inactive snapshots
Peter Krempa (3): qemuSnapshotForEachQcow2: Don't initialize 'nrollback' qemu: process: Export qemuPrepareNVRAM for use in snapshot code qemu: snapshot: Ensure that NVRAM image exists when taking inactive internal snapshot src/qemu/qemu_process.c | 26 ++++++++++++++------------ src/qemu/qemu_process.h | 4 ++++ src/qemu/qemu_snapshot.c | 25 ++++++++++++++++++++----- 3 files changed, 38 insertions(+), 17 deletions(-) -- 2.48.1
The variable holds the amount of disks to roll back the snapshot for. The value must be set before the code jumps to the 'rollback:' label so the best situation is to not initialize it and let the compiler catch errors rather than initialize the unsigned variable to -1 and let it crash. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 7ce018b026..e5226db942 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -290,7 +290,7 @@ qemuSnapshotForEachQcow2(virDomainDef *def, size_t i; bool skipped = false; bool create = STREQ(op, "-c"); - size_t nrollback = -1; + size_t nrollback; virErrorPtr orig_err; /* pre-checks */ -- 2.48.1
Export qemuPrepareNVRAM so that it doesn't require the VM object. The snapshot code needs in the corner case of creating a snapshot of a freshly defined VM ensure that the nvram image exists in order to snapshot it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 26 ++++++++++++++------------ src/qemu/qemu_process.h | 4 ++++ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 34a755a49a..4948801ca1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4608,10 +4608,9 @@ qemuPrepareNVRAMHelper(int dstFD, static int -qemuPrepareNVRAMBlock(virDomainObj *vm, +qemuPrepareNVRAMBlock(virDomainLoaderDef *loader, bool reset_nvram) { - virDomainLoaderDef *loader = vm->def->os.loader; g_autoptr(virCommand) qemuimg = NULL; const char *templateFormatStr = "raw"; @@ -4672,13 +4671,12 @@ qemuPrepareNVRAMBlock(virDomainObj *vm, static int -qemuPrepareNVRAMFile(virDomainObj *vm, +qemuPrepareNVRAMFile(virQEMUDriver *driver, + virDomainLoaderDef *loader, bool reset_nvram) { - qemuDomainObjPrivate *priv = vm->privateData; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE srcFD = -1; - virDomainLoaderDef *loader = vm->def->os.loader; struct qemuPrepareNVRAMHelperData data; if (virFileExists(loader->nvram->path) && !reset_nvram) @@ -4720,21 +4718,24 @@ qemuPrepareNVRAMFile(virDomainObj *vm, } -static int -qemuPrepareNVRAM(virDomainObj *vm, +int +qemuPrepareNVRAM(virQEMUDriver *driver, + virDomainDef *def, bool reset_nvram) { - virDomainLoaderDef *loader = vm->def->os.loader; + virDomainLoaderDef *loader = def->os.loader; if (!loader || !loader->nvram) return 0; + VIR_DEBUG("nvram='%s'", NULLSTR(loader->nvram->path)); + switch (virStorageSourceGetActualType(loader->nvram)) { case VIR_STORAGE_TYPE_FILE: - return qemuPrepareNVRAMFile(vm, reset_nvram); + return qemuPrepareNVRAMFile(driver, loader, reset_nvram); case VIR_STORAGE_TYPE_BLOCK: - return qemuPrepareNVRAMBlock(vm, reset_nvram); + return qemuPrepareNVRAMBlock(loader, reset_nvram); case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_NETWORK: @@ -7399,7 +7400,8 @@ qemuProcessPrepareHost(virQEMUDriver *driver, qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0) return -1; - if (qemuPrepareNVRAM(vm, !!(flags & VIR_QEMU_PROCESS_START_RESET_NVRAM)) < 0) + if (qemuPrepareNVRAM(driver, vm->def, + !!(flags & VIR_QEMU_PROCESS_START_RESET_NVRAM)) < 0) return -1; if (vm->def->vsock) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 12781673c5..fee00ce53b 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -258,3 +258,7 @@ int qemuProcessSetupEmulator(virDomainObj *vm); void qemuProcessHandleNbdkitExit(qemuNbdkitProcess *nbdkit, virDomainObj *vm); + +int qemuPrepareNVRAM(virQEMUDriver *driver, + virDomainDef *def, + bool reset_nvram); -- 2.48.1
Attempting to take an internal snapshot of a freshly defined VM with qcow2 backed NVRAM results in failure as the NVRAM image doesn't get populated until the VM is started for the first time. Fix this by invoking qemuPrepareNVRAM() when qcow2 nvram is defined. Resolves: https://issues.redhat.com/browse/RHEL-73315 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e5226db942..ed140dd41c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -269,6 +269,7 @@ qemuSnapshotForEachQcow2One(virStorageSource *src, /** * qemuSnapshotForEachQcow2: * + * @driver: qemu driver configuration * @def: domain definition * @snap: snapshot object * @op: 'qemu-img snapshot' operation flag, one of "-c", "-d", "-a" @@ -282,7 +283,8 @@ qemuSnapshotForEachQcow2One(virStorageSource *src, * permissive modes. */ static int -qemuSnapshotForEachQcow2(virDomainDef *def, +qemuSnapshotForEachQcow2(virQEMUDriver *driver, + virDomainDef *def, virDomainMomentObj *snap, const char *op) { @@ -352,6 +354,16 @@ qemuSnapshotForEachQcow2(virDomainDef *def, if (virStorageSourceIsLocalStorage(nvram) && nvram->format == VIR_STORAGE_FILE_QCOW2) { + if (create) { + /* Ensure that the NVRAM image exists; e.g. when snapshotting + * a VM directly after defining it */ + if (qemuPrepareNVRAM(driver, def, false) < 0) { + nrollback = def->ndisks; + virErrorPreserveLast(&orig_err); + goto rollback; + } + } + if (qemuSnapshotForEachQcow2One(nvram, op, snap->def->name) < 0) { if (create) { nrollback = def->ndisks; @@ -392,7 +404,8 @@ static int qemuSnapshotCreateInactiveInternal(virDomainObj *vm, virDomainMomentObj *snap) { - return qemuSnapshotForEachQcow2(vm->def, snap, "-c"); + return qemuSnapshotForEachQcow2(QEMU_DOMAIN_PRIVATE(vm)->driver, + vm->def, snap, "-c"); } @@ -2697,7 +2710,8 @@ qemuSnapshotInternalRevertInactive(virDomainObj *vm, } /* Try all disks, but report failure if we skipped any. */ - if (qemuSnapshotForEachQcow2(def, snap, "-a") != 0) + if (qemuSnapshotForEachQcow2(QEMU_DOMAIN_PRIVATE(vm)->driver, + def, snap, "-a") != 0) return -1; return 0; @@ -4064,7 +4078,8 @@ qemuSnapshotDiscardImpl(virDomainObj *vm, if (qemuSnapshotDiscardExternal(vm, snap, externalData) < 0) return -1; } else { - if (qemuSnapshotForEachQcow2(def, snap, "-d") < 0) + if (qemuSnapshotForEachQcow2(QEMU_DOMAIN_PRIVATE(vm)->driver, + def, snap, "-d") < 0) return -1; } } else { -- 2.48.1
On Mon, Feb 03, 2025 at 06:41:08PM +0100, Peter Krempa wrote:
Peter Krempa (3): qemuSnapshotForEachQcow2: Don't initialize 'nrollback' qemu: process: Export qemuPrepareNVRAM for use in snapshot code qemu: snapshot: Ensure that NVRAM image exists when taking inactive internal snapshot
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa