We create some images for testing our code. We've recorded wrong format of the backing file for one of the images though. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/virstoragetest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 39040bf4cb..e7794d6168 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -964,7 +964,7 @@ mymain(void) /* Rewrite wrap and qcow2 back to 3-deep chain, absolute backing */ virCommandFree(cmd); cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", - "-F", "qcow2", "-b", absraw, "qcow2", NULL); + "-F", "raw", "-b", absraw, "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; -- 2.24.1

Both callers pass false. Since we frown upon format probing, remove the unused possibility to do the probing. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 2 +- src/storage/storage_util.c | 2 +- src/util/virstoragefile.c | 23 ++++++----------------- src/util/virstoragefile.h | 3 +-- 4 files changed, 9 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39e1f044e0..943caea011 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12269,7 +12269,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, if (virStorageSourceUpdateBackingSizes(src, fd, &sb) < 0) goto cleanup; - if (virStorageSourceUpdateCapacity(src, buf, len, false) < 0) + if (virStorageSourceUpdateCapacity(src, buf, len) < 0) goto cleanup; /* If guest is not using raw disk format and is on a host block diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c2754dbb93..8199c7a2af 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1756,7 +1756,7 @@ storageBackendUpdateVolTargetInfo(virStorageVolType voltype, } } - if (virStorageSourceUpdateCapacity(target, buf, len, false) < 0) + if (virStorageSourceUpdateCapacity(target, buf, len) < 0) return -1; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b984204b93..cc351e3f53 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3964,18 +3964,15 @@ virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, * @src: disk source definition structure * @buf: buffer to the storage file header * @len: length of the storage file header - * @probe: allow probe * - * Update the storage @src capacity. This may involve probing the storage - * @src in order to "see" if we can recognize what exists. + * Update the storage @src capacity. * * Returns 0 on success, -1 on error. */ int virStorageSourceUpdateCapacity(virStorageSourcePtr src, char *buf, - ssize_t len, - bool probe) + ssize_t len) { int format = src->format; g_autoptr(virStorageSource) meta = NULL; @@ -3984,18 +3981,10 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src, * the metadata has a capacity, use that, otherwise fall back to * physical size. */ if (format == VIR_STORAGE_FILE_NONE) { - if (!probe) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no disk format for %s and probing is disabled"), - src->path); - return -1; - } - - if ((format = virStorageFileProbeFormatFromBuf(src->path, - buf, len)) < 0) - return -1; - - src->format = format; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no disk format for %s was specified"), + src->path); + return -1; } if (format == VIR_STORAGE_FILE_RAW && !src->encryption) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f41e6e357..2a684fd746 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -472,8 +472,7 @@ int virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, int virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, int fd, struct stat const *sb); int virStorageSourceUpdateCapacity(virStorageSourcePtr src, - char *buf, ssize_t len, - bool probe); + char *buf, ssize_t len); int virStorageSourceNewFromBacking(virStorageSourcePtr parent, virStorageSourcePtr *backing); -- 2.24.1

