[libvirt] [PATCH 00/10] qemu: Blockdevize external snapshot creation (blokcdev-add saga)
Peter Krempa (10): qemu: snapshot: Don't modify persistent XML if disk source is different qemu: driver: Remove dead code from qemuDomainSnapshotUpdateDiskSources qemu: Remove cleanup label in qemuDomainSnapshotPrepareDiskExternal qemu: snapshot: Restrict file existance check only for local storage qemu: Split out preparing of single snapshot from qemuDomainSnapshotDiskDataCollect qemu: Disband qemuDomainSnapshotCreateSingleDiskActive qemu: Merge conditions depending on the 'reuse' flag in qemuDomainSnapshotDiskDataCollectOne qemu: snapshot: Skip overlay file creation/interogation if unsupported qemu: Add -blockdev support for external snapshots qemu: Defer support checks for external active snapshots to blockdev code or qemu src/qemu/qemu_driver.c | 333 ++++++++++++++++++++++++++--------------- 1 file changed, 210 insertions(+), 123 deletions(-) -- 2.21.0
While the VM is running the persistent source of a disk might differ e.g. as the 'newDef' was redefined. Our snapshot code would blindly rewrite the source of such disk if it shared the 'target'. Fix this by checking whether the source is the same in the first place. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 11f97dbc65..b6fbb197b8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15332,6 +15332,7 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, qemuDomainSnapshotDiskDataPtr dd; char *backingStoreStr; virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); + virDomainDiskDefPtr persistdisk; int ret = -1; if (VIR_ALLOC_N(data, snapdef->ndisks) < 0) @@ -15352,13 +15353,12 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) goto cleanup; - /* Note that it's unsafe to assume that the disks in the persistent - * definition match up with the disks in the live definition just by - * checking that the target name is the same. We've done that - * historically this way though. */ + /* modify disk in persistent definition only when the source is the same */ if (vm->newDef && - (dd->persistdisk = virDomainDiskByName(vm->newDef, dd->disk->dst, - false))) { + (persistdisk = virDomainDiskByName(vm->newDef, dd->disk->dst, false)) && + virStorageSourceIsSameLocation(dd->disk->src, persistdisk->src)) { + + dd->persistdisk = persistdisk; if (!(dd->persistsrc = virStorageSourceCopy(dd->src, false))) goto cleanup; -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:35PM +0200, Peter Krempa wrote:
While the VM is running the persistent source of a disk might differ e.g. as the 'newDef' was redefined. Our snapshot code would blindly rewrite the source of such disk if it shared the 'target'. Fix this by checking whether the source is the same in the first place.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
dd->src is always allocated in this function as it contains the new source for the snapshot which is meant to replace the disk source. The label handling code executed if that source was not present thus is dead code. Remove it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b6fbb197b8..913b57855c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15421,13 +15421,6 @@ qemuDomainSnapshotUpdateDiskSources(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainSnapshotDiskDataPtr dd) { - if (!dd->src) { - /* Remove old metadata */ - if (qemuSecurityMoveImageMetadata(driver, vm, dd->disk->src, NULL) < 0) - VIR_WARN("Unable to remove disk metadata on vm %s", vm->def->name); - return; - } - /* storage driver access won'd be needed */ if (dd->initialized) virStorageFileDeinit(dd->src); -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:36PM +0200, Peter Krempa wrote:
dd->src is always allocated in this function as it contains the new source for the snapshot which is meant to replace the disk source.
The label handling code executed if that source was not present thus is dead code. Remove it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 ------- 1 file changed, 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Refactor the code to avoid having a cleanup label. This will simplify the change necessary when restricting this check in an upcoming patch. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 913b57855c..29a47af0a2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14997,8 +14997,9 @@ qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk, bool active, bool reuse) { - int ret = -1; struct stat st; + int err; + int rc; if (disk->src->readonly) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -15024,31 +15025,32 @@ qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk, if (virStorageFileInit(snapdisk->src) < 0) return -1; - if (virStorageFileStat(snapdisk->src, &st) < 0) { - if (errno != ENOENT) { - virReportSystemError(errno, + rc = virStorageFileStat(snapdisk->src, &st); + err = errno; + + virStorageFileDeinit(snapdisk->src); + + if (rc < 0) { + if (err != ENOENT) { + virReportSystemError(err, _("unable to stat for disk %s: %s"), snapdisk->name, snapdisk->src->path); - goto cleanup; + return -1; } else if (reuse) { - virReportSystemError(errno, + virReportSystemError(err, _("missing existing file for disk %s: %s"), snapdisk->name, snapdisk->src->path); - goto cleanup; + 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); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - virStorageFileDeinit(snapdisk->src); - return ret; + return 0; } -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:37PM +0200, Peter Krempa wrote:
Refactor the code to avoid having a cleanup label. This will simplify the change necessary when restricting this check in an upcoming patch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Soon we'll allow more protocols and storage types with snapshots where we in some cases can't check whether the storage already exists. Restrict the sanity checks whether the destination images exist or not for local storage where it's easy. For any other case we will fail later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 44 ++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 29a47af0a2..52540eb77d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15022,32 +15022,34 @@ qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk, return -1; } - if (virStorageFileInit(snapdisk->src) < 0) - return -1; + if (virStorageSourceIsLocalStorage(snapdisk->src)) { + if (virStorageFileInit(snapdisk->src) < 0) + return -1; - rc = virStorageFileStat(snapdisk->src, &st); - err = errno; + rc = virStorageFileStat(snapdisk->src, &st); + err = errno; - virStorageFileDeinit(snapdisk->src); + virStorageFileDeinit(snapdisk->src); - if (rc < 0) { - if (err != ENOENT) { - virReportSystemError(err, - _("unable to stat for disk %s: %s"), - snapdisk->name, snapdisk->src->path); - return -1; - } else if (reuse) { - virReportSystemError(err, - _("missing existing file for disk %s: %s"), - snapdisk->name, snapdisk->src->path); + if (rc < 0) { + if (err != ENOENT) { + virReportSystemError(err, + _("unable to stat for disk %s: %s"), + snapdisk->name, snapdisk->src->path); + return -1; + } else 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; } return 0; -- 2.21.0
s/existance/existence/ in the commit summary On Fri, Aug 16, 2019 at 03:54:38PM +0200, Peter Krempa wrote:
Soon we'll allow more protocols and storage types with snapshots where we in some cases can't check whether the storage already exists. Restrict the sanity checks whether the destination images exist or not for local storage where it's easy. For any other case we will fail later.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 44 ++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move the internals into qemuDomainSnapshotDiskDataCollectOne to make it obvious what's happening after moving more code here. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 104 +++++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52540eb77d..57d864cdd2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15316,6 +15316,62 @@ qemuDomainSnapshotDiskDataCleanup(qemuDomainSnapshotDiskDataPtr data, } +static int +qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virDomainSnapshotDiskDefPtr snapdisk, + qemuDomainSnapshotDiskDataPtr dd, + bool reuse) +{ + char *backingStoreStr; + virDomainDiskDefPtr persistdisk; + + dd->disk = disk; + + if (!(dd->src = virStorageSourceCopy(snapdisk->src, false))) + return -1; + + if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) + return -1; + + /* modify disk in persistent definition only when the source is the same */ + if (vm->newDef && + (persistdisk = virDomainDiskByName(vm->newDef, dd->disk->dst, false)) && + virStorageSourceIsSameLocation(dd->disk->src, persistdisk->src)) { + + dd->persistdisk = persistdisk; + + if (!(dd->persistsrc = virStorageSourceCopy(dd->src, false))) + return -1; + + if (virStorageSourceInitChainElement(dd->persistsrc, + dd->persistdisk->src, false) < 0) + return -1; + } + + if (qemuDomainStorageFileInit(driver, vm, dd->src, NULL) < 0) + return -1; + + dd->initialized = true; + + /* relative backing store paths need to be updated so that relative + * block commit still works */ + if (reuse) { + if (virStorageFileGetBackingStoreStr(dd->src, &backingStoreStr) < 0) + return -1; + if (backingStoreStr != NULL) { + if (virStorageIsRelative(backingStoreStr)) + VIR_STEAL_PTR(dd->relPath, backingStoreStr); + else + VIR_FREE(backingStoreStr); + } + } + + return 0; +} + + /** * qemuDomainSnapshotDiskDataCollect: * @@ -15333,10 +15389,7 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, size_t i; qemuDomainSnapshotDiskDataPtr data; size_t ndata = 0; - qemuDomainSnapshotDiskDataPtr dd; - char *backingStoreStr; virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); - virDomainDiskDefPtr persistdisk; int ret = -1; if (VIR_ALLOC_N(data, snapdef->ndisks) < 0) @@ -15346,49 +15399,10 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, if (snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; - dd = data + ndata; - ndata++; - - dd->disk = vm->def->disks[i]; - - if (!(dd->src = virStorageSourceCopy(snapdef->disks[i].src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) + if (qemuDomainSnapshotDiskDataCollectOne(driver, vm, vm->def->disks[i], + snapdef->disks + i, + data + ndata++, reuse) < 0) goto cleanup; - - /* modify disk in persistent definition only when the source is the same */ - if (vm->newDef && - (persistdisk = virDomainDiskByName(vm->newDef, dd->disk->dst, false)) && - virStorageSourceIsSameLocation(dd->disk->src, persistdisk->src)) { - - dd->persistdisk = persistdisk; - - if (!(dd->persistsrc = virStorageSourceCopy(dd->src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(dd->persistsrc, - dd->persistdisk->src, false) < 0) - goto cleanup; - } - - if (qemuDomainStorageFileInit(driver, vm, dd->src, NULL) < 0) - goto cleanup; - - dd->initialized = true; - - /* relative backing store paths need to be updated so that relative - * block commit still works */ - if (reuse) { - if (virStorageFileGetBackingStoreStr(dd->src, &backingStoreStr) < 0) - goto cleanup; - if (backingStoreStr != NULL) { - if (virStorageIsRelative(backingStoreStr)) - VIR_STEAL_PTR(dd->relPath, backingStoreStr); - else - VIR_FREE(backingStoreStr); - } - } } VIR_STEAL_PTR(*rdata, data); -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:39PM +0200, Peter Krempa wrote:
Move the internals into qemuDomainSnapshotDiskDataCollectOne to make it obvious what's happening after moving more code here.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 104 +++++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 45 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
After we always assume support for the 'transaction' command (c358adc57113b) and follow-up cleanups qemuDomainSnapshotCreateSingleDiskActive lost it's value. Move the code into appropriate helpers and remove the function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 53 ++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57d864cdd2..9248f912d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15368,6 +15368,22 @@ qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, } } + /* 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)); + 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) + return -1; + + dd->prepared = true; + return 0; } @@ -15463,36 +15479,6 @@ qemuDomainSnapshotUpdateDiskSources(virQEMUDriverPtr driver, } -static int -qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - qemuDomainSnapshotDiskDataPtr dd, - virJSONValuePtr actions, - bool reuse) -{ - if (qemuBlockSnapshotAddLegacy(actions, dd->disk, dd->src, reuse) < 0) - 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)); - 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) - return -1; - - dd->prepared = true; - - return 0; -} - - /* The domain is expected to be locked and active. */ static int qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, @@ -15535,9 +15521,10 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, * VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and * qcow2 format. */ for (i = 0; i < ndiskdata; i++) { - if (qemuDomainSnapshotCreateSingleDiskActive(driver, vm, - &diskdata[i], - actions, reuse) < 0) + if (qemuBlockSnapshotAddLegacy(actions, + diskdata[i].disk, + diskdata[i].src, + reuse) < 0) goto cleanup; } -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:40PM +0200, Peter Krempa wrote:
After we always assume support for the 'transaction' command (c358adc57113b) and follow-up cleanups qemuDomainSnapshotCreateSingleDiskActive lost it's value. Move the code
its
into appropriate helpers and remove the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 53 ++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57d864cdd2..9248f912d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15368,6 +15368,22 @@ qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, } }
+ /* 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)); + 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) + return -1; + + dd->prepared = true; + return 0;
I don't think qemuDomainSnapshotDiskDataCollectOne is an appropriate place to be creating image files. Either rename them to qemuDomainSnapshotDiskPrepareData or something that implies the change,
}
@@ -15463,36 +15479,6 @@ qemuDomainSnapshotUpdateDiskSources(virQEMUDriverPtr driver, }
-static int -qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - qemuDomainSnapshotDiskDataPtr dd, - virJSONValuePtr actions, - bool reuse) -{ - if (qemuBlockSnapshotAddLegacy(actions, dd->disk, dd->src, reuse) < 0) - return -1; -
or just split out this call out of qemuDomainSnapshotCreateSingleDiskActive
- /* 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)); - 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) - return -1; - - dd->prepared = true; - - return 0; -} - - /* The domain is expected to be locked and active. */ static int qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, @@ -15535,9 +15521,10 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
And iterate over diskdata here as well. With that change Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
* VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and * qcow2 format. */ for (i = 0; i < ndiskdata; i++) { - if (qemuDomainSnapshotCreateSingleDiskActive(driver, vm, - &diskdata[i], - actions, reuse) < 0) + if (qemuBlockSnapshotAddLegacy(actions, + diskdata[i].disk, + diskdata[i].src, + reuse) < 0)
goto cleanup; }
-- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9248f912d0..0bbde4b52d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15366,16 +15366,16 @@ qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, else VIR_FREE(backingStoreStr); } - } - - /* 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)); - return -1; + } else { + /* pre-create the image file so that we can label it before handing it to qemu */ + if (dd->src->type != VIR_STORAGE_TYPE_BLOCK) { + if (virStorageFileCreate(dd->src) < 0) { + virReportSystemError(errno, _("failed to create image file '%s'"), + NULLSTR(dd->src->path)); + return -1; + } + dd->created = true; } - dd->created = true; } /* set correct security, cgroup and locking options on the new image */ -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:41PM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
With blockdev we'll be able to support protocols which are not supported by the storage backends in libvirt. This means that we have to be able to skip the creation and relative storage path reading if it's not supported. This will make it impossible to use relative backing for network protocols but that would be almost insane anyways. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0bbde4b52d..6e10b691e9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15326,6 +15326,8 @@ qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, { char *backingStoreStr; virDomainDiskDefPtr persistdisk; + bool supportsCreate; + bool supportsBacking; dd->disk = disk; @@ -15350,31 +15352,38 @@ qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, return -1; } - if (qemuDomainStorageFileInit(driver, vm, dd->src, NULL) < 0) - return -1; - - dd->initialized = true; + supportsCreate = virStorageFileSupportsCreate(dd->src); + supportsBacking = virStorageFileSupportsBackingChainTraversal(dd->src); - /* relative backing store paths need to be updated so that relative - * block commit still works */ - if (reuse) { - if (virStorageFileGetBackingStoreStr(dd->src, &backingStoreStr) < 0) + if (supportsCreate || supportsBacking) { + if (qemuDomainStorageFileInit(driver, vm, dd->src, NULL) < 0) return -1; - if (backingStoreStr != NULL) { - if (virStorageIsRelative(backingStoreStr)) - VIR_STEAL_PTR(dd->relPath, backingStoreStr); - else - VIR_FREE(backingStoreStr); - } - } else { - /* pre-create the image file so that we can label it before handing it to qemu */ - if (dd->src->type != VIR_STORAGE_TYPE_BLOCK) { - if (virStorageFileCreate(dd->src) < 0) { - virReportSystemError(errno, _("failed to create image file '%s'"), - NULLSTR(dd->src->path)); - return -1; + + dd->initialized = true; + + /* relative backing store paths need to be updated so that relative + * block commit still works */ + if (reuse) { + if (supportsBacking) { + if (virStorageFileGetBackingStoreStr(dd->src, &backingStoreStr) < 0) + return -1; + if (backingStoreStr != NULL) { + if (virStorageIsRelative(backingStoreStr)) + VIR_STEAL_PTR(dd->relPath, backingStoreStr); + else + VIR_FREE(backingStoreStr); + } + } + } else { + /* pre-create the image file so that we can label it before handing it to qemu */ + if (supportsCreate && dd->src->type != VIR_STORAGE_TYPE_BLOCK) { + if (virStorageFileCreate(dd->src) < 0) { + virReportSystemError(errno, _("failed to create image file '%s'"), + NULLSTR(dd->src->path)); + return -1; + } + dd->created = true; } - dd->created = true; } } -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:42PM +0200, Peter Krempa wrote:
With blockdev we'll be able to support protocols which are not supported by the storage backends in libvirt. This means that we have to be able to skip the creation and relative storage path reading if it's not supported. This will make it impossible to use relative backing for network protocols but that would be almost insane anyways.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use the code for creating or attaching new storage source in the snapshot code and switch to 'blockdev-snapshot' for creating the snapshot itself. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 104 ++++++++++++++++++++++++++++++++++------- 1 file changed, 88 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6e10b691e9..c22f74adaa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15270,6 +15270,8 @@ struct _qemuDomainSnapshotDiskData { bool prepared; /* @src was prepared using qemuDomainStorageSourceAccessAllow */ virDomainDiskDefPtr disk; char *relPath; /* relative path component to fill into original disk */ + qemuBlockStorageSourceChainDataPtr crdata; + bool blockdevadded; virStorageSourcePtr persistsrc; virDomainDiskDefPtr persistdisk; @@ -15283,7 +15285,8 @@ static void qemuDomainSnapshotDiskDataCleanup(qemuDomainSnapshotDiskDataPtr data, size_t ndata, virQEMUDriverPtr driver, - virDomainObjPtr vm) + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) { size_t i; @@ -15294,6 +15297,15 @@ qemuDomainSnapshotDiskDataCleanup(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].blockdevadded) { + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { + + qemuBlockStorageSourceAttachRollback(qemuDomainGetMonitor(vm), + data[i].crdata->srcdata[0]); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + } + } + if (data[i].created && virStorageFileUnlink(data[i].src) < 0) { VIR_WARN("Unable to remove just-created %s", @@ -15312,6 +15324,7 @@ qemuDomainSnapshotDiskDataCleanup(qemuDomainSnapshotDiskDataPtr data, VIR_FREE(data[i].relPath); } + qemuBlockStorageSourceChainDataFree(data[i].crdata); VIR_FREE(data); } @@ -15319,15 +15332,20 @@ qemuDomainSnapshotDiskDataCleanup(qemuDomainSnapshotDiskDataPtr data, static int qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, virDomainObjPtr vm, + virQEMUDriverConfigPtr cfg, virDomainDiskDefPtr disk, virDomainSnapshotDiskDefPtr snapdisk, qemuDomainSnapshotDiskDataPtr dd, - bool reuse) + bool reuse, + bool blockdev, + qemuDomainAsyncJob asyncJob) { + qemuDomainObjPrivatePtr priv = vm->privateData; char *backingStoreStr; virDomainDiskDefPtr persistdisk; bool supportsCreate; bool supportsBacking; + int rc; dd->disk = disk; @@ -15393,6 +15411,44 @@ qemuDomainSnapshotDiskDataCollectOne(virQEMUDriverPtr driver, dd->prepared = true; + if (blockdev) { + /* create a terminator for the snapsot disks so that qemu does not try + * to open them at first */ + virObjectUnref(dd->src->backingStore); + if (!(dd->src->backingStore = virStorageSourceNew())) + return -1; + + if (qemuDomainPrepareStorageSourceBlockdev(dd->disk, dd->src, + priv, cfg) < 0) + return -1; + + if (!(dd->crdata = qemuBuildStorageSourceChainAttachPrepareBlockdevTop(dd->src, + priv->qemuCaps))) + return -1; + + if (reuse) { + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + rc = qemuBlockStorageSourceAttachApply(qemuDomainGetMonitor(vm), + dd->crdata->srcdata[0]); + + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + return -1; + } else { + if (qemuBlockStorageSourceCreateDetectSize(vm, dd->src, dd->disk->src, + asyncJob) < 0) + return -1; + + if (qemuBlockStorageSourceCreate(vm, dd->src, dd->disk->src, + NULL, dd->crdata->srcdata[0], + asyncJob) < 0) + return -1; + } + + dd->blockdevadded = true; + } + return 0; } @@ -15407,7 +15463,10 @@ static int qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMomentObjPtr snap, + virQEMUDriverConfigPtr cfg, bool reuse, + bool blockdev, + qemuDomainAsyncJob asyncJob, qemuDomainSnapshotDiskDataPtr *rdata, size_t *rndata) { @@ -15424,9 +15483,10 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, if (snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; - if (qemuDomainSnapshotDiskDataCollectOne(driver, vm, vm->def->disks[i], + if (qemuDomainSnapshotDiskDataCollectOne(driver, vm, cfg, vm->def->disks[i], snapdef->disks + i, - data + ndata++, reuse) < 0) + data + ndata++, reuse, blockdev, + asyncJob) < 0) goto cleanup; } @@ -15435,7 +15495,7 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, ret = 0; cleanup: - qemuDomainSnapshotDiskDataCleanup(data, ndata, driver, vm); + qemuDomainSnapshotDiskDataCleanup(data, ndata, driver, vm, asyncJob); return ret; } @@ -15456,13 +15516,15 @@ qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src) * @driver: QEMU driver * @vm: domain object * @dd: snapshot disk data object + * @blockdev: -blockdev is in use for the VM * * Updates disk definition after a successful snapshot. */ static void qemuDomainSnapshotUpdateDiskSources(virQEMUDriverPtr driver, virDomainObjPtr vm, - qemuDomainSnapshotDiskDataPtr dd) + qemuDomainSnapshotDiskDataPtr dd, + bool blockdev) { /* storage driver access won'd be needed */ if (dd->initialized) @@ -15475,11 +15537,13 @@ qemuDomainSnapshotUpdateDiskSources(virQEMUDriverPtr driver, dd->disk->src->readonly = true; VIR_STEAL_PTR(dd->disk->src->relPath, dd->relPath); + virObjectUnref(dd->src->backingStore); VIR_STEAL_PTR(dd->src->backingStore, dd->disk->src); VIR_STEAL_PTR(dd->disk->src, dd->src); /* fix numbering of disks */ - qemuDomainSnapshotUpdateDiskSourcesRenumber(dd->disk->src); + if (!blockdev) + qemuDomainSnapshotUpdateDiskSourcesRenumber(dd->disk->src); if (dd->persistdisk) { VIR_STEAL_PTR(dd->persistsrc->backingStore, dd->persistdisk->src); @@ -15506,6 +15570,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, qemuDomainSnapshotDiskDataPtr diskdata = NULL; size_t ndiskdata = 0; virErrorPtr orig_err = NULL; + bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); if (virDomainObjCheckActive(vm) < 0) return -1; @@ -15515,8 +15580,8 @@ 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 (qemuDomainSnapshotDiskDataCollect(driver, vm, snap, reuse, - &diskdata, &ndiskdata) < 0) + if (qemuDomainSnapshotDiskDataCollect(driver, vm, snap, cfg, reuse, blockdev, + asyncJob, &diskdata, &ndiskdata) < 0) goto cleanup; /* check whether there's anything to do */ @@ -15530,11 +15595,18 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, * VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and * qcow2 format. */ for (i = 0; i < ndiskdata; i++) { - if (qemuBlockSnapshotAddLegacy(actions, - diskdata[i].disk, - diskdata[i].src, - reuse) < 0) - goto cleanup; + if (blockdev) { + if (qemuBlockSnapshotAddBlockdev(actions, + diskdata[i].disk, + diskdata[i].src)) + goto cleanup; + } else { + if (qemuBlockSnapshotAddLegacy(actions, + diskdata[i].disk, + diskdata[i].src, + reuse) < 0) + goto cleanup; + } } if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) @@ -15551,7 +15623,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0); if (rc == 0) - qemuDomainSnapshotUpdateDiskSources(driver, vm, dd); + qemuDomainSnapshotUpdateDiskSources(driver, vm, dd, blockdev); } if (rc < 0) @@ -15583,7 +15655,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, vm->newDef) < 0)) ret = -1; - qemuDomainSnapshotDiskDataCleanup(diskdata, ndiskdata, driver, vm); + qemuDomainSnapshotDiskDataCleanup(diskdata, ndiskdata, driver, vm, asyncJob); virErrorRestore(&orig_err); return ret; -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:43PM +0200, Peter Krempa wrote:
Use the code for creating or attaching new storage source in the snapshot code and switch to 'blockdev-snapshot' for creating the snapshot itself.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 104 ++++++++++++++++++++++++++++++++++------- 1 file changed, 88 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove libvit's support check for the target of an external snapshot to the blockdev code or qemu. This will potentially require a more complex cleanup but removes a level of hardcoded feature checks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c22f74adaa..d9315e0df4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14931,7 +14931,8 @@ qemuDomainSnapshotPrepareDiskExternalInactive(virDomainSnapshotDiskDefPtr snapdi static int qemuDomainSnapshotPrepareDiskExternalActive(virDomainSnapshotDiskDefPtr snapdisk, - virDomainDiskDefPtr domdisk) + virDomainDiskDefPtr domdisk, + bool blockdev) { int actualType = virStorageSourceGetActualType(snapdisk->src); @@ -14948,6 +14949,10 @@ qemuDomainSnapshotPrepareDiskExternalActive(virDomainSnapshotDiskDefPtr snapdisk break; case VIR_STORAGE_TYPE_NETWORK: + /* defer all of the checking to either qemu or libvirt's blockdev code */ + if (blockdev) + break; + switch ((virStorageNetProtocol) snapdisk->src->protocol) { case VIR_STORAGE_NET_PROTOCOL_GLUSTER: break; @@ -14995,7 +15000,8 @@ static int qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk, virDomainSnapshotDiskDefPtr snapdisk, bool active, - bool reuse) + bool reuse, + bool blockdev) { struct stat st; int err; @@ -15018,7 +15024,7 @@ qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk, if (qemuDomainSnapshotPrepareDiskExternalInactive(snapdisk, disk) < 0) return -1; } else { - if (qemuDomainSnapshotPrepareDiskExternalActive(snapdisk, disk) < 0) + if (qemuDomainSnapshotPrepareDiskExternalActive(snapdisk, disk, blockdev) < 0) return -1; } @@ -15119,6 +15125,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, unsigned int *flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; + bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); int ret = -1; size_t i; bool active = virDomainObjIsActive(vm); @@ -15177,7 +15185,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, } if (qemuDomainSnapshotPrepareDiskExternal(dom_disk, disk, - active, reuse) < 0) + active, reuse, blockdev) < 0) goto cleanup; external++; -- 2.21.0
On Fri, Aug 16, 2019 at 03:54:44PM +0200, Peter Krempa wrote:
Remove libvit's support check for the target of an external snapshot to
libvirt
the blockdev code or qemu. This will potentially require a more complex cleanup but removes a level of hardcoded feature checks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa