Changing labelling of the images does not need to happen after setting the labeling and lock manager access. This saves the cleanup of the labeling if the relative path can't be determined. --- src/qemu/qemu_driver.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3526da8c..0cf4aaa95 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17193,19 +17193,6 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; } - /* For the commit to succeed, we must allow qemu to open both the - * 'base' image and the parent of 'top' as read/write; 'top' might - * not have a parent, or might already be read-write. XXX It - * would also be nice to revert 'base' to read-only, as well as - * revoke access to files removed from the chain, when the commit - * operation succeeds, but doing that requires tracking the - * operation in XML across libvirtd restarts. */ - clean_access = true; - if (qemuDomainDiskChainElementPrepare(driver, vm, baseSource, false) < 0 || - (top_parent && top_parent != disk->src && - qemuDomainDiskChainElementPrepare(driver, vm, top_parent, false) < 0)) - goto endjob; - if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && topSource != disk->src) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { @@ -17225,6 +17212,19 @@ qemuDomainBlockCommit(virDomainPtr dom, } } + /* For the commit to succeed, we must allow qemu to open both the + * 'base' image and the parent of 'top' as read/write; 'top' might + * not have a parent, or might already be read-write. XXX It + * would also be nice to revert 'base' to read-only, as well as + * revoke access to files removed from the chain, when the commit + * operation succeeds, but doing that requires tracking the + * operation in XML across libvirtd restarts. */ + clean_access = true; + if (qemuDomainDiskChainElementPrepare(driver, vm, baseSource, false) < 0 || + (top_parent && top_parent != disk->src && + qemuDomainDiskChainElementPrepare(driver, vm, top_parent, false) < 0)) + goto endjob; + /* Start the commit operation. Pass the user's original spelling, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or -- 2.12.2

Calls to qemuDomainDiskChainElementPrepare resets the original error, thus we need to save it in the cleanup path of qemuDomainBlockCommit. --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0cf4aaa95..4f62c1a7a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17270,10 +17270,16 @@ qemuDomainBlockCommit(virDomainPtr dom, endjob: if (ret < 0 && clean_access) { + virErrorPtr orig_err = virSaveLastError(); /* Revert access to read-only, if possible. */ qemuDomainDiskChainElementPrepare(driver, vm, baseSource, true); if (top_parent && top_parent != disk->src) qemuDomainDiskChainElementPrepare(driver, vm, top_parent, true); + + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } } virStorageSourceFree(mirror); qemuDomainObjEndJob(driver, vm); -- 2.12.2

--- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 2 +- src/util/virstoragefile.h | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 044510f09..c1e9471c5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2593,6 +2593,7 @@ virStorageFileParseChainIndex; virStorageFileProbeFormat; virStorageFileResize; virStorageIsFile; +virStorageIsRelative; virStorageNetHostDefClear; virStorageNetHostDefCopy; virStorageNetHostDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3cbbb6c8f..8047d977f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -801,7 +801,7 @@ virStorageIsFile(const char *backing) } -static bool +bool virStorageIsRelative(const char *backing) { if (backing[0] == '/') diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 9ebfc1108..ce54a19ce 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -332,6 +332,7 @@ int virStorageFileResize(const char *path, int virStorageFileIsClusterFS(const char *path); bool virStorageIsFile(const char *path); +bool virStorageIsRelative(const char *backing); int virStorageFileGetLVMKey(const char *path, char **key); -- 2.12.2

It is necessary for some parts of the code to refresh just data based on the based on the backing store string. Add a convenience function that will retrieve this data. --- src/storage/storage_driver.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 3 +++ 2 files changed, 47 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 967776698..ab1dc8f36 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3263,6 +3263,50 @@ virStorageFileGetMetadata(virStorageSourcePtr src, } +/** + * virStorageFileGetBackingStoreStr: + * @src: storage object + * + * Extracts the backing store string as stored in the storage volume described + * by @src and returns it to the user. Caller is responsible for freeing it. + * In case when the string can't be retrieved or does not exist NULL is + * returned. + */ +char * +virStorageFileGetBackingStoreStr(virStorageSourcePtr src) +{ + virStorageSourcePtr tmp = NULL; + char *buf = NULL; + ssize_t headerLen; + char *ret = NULL; + + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return NULL; + + if (virStorageFileAccess(src, F_OK) < 0) + return NULL; + + if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) + return NULL; + + if (!(tmp = virStorageSourceCopy(src, false))) + goto cleanup; + + if (virStorageFileGetMetadataInternal(tmp, buf, headerLen, NULL) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, tmp->backingStoreRaw); + + cleanup: + VIR_FREE(buf); + virStorageSourceFree(tmp); + + return ret; +} + + static int virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def, virStoragePoolDefPtr pooldef) diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 530bc3388..ade05f715 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -54,6 +54,9 @@ int virStorageFileGetMetadata(virStorageSourcePtr src, bool report_broken) ATTRIBUTE_NONNULL(1); +char *virStorageFileGetBackingStoreStr(virStorageSourcePtr src) + ATTRIBUTE_NONNULL(1); + int virStorageTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def); -- 2.12.2

