From: Peter Krempa <pkrempa@redhat.com> When the 'backingStore' pointer is not populated the function calls 'virStorageSourceGetMetadata' to try to populate it but if the on-disk metadata doesn't have a backing image (e.g. if it's the 'base' image of the chain) the 'backingStore' or the metadata fetcher fails the pointer will still be NULL. The function then calls 'virStorageSourceIsSameLocation' but the internal functions for dealing with storage sources don't handle NULL gracefully. Since the code calling 'qemu-img' based on the data detected here doesn't actually raise errors if the operations fail there's no point in raising errors here either. Closes: https://gitlab.com/libvirt/libvirt/-/issues/844 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e738afffc3..8f58df3b45 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3147,7 +3147,8 @@ qemuSnapshotDiskHasBackingDisk(void *payload, if (!disk->src->backingStore) ignore_value(virStorageSourceGetMetadata(disk->src, uid, gid, 1, false)); - if (virStorageSourceIsSameLocation(disk->src->backingStore, iterdata->diskSrc)) { + if (disk->src->backingStore && + virStorageSourceIsSameLocation(disk->src->backingStore, iterdata->diskSrc)) { struct _qemuSnapshotDisksWithBackingStoreData *data = g_new0(struct _qemuSnapshotDisksWithBackingStoreData, 1); -- 2.52.0

From: Peter Krempa <pkrempa@redhat.com> The 'max_depth' argument of 'virStorageSourceGetMetadata' doesn't just limit how far the function goes but also fails completely if the chain is deeper than the passed value. In 'qemuSnapshotDiskHasBackingDisk' we only care about finding the backing image, so just one level below, the passed path, but due to the above setting '1' as max_depth will make the function simply fail every time. Extract and reuse QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH as the detection depth. While '200' layers is overkill for this code, we also start a full qemu instance just to delete an snapshot so this doens't matter and still protects from self-referential images. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_snapshot.c | 4 +++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ac56fc7cb4..486a0e7913 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6297,8 +6297,6 @@ qemuDomainStorageAlias(const char *device, int depth) } -#define QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH 200 - /** * qemuDomainStorageSourceValidateDepth: * @src: storage source chain to validate diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3396f929fd..b9bb338682 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -706,6 +706,7 @@ int qemuDomainCheckDiskStartupPolicy(virQEMUDriver *driver, size_t diskIndex, bool cold_boot); +#define QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH 200 int qemuDomainStorageSourceValidateDepth(virStorageSource *src, int add, const char *diskdst); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 19bb6f8b37..1628c33865 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3145,7 +3145,9 @@ qemuSnapshotDiskHasBackingDisk(void *payload, NULL, &uid, &gid); if (!disk->src->backingStore) - ignore_value(virStorageSourceGetMetadata(disk->src, uid, gid, 1, false)); + ignore_value(virStorageSourceGetMetadata(disk->src, uid, gid, + QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH, + false)); if (disk->src->backingStore && virStorageSourceIsSameLocation(disk->src->backingStore, iterdata->diskSrc)) { -- 2.52.0

From: Peter Krempa <pkrempa@redhat.com> The code calls 'qemu-img rebase' to fix the backing store references. The 'qemu-img' process here is run as the 'qemu' user or whatever the defaults and domain XML resolve to. Since this, in certain cases, works also on images which are not part of the backing chain and in privileged deployments thus can be owned by 'root:root' the update may fail (silently). To preserver root-squash deployments but fix also the above case, retry the operation on failure as current user. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 53 ++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1628c33865..70e4c37144 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3710,25 +3710,48 @@ qemuSnapshotUpdateBackingStore(qemuSnapshotDeleteExternalData *data) for (cur = data->disksWithBacking; cur; cur = g_slist_next(cur)) { struct _qemuSnapshotDisksWithBackingStoreData *backingData = cur->data; - g_autoptr(virCommand) cmd = NULL; + /* Try to run the command first as the appropriate user based on the + * domain definition and config. If error is returned retry as current + * (possibly privileged) user for cases where seclabels were reset + * to the default */ + g_autoptr(virCommand) cmd_user_qemu = NULL; + g_autoptr(virCommand) cmd_user_curr = NULL; + + if (!(cmd_user_qemu = virCommandNewArgList("qemu-img", + "rebase", + "-u", + "-F", + virStorageFileFormatTypeToString(data->parentDiskSrc->format), + "-f", + virStorageFileFormatTypeToString(backingData->diskSrc->format), + "-b", + data->parentDiskSrc->path, + backingData->diskSrc->path, + NULL))) + continue; - if (!(cmd = virCommandNewArgList("qemu-img", - "rebase", - "-u", - "-F", - virStorageFileFormatTypeToString(data->parentDiskSrc->format), - "-f", - virStorageFileFormatTypeToString(backingData->diskSrc->format), - "-b", - data->parentDiskSrc->path, - backingData->diskSrc->path, - NULL))) + virCommandSetUID(cmd_user_qemu, backingData->uid); + virCommandSetGID(cmd_user_qemu, backingData->gid); + + /* done on success */ + if (virCommandRun(cmd_user_qemu, NULL) == 0) continue; - virCommandSetUID(cmd, backingData->uid); - virCommandSetGID(cmd, backingData->gid); + /* retry as current user */ + if (!(cmd_user_curr = virCommandNewArgList("qemu-img", + "rebase", + "-u", + "-F", + virStorageFileFormatTypeToString(data->parentDiskSrc->format), + "-f", + virStorageFileFormatTypeToString(backingData->diskSrc->format), + "-b", + data->parentDiskSrc->path, + backingData->diskSrc->path, + NULL))) + continue; - ignore_value(virCommandRun(cmd, NULL)); + ignore_value(virCommandRun(cmd_user_curr, NULL)); } } -- 2.52.0