We store the backing file string in the structure so we should also store the format so that callers can be simplified. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virstoragefile.c | 35 +++++++++++++++++------------------ src/util/virstoragefile.h | 1 + 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cc351e3f53..2d8fe59c25 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -935,15 +935,11 @@ virStorageFileGetEncryptionPayloadOffset(const struct FileEncryptionInfo *info, static int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, - size_t len, - int *backingFormat) + size_t len) { - int dummy; + int format; size_t i; - if (!backingFormat) - backingFormat = &dummy; - VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", meta->path, buf, len, meta->format); @@ -1009,8 +1005,10 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, VIR_FREE(meta->backingStoreRaw); if (fileTypeInfo[meta->format].getBackingStore != NULL) { int store = fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, - backingFormat, + &format, buf, len); + meta->backingStoreRawFormat = format; + if (store == BACKING_STORE_INVALID) return 0; @@ -1135,20 +1133,18 @@ virStorageFileGetMetadataFromBuf(const char *path, int *backingFormat) { virStorageSourcePtr ret = NULL; - int dummy; - - if (!backingFormat) - backingFormat = &dummy; if (!(ret = virStorageFileMetadataNew(path, format))) return NULL; - if (virStorageFileGetMetadataInternal(ret, buf, len, - backingFormat) < 0) { + if (virStorageFileGetMetadataInternal(ret, buf, len) < 0) { virObjectUnref(ret); return NULL; } + if (backingFormat) + *backingFormat = ret->backingStoreRawFormat; + return ret; } @@ -1211,9 +1207,12 @@ virStorageFileGetMetadataFromFD(const char *path, return NULL; } - if (virStorageFileGetMetadataInternal(meta, buf, len, backingFormat) < 0) + if (virStorageFileGetMetadataInternal(meta, buf, len) < 0) return NULL; + if (backingFormat) + *backingFormat = meta->backingStoreRawFormat; + if (S_ISREG(sb.st_mode)) meta->type = VIR_STORAGE_TYPE_FILE; else if (S_ISBLK(sb.st_mode)) @@ -2293,6 +2292,7 @@ virStorageSourceCopy(const virStorageSource *src, def->volume = g_strdup(src->volume); def->relPath = g_strdup(src->relPath); def->backingStoreRaw = g_strdup(src->backingStoreRaw); + def->backingStoreRawFormat = src->backingStoreRawFormat; def->externalDataStoreRaw = g_strdup(src->externalDataStoreRaw); def->snapshot = g_strdup(src->snapshot); def->configFile = g_strdup(src->configFile); @@ -5000,7 +5000,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, unsigned int depth) { size_t headerLen; - int backingFormat; int rv; g_autofree char *buf = NULL; g_autoptr(virStorageSource) backingStore = NULL; @@ -5018,7 +5017,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, &buf, &headerLen, cycle) < 0) return -1; - if (virStorageFileGetMetadataInternal(src, buf, headerLen, &backingFormat) < 0) + if (virStorageFileGetMetadataInternal(src, buf, headerLen) < 0) return -1; if (src->backingStoreRaw) { @@ -5029,7 +5028,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, if (rv == 1) return 0; - backingStore->format = backingFormat; + backingStore->format = src->backingStoreRawFormat; if (backingStore->format == VIR_STORAGE_FILE_AUTO) { /* Assuming the backing store to be raw can lead to failures. We do @@ -5180,7 +5179,7 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src, if (!(tmp = virStorageSourceCopy(src, false))) return -1; - if (virStorageFileGetMetadataInternal(tmp, buf, headerLen, NULL) < 0) + if (virStorageFileGetMetadataInternal(tmp, buf, headerLen) < 0) return -1; *backing = g_steal_pointer(&tmp->backingStoreRaw); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2a684fd746..ecba418bb3 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -330,6 +330,7 @@ struct _virStorageSource { /* Name of the child backing store recorded in metadata of the * current file. */ char *backingStoreRaw; + virStorageFileFormat backingStoreRawFormat; /* Name of the child data file recorded in metadata of the current file. */ char *externalDataStoreRaw; -- 2.24.1

When we create the new virStorageSource from the definitions stored in the parent we should also use the 'backingStoreRawFormat' field to populate the format. Callers which use virStorageSourceNewFromBacking are also fixed to stop setting the format manually. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/storage/storage_util.c | 5 +---- src/util/virstoragefile.c | 3 +-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 8199c7a2af..f71abbd729 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3329,7 +3329,6 @@ static int storageBackendProbeTarget(virStorageSourcePtr target, virStorageEncryptionPtr *encryption) { - int backingStoreFormat; int rc; struct stat sb; g_autoptr(virStorageSource) meta = NULL; @@ -3360,15 +3359,13 @@ storageBackendProbeTarget(virStorageSourcePtr target, if (!(meta = virStorageFileGetMetadataFromFD(target->path, fd, VIR_STORAGE_FILE_AUTO, - &backingStoreFormat))) + NULL))) return -1; if (meta->backingStoreRaw) { if (virStorageSourceNewFromBacking(meta, &target->backingStore) < 0) return -1; - target->backingStore->format = backingStoreFormat; - /* XXX: Remote storage doesn't play nicely with volumes backed by * remote storage. To avoid trouble, just fake the backing store is RAW * and put the string from the metadata as the path of the target. */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2d8fe59c25..6fddc0610e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3824,6 +3824,7 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent, backing)) < 0) return rc; + (*backing)->format = parent->backingStoreRawFormat; (*backing)->readonly = true; return rc; } @@ -5028,8 +5029,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, if (rv == 1) return 0; - backingStore->format = src->backingStoreRawFormat; - if (backingStore->format == VIR_STORAGE_FILE_AUTO) { /* Assuming the backing store to be raw can lead to failures. We do * it only when we must not report an error to prevent losing VMs. -- 2.24.1

Take the format of the backing store from the 'meta' object directly and use g_steal_pointer go steal the path. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/storage/storage_backend_gluster.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 4a8ee3870d..e92e9612bd 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -224,7 +224,6 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, int ret = -1; glfs_fd_t *fd = NULL; ssize_t len; - int backingFormat; g_autoptr(virStorageVolDef) vol = NULL; g_autoptr(virStorageSource) meta = NULL; g_autofree char *header = NULL; @@ -277,7 +276,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, VIR_STORAGE_FILE_AUTO, - &backingFormat))) + NULL))) goto cleanup; if (meta->backingStoreRaw) { @@ -286,13 +285,11 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, vol->target.backingStore->type = VIR_STORAGE_TYPE_NETWORK; - vol->target.backingStore->path = meta->backingStoreRaw; + vol->target.backingStore->path = g_steal_pointer(&meta->backingStoreRaw); + vol->target.backingStore->format = meta->backingStoreRawFormat; - if (backingFormat < 0) + if (vol->target.backingStore->format < 0) vol->target.backingStore->format = VIR_STORAGE_FILE_RAW; - else - vol->target.backingStore->format = backingFormat; - meta->backingStoreRaw = NULL; } vol->target.format = meta->format; -- 2.24.1

None of the callers actually use it. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/storage/storage_backend_gluster.c | 3 +-- src/util/virstoragefile.c | 16 ++++++---------- src/util/virstoragefile.h | 3 +-- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e92e9612bd..db3df82aad 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -275,8 +275,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup; if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, - VIR_STORAGE_FILE_AUTO, - NULL))) + VIR_STORAGE_FILE_AUTO))) goto cleanup; if (meta->backingStoreRaw) { diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6fddc0610e..94805e8741 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1109,7 +1109,6 @@ virStorageFileMetadataNew(const char *path, * @buf: header bytes from @path * @len: length of @buf * @format: format of the storage file - * @backingFormat: format of @backing * * Extract metadata about the storage volume with the specified image format. * If image format is VIR_STORAGE_FILE_AUTO, it will probe to automatically @@ -1119,9 +1118,10 @@ virStorageFileMetadataNew(const char *path, * that might be raw if that file will then be passed to a guest, since a * malicious guest can turn a raw file into any other non-raw format at will. * - * If the returned @backingFormat is VIR_STORAGE_FILE_AUTO it indicates the - * image didn't specify an explicit format for its backing store. Callers are - * advised against probing for the backing store format in this case. + * If the 'backingStoreRawFormat' field of the returned structure is + * VIR_STORAGE_FILE_AUTO it indicates the image didn't specify an explicit + * format for its backing store. Callers are advised against probing for the + * backing store format in this case. * * Caller MUST free the result after use via virObjectUnref. */ @@ -1129,8 +1129,7 @@ virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - int format, - int *backingFormat) + int format) { virStorageSourcePtr ret = NULL; @@ -1142,9 +1141,6 @@ virStorageFileGetMetadataFromBuf(const char *path, return NULL; } - if (backingFormat) - *backingFormat = ret->backingStoreRawFormat; - return ret; } @@ -3991,7 +3987,7 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src, if (format == VIR_STORAGE_FILE_RAW && !src->encryption) { src->capacity = src->physical; } else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf, - len, format, NULL))) { + len, format))) { src->capacity = meta->capacity ? meta->capacity : src->physical; if (src->encryption && meta->encryption) src->encryption->payload_offset = meta->encryption->payload_offset; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index ecba418bb3..32f2c82147 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -386,8 +386,7 @@ virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - int format, - int *backingFormat) + int format) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageFileChainGetBroken(virStorageSourcePtr chain, char **broken_file); -- 2.24.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/storage/storage_util.c | 3 +-- src/util/virstoragefile.c | 12 +----------- src/util/virstoragefile.h | 3 +-- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index f71abbd729..8e43c7868d 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3358,8 +3358,7 @@ storageBackendProbeTarget(virStorageSourcePtr target, if (!(meta = virStorageFileGetMetadataFromFD(target->path, fd, - VIR_STORAGE_FILE_AUTO, - NULL))) + VIR_STORAGE_FILE_AUTO))) return -1; if (meta->backingStoreRaw) { diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 94805e8741..d5ac5970f1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1161,21 +1161,14 @@ virStorageFileGetMetadataFromBuf(const char *path, virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, - int format, - int *backingFormat) + int format) { ssize_t len = VIR_STORAGE_MAX_HEADER; struct stat sb; - int dummy; g_autofree char *buf = NULL; g_autoptr(virStorageSource) meta = NULL; - if (!backingFormat) - backingFormat = &dummy; - - *backingFormat = VIR_STORAGE_FILE_NONE; - if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), path); @@ -1206,9 +1199,6 @@ virStorageFileGetMetadataFromFD(const char *path, if (virStorageFileGetMetadataInternal(meta, buf, len) < 0) return NULL; - if (backingFormat) - *backingFormat = meta->backingStoreRawFormat; - if (S_ISREG(sb.st_mode)) meta->type = VIR_STORAGE_TYPE_FILE; else if (S_ISBLK(sb.st_mode)) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 32f2c82147..5b995d54ab 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -381,8 +381,7 @@ int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, - int format, - int *backingFormat); + int format); virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, -- 2.24.1