Commit 7456c4f5f introduced a regression by not reloading the backing chain of a disk after snapshot. The regression was caused as src->relPath was not set and thus the block commit code could not determine the relative path. This patch adds code that will load the backing store string if VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT and store it in the correct place when a snapshot is successfully completed. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1461303 --- src/qemu/qemu_driver.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4f62c1a7a..e91663ca9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14091,6 +14091,7 @@ struct _qemuDomainSnapshotDiskData { bool created; /* @src was created by the snapshot code */ bool prepared; /* @src was prepared using qemuDomainDiskChainElementPrepare */ virDomainDiskDefPtr disk; + char *relPath; /* relative path component to fill into original disk */ virStorageSourcePtr persistsrc; virDomainDiskDefPtr persistdisk; @@ -14124,6 +14125,7 @@ qemuDomainSnapshotDiskDataFree(qemuDomainSnapshotDiskDataPtr data, virStorageSourceFree(data[i].src); } virStorageSourceFree(data[i].persistsrc); + VIR_FREE(data[i].relPath); } VIR_FREE(data); @@ -14139,11 +14141,13 @@ qemuDomainSnapshotDiskDataFree(qemuDomainSnapshotDiskDataPtr data, static qemuDomainSnapshotDiskDataPtr qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainSnapshotObjPtr snap) + virDomainSnapshotObjPtr snap, + bool reuse) { size_t i; qemuDomainSnapshotDiskDataPtr ret; qemuDomainSnapshotDiskDataPtr dd; + char *backingStoreStr; if (VIR_ALLOC_N(ret, snap->def->ndisks) < 0) return NULL; @@ -14167,6 +14171,16 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, dd->initialized = true; + /* relative backing store paths need to be updated so that relative + * block commit still works */ + if (reuse && + (backingStoreStr = virStorageFileGetBackingStoreStr(dd->src))) { + if (virStorageIsRelative(backingStoreStr)) + VIR_STEAL_PTR(dd->relPath, backingStoreStr); + else + VIR_FREE(backingStoreStr); + } + /* 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 @@ -14210,6 +14224,7 @@ qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd, if (dd->initialized) virStorageFileDeinit(dd->src); + VIR_STEAL_PTR(dd->disk->src->relPath, dd->relPath); VIR_STEAL_PTR(dd->src->backingStore, dd->disk->src); VIR_STEAL_PTR(dd->disk->src, dd->src); @@ -14323,7 +14338,7 @@ 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 (!(diskdata = qemuDomainSnapshotDiskDataCollect(driver, vm, snap))) + if (!(diskdata = qemuDomainSnapshotDiskDataCollect(driver, vm, snap, reuse))) goto cleanup; cfg = virQEMUDriverGetConfig(driver); -- 2.12.2