[PATCH 00/15] storage_file_probe: Fix ancient bug in qcow2 header extensions parser and refactor the image probing callbacks
Patch 3 fixes an almost 15 year old bug in the qcow2 header extension parser which breaks when the qcow2 image has more than 1 header extension. For us it caused problems for qcow2 images with data file and backing file which we didn't use before, and the only field we cared about was always put first by qemu. Ironically we did have a test file that had such config but it was missed in the test output. Patches 1, 2 are refinment of debug tools I used to see what's happening. Patch 4 adds bitmaps to some test images. We don't parse them but just to be sure. The rest of the series refactors the metadata parser callbacks with the end goal to not parse the qcow2 header extensions twice. Peter Krempa (15): qcow2GetExtensions: Add debug logs for interesting fields in qcow2 header extension parser virstoragetest: Reformat output to hilight dataFile relationship storage_file_probe: qcow2GetExtensions: Fix qcow2 header extension parsing virstoragetest: Add qcow2 bitmaps to some images storage_file_probe: Add image specific callback taking the whole virStorageSource storage_file_probe: Refactor cowGetBackingStore into cowGetImageSpecific storage_file_probe: Refactor qedGetBackingStore into qedGetImageSpecific storage_file_probe: Refactor vmdk4GetBackingStore into vmdk4GetImageSpecific storage_file_probe: Refactor qcowXGetBackingStore into specific callbacks for qcow and qcow2 storage_file_probe: Move logic from qcow2GetClusterSize to qcow2GetImageSpecific storage_file_probe: Move qcow2GetFeatures(ProcessGroup) functions storage_file_probe: Call qcow2GetFeatures from qcow2GetImageSpecific storage_file_probe: Parse all qcow2 extensions at once storage_file_probe: Move setting of 'compat' attribute to qcow2GetFeatures storage_file_probe: Remove unused image probing callbacks src/storage_file/storage_file_probe.c | 439 ++++++++---------- tests/virstoragetest.c | 20 +- .../virstoragetestdata/images/datafile.qcow2 | Bin 327680 -> 393256 bytes .../images/qcow2_qcow2-qcow2_qcow2-auto.qcow2 | Bin 196616 -> 327720 bytes .../images/qcow2datafile-datafile.qcow2 | Bin 196616 -> 327720 bytes tests/virstoragetestdata/out/directory-dir | 1 + tests/virstoragetestdata/out/directory-none | 1 + tests/virstoragetestdata/out/directory-raw | 1 + .../out/qcow2-auto_qcow2-qcow2_raw-raw | 1 + .../out/qcow2-auto_raw-raw-relative | 1 + tests/virstoragetestdata/out/qcow2-datafile | 15 +- .../out/qcow2-protocol-backing-file | 2 + .../out/qcow2-protocol-backing-nbd | 2 + .../out/qcow2-qcow2_nbd-raw | 2 + .../out/qcow2-qcow2_qcow2-auto | 2 + .../out/qcow2-qcow2_qcow2-qcow2_qcow2-auto | 3 + .../out/qcow2-qcow2_qcow2-qcow2_raw-auto | 3 + .../out/qcow2-qcow2_qcow2-qcow2_raw-raw | 3 + .../out/qcow2-qcow2_raw-raw-relative | 2 + tests/virstoragetestdata/out/qcow2-symlinks | 3 + .../out/qcow2datafile-qcow2_qcow2-datafile | 24 +- tests/virstoragetestdata/out/qed-auto_raw | 1 + tests/virstoragetestdata/out/qed-qed_raw | 2 + tests/virstoragetestdata/out/raw-auto | 1 + tests/virstoragetestdata/out/raw-raw | 1 + 25 files changed, 263 insertions(+), 267 deletions(-) -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Add debug statements which were useful in figuring out bugs in the qcow2 extension parser. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index f5c50d4597..04a2dcd9aa 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -442,6 +442,10 @@ qcow2GetExtensions(const char *buf, * is stored at QCOW2v3_HDR_SIZE */ extension_end = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET); + + VIR_DEBUG("extension_start:%zu, extension_end:%zu, buf_size:%zu", + extension_start, extension_end, buf_size); + if (extension_end > buf_size) return -1; @@ -461,6 +465,8 @@ qcow2GetExtensions(const char *buf, unsigned int magic = virReadBufInt32BE(buf + offset); unsigned int len = virReadBufInt32BE(buf + offset + 4); + VIR_DEBUG("offset:%zu, len:%u, magic:0x%x", offset, len, magic); + offset += 8; if ((offset + len) < offset) -- 2.49.0
On a Thursday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Add debug statements which were useful in figuring out bugs in the qcow2 extension parser.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> Move the 'dataFileRaw' field to the main block as it's based on the data in the qcow2 header same as 'backingStoreRaw'. Indent and annotate the corresponding dataFile block to show where it belongs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 20 ++++++++++--------- tests/virstoragetestdata/out/directory-dir | 1 + tests/virstoragetestdata/out/directory-none | 1 + tests/virstoragetestdata/out/directory-raw | 1 + .../out/qcow2-auto_qcow2-qcow2_raw-raw | 1 + .../out/qcow2-auto_raw-raw-relative | 1 + tests/virstoragetestdata/out/qcow2-datafile | 15 +++++++------- .../out/qcow2-protocol-backing-file | 2 ++ .../out/qcow2-protocol-backing-nbd | 2 ++ .../out/qcow2-qcow2_nbd-raw | 2 ++ .../out/qcow2-qcow2_qcow2-auto | 2 ++ .../out/qcow2-qcow2_qcow2-qcow2_qcow2-auto | 3 +++ .../out/qcow2-qcow2_qcow2-qcow2_raw-auto | 3 +++ .../out/qcow2-qcow2_qcow2-qcow2_raw-raw | 3 +++ .../out/qcow2-qcow2_raw-raw-relative | 2 ++ tests/virstoragetestdata/out/qcow2-symlinks | 3 +++ .../out/qcow2datafile-qcow2_qcow2-datafile | 16 +++++++-------- tests/virstoragetestdata/out/qed-auto_raw | 1 + tests/virstoragetestdata/out/qed-qed_raw | 2 ++ tests/virstoragetestdata/out/raw-auto | 1 + tests/virstoragetestdata/out/raw-raw | 1 + 21 files changed, 58 insertions(+), 25 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 78dc644637..e568bd3141 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -179,12 +179,14 @@ testStorageChain(const void *args) for (elt = meta; virStorageSourceIsBacking(elt); elt = elt->backingStore) { g_autofree char *strippedPath = virTestStablePath(elt->path); g_autofree char *strippedBackingStoreRaw = virTestStablePath(elt->backingStoreRaw); + g_autofree char *strippedDataFileRaw = virTestStablePath(elt->dataFileRaw); g_autofree char *strippedRelPath = virTestStablePath(elt->relPath); virBufferAsprintf(&buf, "path:%s\n" "backingStoreRaw: %s\n" "backingStoreRawFormat: %s(%d)\n" + "dataFileRaw: %s\n" "capacity: %lld\n" "encryption: %d\n" "relPath:%s\n" @@ -196,6 +198,7 @@ testStorageChain(const void *args) strippedBackingStoreRaw, NULLSTR(virStorageFileFormatTypeToString(elt->backingStoreRawFormat)), elt->backingStoreRawFormat, + strippedDataFileRaw, elt->capacity, !!elt->encryption, strippedRelPath, @@ -205,18 +208,17 @@ testStorageChain(const void *args) NULLSTR(elt->nhosts ? elt->hosts[0].name : NULL)); if (elt->dataFileStore) { - g_autofree char *strippedPathDataFileRaw = virTestStablePath(elt->dataFileRaw); g_autofree char *strippedPathDataFile = virTestStablePath(elt->dataFileStore->path); virBufferAsprintf(&buf, - "dataFileRaw: %s\n\n\n" - "dataFileStoreSource:\n" - "path: %s\n" - "capacity: %lld\n" - "encryption: %d\n" - "type:%s\n" - "format:%s\n", - strippedPathDataFileRaw, + " dataFileStoreSource for '%s':\n" + " path: %s\n" + " capacity: %lld\n" + " encryption: %d\n" + " type:%s\n" + " format:%s\n" + "\n\n", + strippedPath, strippedPathDataFile, elt->dataFileStore->capacity, !!elt->dataFileStore->encryption, diff --git a/tests/virstoragetestdata/out/directory-dir b/tests/virstoragetestdata/out/directory-dir index c6a2fa3673..ea3bb90191 100644 --- a/tests/virstoragetestdata/out/directory-dir +++ b/tests/virstoragetestdata/out/directory-dir @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/ backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/directory-none b/tests/virstoragetestdata/out/directory-none index c6a2fa3673..ea3bb90191 100644 --- a/tests/virstoragetestdata/out/directory-none +++ b/tests/virstoragetestdata/out/directory-none @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/ backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/directory-raw b/tests/virstoragetestdata/out/directory-raw index 6e190c97f4..a0b9f882ed 100644 --- a/tests/virstoragetestdata/out/directory-raw +++ b/tests/virstoragetestdata/out/directory-raw @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/ backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/qcow2-auto_qcow2-qcow2_raw-raw b/tests/virstoragetestdata/out/qcow2-auto_qcow2-qcow2_raw-raw index 0540be0c09..14d9eb987b 100644 --- a/tests/virstoragetestdata/out/qcow2-auto_qcow2-qcow2_raw-raw +++ b/tests/virstoragetestdata/out/qcow2-auto_qcow2-qcow2_raw-raw @@ -1,6 +1,7 @@ path:ABS_BUILDDIR/virstoragedata/wrap backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/qcow2-auto_raw-raw-relative b/tests/virstoragetestdata/out/qcow2-auto_raw-raw-relative index e145cca417..b99f64ee97 100644 --- a/tests/virstoragetestdata/out/qcow2-auto_raw-raw-relative +++ b/tests/virstoragetestdata/out/qcow2-auto_raw-raw-relative @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2_raw-raw-relative.qcow2 backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/qcow2-datafile b/tests/virstoragetestdata/out/qcow2-datafile index d0f46f7e1b..c395f7f65c 100644 --- a/tests/virstoragetestdata/out/qcow2-datafile +++ b/tests/virstoragetestdata/out/qcow2-datafile @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/datafile.qcow2 backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: raw capacity: 1024 encryption: 0 relPath:<null> @@ -9,12 +10,10 @@ format:qcow2 protocol:none hostname:<null> -dataFileRaw: raw - + dataFileStoreSource for 'ABS_SRCDIR/virstoragetestdata/images/datafile.qcow2': + path: ABS_SRCDIR/virstoragetestdata/images/raw + capacity: 0 + encryption: 0 + type:file + format:raw -dataFileStoreSource: -path: ABS_SRCDIR/virstoragetestdata/images/raw -capacity: 0 -encryption: 0 -type:file -format:raw diff --git a/tests/virstoragetestdata/out/qcow2-protocol-backing-file b/tests/virstoragetestdata/out/qcow2-protocol-backing-file index d9d4c1316c..d0bf411552 100644 --- a/tests/virstoragetestdata/out/qcow2-protocol-backing-file +++ b/tests/virstoragetestdata/out/qcow2-protocol-backing-file @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2-protocol-backing-file.qcow2 backingStoreRaw: raw backingStoreRawFormat: raw(1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:raw diff --git a/tests/virstoragetestdata/out/qcow2-protocol-backing-nbd b/tests/virstoragetestdata/out/qcow2-protocol-backing-nbd index 360a496ab0..d41a8538a6 100644 --- a/tests/virstoragetestdata/out/qcow2-protocol-backing-nbd +++ b/tests/virstoragetestdata/out/qcow2-protocol-backing-nbd @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2-protocol-backing-nbd.qcow2 backingStoreRaw: nbd+tcp://example.org:6000/blah backingStoreRawFormat: raw(1) +dataFileRaw: <null> capacity: 10485760 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:blah backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/qcow2-qcow2_nbd-raw b/tests/virstoragetestdata/out/qcow2-qcow2_nbd-raw index 0c2bb0ddc4..1ec84d771e 100644 --- a/tests/virstoragetestdata/out/qcow2-qcow2_nbd-raw +++ b/tests/virstoragetestdata/out/qcow2-qcow2_nbd-raw @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2_nbd-raw.qcow2 backingStoreRaw: nbd+tcp://example.org:6000/blah backingStoreRawFormat: raw(1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:blah backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-auto b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-auto index 58f1dd6d9e..4b6b4e2283 100644 --- a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-auto +++ b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-auto @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2_qcow2-auto.qcow2 backingStoreRaw: qcow2 backingStoreRawFormat: <null>(-1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/qcow2 backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:qcow2 diff --git a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_qcow2-auto b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_qcow2-auto index 81263c4bc0..79943f432e 100644 --- a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_qcow2-auto +++ b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_qcow2-auto @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2_qcow2-qcow2_qcow2-auto.qcow2 backingStoreRaw: qcow2_qcow2-auto.qcow2 backingStoreRawFormat: qcow2(14) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/qcow2_qcow2-auto.qcow2 backingStoreRaw: qcow2 backingStoreRawFormat: <null>(-1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:qcow2_qcow2-auto.qcow2 @@ -23,6 +25,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/qcow2 backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:qcow2 diff --git a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-auto b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-auto index cbb8d6a33f..02e2c5a966 100644 --- a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-auto +++ b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-auto @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2_qcow2-qcow2_raw-auto.qcow2 backingStoreRaw: qcow2_raw-auto.qcow2 backingStoreRawFormat: qcow2(14) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/qcow2_raw-auto.qcow2 backingStoreRaw: raw backingStoreRawFormat: <null>(-1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:qcow2_raw-auto.qcow2 @@ -23,6 +25,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:raw diff --git a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-raw b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-raw index de11029922..843a013ef5 100644 --- a/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-raw +++ b/tests/virstoragetestdata/out/qcow2-qcow2_qcow2-qcow2_raw-raw @@ -1,6 +1,7 @@ path:ABS_BUILDDIR/virstoragedata/wrap backingStoreRaw: ABS_BUILDDIR/virstoragedata/qcow2 backingStoreRawFormat: qcow2(14) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_BUILDDIR/virstoragedata/qcow2 backingStoreRaw: ABS_BUILDDIR/virstoragedata/raw backingStoreRawFormat: raw(1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -23,6 +25,7 @@ hostname:<null> path:ABS_BUILDDIR/virstoragedata/raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/qcow2-qcow2_raw-raw-relative b/tests/virstoragetestdata/out/qcow2-qcow2_raw-raw-relative index b4bfd432ad..33526cfd48 100644 --- a/tests/virstoragetestdata/out/qcow2-qcow2_raw-raw-relative +++ b/tests/virstoragetestdata/out/qcow2-qcow2_raw-raw-relative @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2_raw-raw-relative.qcow2 backingStoreRaw: raw backingStoreRawFormat: raw(1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:raw diff --git a/tests/virstoragetestdata/out/qcow2-symlinks b/tests/virstoragetestdata/out/qcow2-symlinks index f053f95fd7..c74cf6be53 100644 --- a/tests/virstoragetestdata/out/qcow2-symlinks +++ b/tests/virstoragetestdata/out/qcow2-symlinks @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/sub/link2 backingStoreRaw: ../sub/link1 backingStoreRawFormat: qcow2(14) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/sub/../sub/link1 backingStoreRaw: ../raw backingStoreRawFormat: raw(1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:../sub/link1 @@ -23,6 +25,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/sub/../sub/../raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:../raw diff --git a/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile b/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile index cc8bac3138..c5a5d99e9e 100644 --- a/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile +++ b/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2datafile-datafile.qcow2 backingStoreRaw: datafile.qcow2 backingStoreRawFormat: qcow2(14) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/datafile.qcow2 backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: raw capacity: 1024 encryption: 0 relPath:datafile.qcow2 @@ -20,12 +22,10 @@ format:qcow2 protocol:none hostname:<null> -dataFileRaw: raw - + dataFileStoreSource for 'ABS_SRCDIR/virstoragetestdata/images/datafile.qcow2': + path: ABS_SRCDIR/virstoragetestdata/images/raw + capacity: 0 + encryption: 0 + type:file + format:raw -dataFileStoreSource: -path: ABS_SRCDIR/virstoragetestdata/images/raw -capacity: 0 -encryption: 0 -type:file -format:raw diff --git a/tests/virstoragetestdata/out/qed-auto_raw b/tests/virstoragetestdata/out/qed-auto_raw index 260c3fbc79..3905398142 100644 --- a/tests/virstoragetestdata/out/qed-auto_raw +++ b/tests/virstoragetestdata/out/qed-auto_raw @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qed_raw-raw-relative backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/qed-qed_raw b/tests/virstoragetestdata/out/qed-qed_raw index a13d407a5f..5ae7fef357 100644 --- a/tests/virstoragetestdata/out/qed-qed_raw +++ b/tests/virstoragetestdata/out/qed-qed_raw @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qed_raw-raw-relative backingStoreRaw: raw backingStoreRawFormat: raw(1) +dataFileRaw: <null> capacity: 1024 encryption: 0 relPath:<null> @@ -12,6 +13,7 @@ hostname:<null> path:ABS_SRCDIR/virstoragetestdata/images/raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:raw diff --git a/tests/virstoragetestdata/out/raw-auto b/tests/virstoragetestdata/out/raw-auto index 70ec22f309..5eb1d71681 100644 --- a/tests/virstoragetestdata/out/raw-auto +++ b/tests/virstoragetestdata/out/raw-auto @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> diff --git a/tests/virstoragetestdata/out/raw-raw b/tests/virstoragetestdata/out/raw-raw index 70ec22f309..5eb1d71681 100644 --- a/tests/virstoragetestdata/out/raw-raw +++ b/tests/virstoragetestdata/out/raw-raw @@ -1,6 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/raw backingStoreRaw: <null> backingStoreRawFormat: none(0) +dataFileRaw: <null> capacity: 0 encryption: 0 relPath:<null> -- 2.49.0
In the commit summary: s/hilight/highlight/ On a Thursday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Move the 'dataFileRaw' field to the main block as it's based on the data in the qcow2 header same as 'backingStoreRaw'.
Indent and annotate the corresponding dataFile block to show where it belongs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 20 ++++++++++--------- tests/virstoragetestdata/out/directory-dir | 1 + tests/virstoragetestdata/out/directory-none | 1 + tests/virstoragetestdata/out/directory-raw | 1 + .../out/qcow2-auto_qcow2-qcow2_raw-raw | 1 + .../out/qcow2-auto_raw-raw-relative | 1 + tests/virstoragetestdata/out/qcow2-datafile | 15 +++++++------- .../out/qcow2-protocol-backing-file | 2 ++ .../out/qcow2-protocol-backing-nbd | 2 ++ .../out/qcow2-qcow2_nbd-raw | 2 ++ .../out/qcow2-qcow2_qcow2-auto | 2 ++ .../out/qcow2-qcow2_qcow2-qcow2_qcow2-auto | 3 +++ .../out/qcow2-qcow2_qcow2-qcow2_raw-auto | 3 +++ .../out/qcow2-qcow2_qcow2-qcow2_raw-raw | 3 +++ .../out/qcow2-qcow2_raw-raw-relative | 2 ++ tests/virstoragetestdata/out/qcow2-symlinks | 3 +++ .../out/qcow2datafile-qcow2_qcow2-datafile | 16 +++++++-------- tests/virstoragetestdata/out/qed-auto_raw | 1 + tests/virstoragetestdata/out/qed-qed_raw | 2 ++ tests/virstoragetestdata/out/raw-auto | 1 + tests/virstoragetestdata/out/raw-raw | 1 + 21 files changed, 58 insertions(+), 25 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> There are 3 bugs in the qcow2 header extension parser: 1) Header extension padding not taken into account Per qcow2 documentation (qemu.git/docs/interop/qcow2.txt, also now mirrored in the comment explaining the parser) each header extension entry is padded to a multiple of 8 bytes. Our parser didn't take the padding into account and advanced the current offset only by the 'length', which corresponds to the length of the data. This meant that in vast majority of cases only the first extension would be parsed correctly. For any other one we'd try to fetch the magic and length from wrong place. Luckily this wasn't problem for most of the almost 15 years this bug existed as we only cared about the backing file format string header which was always stored first by qemu. It is now a problem in the very specific case when a qcow2 image has a 'data-file' and also a backing store with format. In such case we'd parse the backing store format properly as it's the first header and 'data-file' being the second would be skipped. The buffer bounds checks were correct so we didn't violate any memory boundaries. 2) Integer underflow in calculation of end of header extension block If the image doesn't have a backing image, the 'backing_file_offset' qcow2 header field is 0. We use that value as 'extensions_end' which is used in the while loop to parse the extension entries. The check was done as "offset < (extensions_end - 8)", thus it unreflows when there's no filename. The design of the loop prevented anything bad from happening though. 3) Off-by-one when determining end of header extension block The aforementioned end of extension check above also has an off-by-one error as it allowed another loop if more than 8 bytes were available. Now the termination entry has data length of 0 bytes so we'd not be able to properly process that one. This wasn't a problem either as for now there's just the terminator having 0 byte lenght. This patch improves documentation by quoting the qcow2 interoperability document and adjusts the loop condition and lenght handling to comply with the specs. Interestingly we also had a test case for this specific scenario but the expected test output was wrong. Fixes: a93402d48b2996e5300754d299ef0d3f646aa098 Resolves: https://issues.redhat.com/browse/RHEL-93775 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 62 +++++++++---------- .../out/qcow2datafile-qcow2_qcow2-datafile | 10 ++- 2 files changed, 40 insertions(+), 32 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 04a2dcd9aa..f8857fa934 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -415,31 +415,21 @@ qcow2GetExtensions(const char *buf, extension_start = virReadBufInt32BE(buf + QCOW2v3_HDR_SIZE); /* - * Traditionally QCow2 files had a layout of - * - * [header] - * [backingStoreName] - * - * Although the backingStoreName typically followed - * the header immediately, this was not required by - * the format. By specifying a higher byte offset for - * the backing file offset in the header, it was - * possible to leave space between the header and - * start of backingStore. - * - * This hack is now used to store extensions to the - * qcow2 format: + * QCow2 header extensions are stored directly after the header before + * the (optional) backing store filename: * * [header] * [extensions] * [backingStoreName] * - * Thus the file region to search for extensions is - * between the end of the header (QCOW2_HDR_TOTAL_SIZE) - * and the start of the backingStoreName (offset) + * For qcow2(v2) the [header] portion has a fixed size (QCOW2_HDR_TOTAL_SIZE), + * whereas for qcow2v3 the size of the header itself is recorded inside + * the header (at offset QCOW2v3_HDR_SIZE). * - * for qcow2 v3 images, the length of the header - * is stored at QCOW2v3_HDR_SIZE + * Thus the file region to search for header extensions is + * between the end of the header and the start of the backingStoreName + * (QCOWX_HDR_BACKING_FILE_OFFSET) if a backing file is present (as + * otherwise the value at QCOWX_HDR_BACKING_FILE_OFFSET is 0) */ extension_end = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET); @@ -449,19 +439,28 @@ qcow2GetExtensions(const char *buf, if (extension_end > buf_size) return -1; - /* - * The extensions take format of - * - * int32: magic - * int32: length - * byte[length]: payload - * - * Unknown extensions can be ignored by skipping - * over "length" bytes in the data stream. - */ offset = extension_start; while (offset < (buf_size-8) && - offset < (extension_end-8)) { + (extension_end == 0 || offset <= (extension_end - 8))) { + /** + * Directly after the image header, optional sections called header + * extensions can + * be stored. Each extension has a structure like the following: + * + * Byte 0 - 3: Header extension type: + * 0x00000000 - End of the header extension area + * 0xe2792aca - Backing file format name string + * 0x6803f857 - Feature name table + * 0x23852875 - Bitmaps extension + * 0x0537be77 - Full disk encryption header pointer + * 0x44415441 - External data file name string + * other - Unknown header extension, can be safely ignored + * + * 4 - 7: Length of the header extension data + * 8 - n: Header extension data + * n - m: Padding to round up the header extension size to the + * next multiple of 8. + */ unsigned int magic = virReadBufInt32BE(buf + offset); unsigned int len = virReadBufInt32BE(buf + offset + 4); @@ -510,7 +509,8 @@ qcow2GetExtensions(const char *buf, return 0; } - offset += len; + /* take padding into account; see above */ + offset += VIR_ROUND_UP(len, 8); } return 0; diff --git a/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile b/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile index c5a5d99e9e..a200ba98fa 100644 --- a/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile +++ b/tests/virstoragetestdata/out/qcow2datafile-qcow2_qcow2-datafile @@ -1,7 +1,7 @@ path:ABS_SRCDIR/virstoragetestdata/images/qcow2datafile-datafile.qcow2 backingStoreRaw: datafile.qcow2 backingStoreRawFormat: qcow2(14) -dataFileRaw: <null> +dataFileRaw: raw capacity: 1024 encryption: 0 relPath:<null> @@ -10,6 +10,14 @@ format:qcow2 protocol:none hostname:<null> + dataFileStoreSource for 'ABS_SRCDIR/virstoragetestdata/images/qcow2datafile-datafile.qcow2': + path: ABS_SRCDIR/virstoragetestdata/images/raw + capacity: 0 + encryption: 0 + type:file + format:raw + + path:ABS_SRCDIR/virstoragetestdata/images/datafile.qcow2 backingStoreRaw: <null> backingStoreRawFormat: none(0) -- 2.49.0
On a Thursday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
There are 3 bugs in the qcow2 header extension parser:
1) Header extension padding not taken into account
Per qcow2 documentation (qemu.git/docs/interop/qcow2.txt, also now mirrored in the comment explaining the parser) each header extension entry is padded to a multiple of 8 bytes.
Our parser didn't take the padding into account and advanced the current offset only by the 'length', which corresponds to the length of the data.
This meant that in vast majority of cases only the first extension would be parsed correctly. For any other one we'd try to fetch the magic and length from wrong place.
Luckily this wasn't problem for most of the almost 15 years this bug
a problem
existed as we only cared about the backing file format string header which was always stored first by qemu.
It is now a problem in the very specific case when a qcow2 image has a 'data-file' and also a backing store with format. In such case we'd parse the backing store format properly as it's the first header and 'data-file' being the second would be skipped.
The buffer bounds checks were correct so we didn't violate any memory boundaries.
2) Integer underflow in calculation of end of header extension block
If the image doesn't have a backing image, the 'backing_file_offset' qcow2 header field is 0. We use that value as 'extensions_end' which is used in the while loop to parse the extension entries.
The check was done as "offset < (extensions_end - 8)", thus it unreflows when there's no filename.
The design of the loop prevented anything bad from happening though.
3) Off-by-one when determining end of header extension block
The aforementioned end of extension check above also has an off-by-one error as it allowed another loop if more than 8 bytes were available.
Now the termination entry has data length of 0 bytes so we'd not be able to properly process that one.
This wasn't a problem either as for now there's just the terminator having 0 byte lenght.
length
This patch improves documentation by quoting the qcow2 interoperability document and adjusts the loop condition and lenght handling to comply with the specs.
length
Interestingly we also had a test case for this specific scenario but the expected test output was wrong.
Fixes: a93402d48b2996e5300754d299ef0d3f646aa098 Resolves: https://issues.redhat.com/browse/RHEL-93775 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 62 +++++++++---------- .../out/qcow2datafile-qcow2_qcow2-datafile | 10 ++- 2 files changed, 40 insertions(+), 32 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> Add change block tracking bitmaps to some of the qcow2 images, to ensure that they work with our qcow2 header parser even when we don't parse them ourselves. The existing images were modified by running: $ qemu-img bitmap --add qcow2_qcow2-qcow2_qcow2-auto.qcow2 testbitmap $ qemu-img bitmap --add qcow2datafile-datafile.qcow2 testbitmap $ qemu-img bitmap --add datafile.qcow2 testbitmap in tests/virstoragetestdata/images/. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../virstoragetestdata/images/datafile.qcow2 | Bin 327680 -> 393256 bytes .../images/qcow2_qcow2-qcow2_qcow2-auto.qcow2 | Bin 196616 -> 327720 bytes .../images/qcow2datafile-datafile.qcow2 | Bin 196616 -> 327720 bytes 3 files changed, 0 insertions(+), 0 deletions(-) diff --git a/tests/virstoragetestdata/images/datafile.qcow2 b/tests/virstoragetestdata/images/datafile.qcow2 index 32f3acff8600b169311c9a9ea17c2b12f3450e0c..5a8bf617ab622a21e236b2fd12a2fe302b7f4657 100644 GIT binary patch delta 109 zcmZo@5LwY6F(ICjd1Jx~MmgnHjZy{%1_=fR21W)1&;T>pHh*Ni|F@NuaVjel+g5g_ rum4+^TUa(RvHn}c#H7p$Rs+(-#3;bP1>%*Y7MCPtmgFWDK#T$a1b!9! delta 44 zcmZ3{AkokuG9jLkX=B0)#?1~)NB>TCV6tdsWt_^&#J1Up>GI#r4opf+EzB(}0A)Q6 A(EtDd diff --git a/tests/virstoragetestdata/images/qcow2_qcow2-qcow2_qcow2-auto.qcow2 b/tests/virstoragetestdata/images/qcow2_qcow2-qcow2_qcow2-auto.qcow2 index 8b9c3cedeacf4aef8c7645314ecb68c992d4b51d..44e175552f172244dc82392c07fa17a12d1197cf 100644 GIT binary patch delta 129 zcmeBZ;91ciA`s~2`I~`(ftdjem<%Qg_%kwYOjyAvr`)Ph%D})N!N9=42$2N?4KT^N znT_fG-&R(}sjN&at*lI&9hi>(g%L$8$}C`2Abm`X0t{RrUP)?kNm6D>ZejsgjsXBZ CIv8&N delta 62 zcmZ3{AkxvmBM|83`I~`(ftdjem;@#Y_)iRA*_gbPabf_+W&@_He<vp}DYUXOPGx0c P*&M{Q`S0cgCLbmM3z!kw diff --git a/tests/virstoragetestdata/images/qcow2datafile-datafile.qcow2 b/tests/virstoragetestdata/images/qcow2datafile-datafile.qcow2 index 1fce3ffbdf44f2c236ba8b455fb2cbe8f55f829b..2310e3fb67ca5e6945888f4272d184ce322a5304 100644 GIT binary patch delta 126 zcmeBZ;91ciA`s~2`I~`(ftdjem>ebw_%kwYOxVFFr`)Ph%D})N!N9=42$2N?4KT^N zS&ZrY-&R(}sjN&at*lI&9hi>(g%L$86aCp_L7JEt1sJ$M8cI@&OOi56auW+6rT_pc COc`qc delta 62 zcmZ3{AkxvmBM|83`I~`(ftdjem=q=o_)k2*vN3rp<HQ3Tn;n>r{+*n_q|nOBIF*%& PWpfbI=D(X0n0%N3E4UKc -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> The callbacks getting just some fields are not flexible and in some cases cause the metadata to be probed multiple times. Add a callback that will pass the whole virStorageSource struct being probed so that the code can be written more efficiently. As a first step we add just the callback. The specific format helpers will be refactored and subsequently all the other callbacks will be removed. To simplify the refactors that will convert all the code to the new callbacks the new callback is placed first but the calls to cleanup previous metadata are moved before it. They'll be removed once the refactors are complete together with the other callbacks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 49 ++++++++++++++++----------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index f8857fa934..29837792e4 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -96,6 +96,9 @@ struct FileTypeInfo { int (*getDataFile)(char **res, virBitmap *features, char *buf, size_t buf_size); int (*getFeatures)(virBitmap **features, int format, char *buf, ssize_t len); + int (*getImageSpecific)(virStorageSource *meta, + const char *buf, + size_t buf_size); }; @@ -241,18 +244,18 @@ static struct FileEncryptionInfo const qcow2EncryptionInfo[] = { static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_RAW] = { 0, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, 0, 0, 0, luksEncryptionInfo, - NULL, NULL, NULL, NULL }, + NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_DIR] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, 64, 4, {0x20000}, - 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL, NULL, NULL + 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_CLOOP] = { /* #!/bin/sh @@ -261,7 +264,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, @@ -269,45 +272,45 @@ static struct FileTypeInfo const fileTypeInfo[] = { * would have to match) but then disables that check. */ 0, NULL, 0, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", LV_LITTLE_ENDIAN, -2, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VPC] = { 0, "conectix", LV_BIG_ENDIAN, 12, 4, {0x10000}, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL, NULL, NULL + 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL, NULL, NULL, NULL }, /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { 64, "\x7f\x10\xda\xbe", LV_LITTLE_ENDIAN, 68, 4, {0x00010001}, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL, NULL, NULL}, + 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL, NULL, NULL, NULL}, /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", LV_LITTLE_ENDIAN, -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 8, - PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL, NULL, NULL }, + PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL, NULL, NULL, NULL }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { 0, "OOOM", LV_BIG_ENDIAN, 4, 4, {2}, - 4+4+1024+4, 8, 1, NULL, NULL, cowGetBackingStore, NULL, NULL + 4+4+1024+4, 8, 1, NULL, NULL, cowGetBackingStore, NULL, NULL, NULL }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", LV_BIG_ENDIAN, 4, 4, {1}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow1EncryptionInfo, - NULL, qcowXGetBackingStore, NULL, NULL + NULL, qcowXGetBackingStore, NULL, NULL, NULL }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", @@ -317,18 +320,19 @@ static struct FileTypeInfo const fileTypeInfo[] = { qcow2GetClusterSize, qcowXGetBackingStore, qcow2GetDataFile, - qcow2GetFeatures + qcow2GetFeatures, + NULL }, [VIR_STORAGE_FILE_QED] = { /* https://wiki.qemu.org/Features/QED */ 0, "QED", LV_LITTLE_ENDIAN, -2, 0, {0}, - QED_HDR_IMAGE_SIZE, 8, 1, NULL, NULL, qedGetBackingStore, NULL, NULL + QED_HDR_IMAGE_SIZE, 8, 1, NULL, NULL, qedGetBackingStore, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", LV_LITTLE_ENDIAN, 4, 4, {1, 2, 3}, - 4+4+4, 8, 512, NULL, NULL, vmdk4GetBackingStore, NULL, NULL + 4+4+4, 8, 512, NULL, NULL, vmdk4GetBackingStore, NULL, NULL, NULL }, }; G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST); @@ -991,22 +995,27 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier; } + VIR_FREE(meta->backingStoreRaw); + g_clear_pointer(&meta->features, virBitmapFree); + VIR_FREE(meta->dataFileRaw); + + if (fileTypeInfo[meta->format].getImageSpecific && + fileTypeInfo[meta->format].getImageSpecific(meta, buf, len) < 0) + return -1; + if (fileTypeInfo[meta->format].getClusterSize != NULL) meta->clusterSize = fileTypeInfo[meta->format].getClusterSize(buf, len); - VIR_FREE(meta->backingStoreRaw); if (fileTypeInfo[meta->format].getBackingStore != NULL) { fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, &format, buf, len); meta->backingStoreRawFormat = format; } - g_clear_pointer(&meta->features, virBitmapFree); if (fileTypeInfo[meta->format].getFeatures != NULL && fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0) return -1; - VIR_FREE(meta->dataFileRaw); if (fileTypeInfo[meta->format].getDataFile != NULL) { fileTypeInfo[meta->format].getDataFile(&meta->dataFileRaw, meta->features, buf, len); -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Rename the function, adjust parameters and fix the code to fill the virStorageSource fields directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 29837792e4..08fd3f2265 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -102,8 +102,10 @@ struct FileTypeInfo { }; -static int cowGetBackingStore(char **, int *, - const char *, size_t); +static int +cowGetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size); static unsigned long long qcow2GetClusterSize(const char *buf, size_t buf_size); @@ -303,7 +305,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_COW] = { 0, "OOOM", LV_BIG_ENDIAN, 4, 4, {2}, - 4+4+1024+4, 8, 1, NULL, NULL, cowGetBackingStore, NULL, NULL, NULL + 4+4+1024+4, 8, 1, NULL, NULL, NULL, NULL, NULL, cowGetImageSpecific }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", @@ -375,23 +377,22 @@ G_STATIC_ASSERT(G_N_ELEMENTS(qcow2IncompatibleFeatureArray) == QCOW2_INCOMPATIBL static int -cowGetBackingStore(char **res, - int *format, - const char *buf, - size_t buf_size) +cowGetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size) { #define COW_FILENAME_MAXLEN 1024 - *res = NULL; - *format = VIR_STORAGE_FILE_AUTO; - if (buf_size < 4+4+ COW_FILENAME_MAXLEN) + g_clear_pointer(&meta->backingStoreRaw, g_free); + + if (buf_size < 4 + 4 + COW_FILENAME_MAXLEN) return 0; - if (buf[4+4] == '\0') { /* cow_header_v2.backing_file[0] */ - *format = VIR_STORAGE_FILE_NONE; + if (buf[4 + 4] == '\0') { /* cow_header_v2.backing_file[0] */ + meta->backingStoreRawFormat = VIR_STORAGE_FILE_NONE; return 0; } - *res = g_strndup((const char *)buf + 4 + 4, COW_FILENAME_MAXLEN); + meta->backingStoreRaw = g_strndup((const char *)buf + 4 + 4, COW_FILENAME_MAXLEN); return 0; } -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Change to the new function prototype and adjust the code to fill the fields in virStorageSource directly. The code also converts the copying of the backing file string from 'g_new0' + 'memcpy' to 'g_strndup' as we treat it as a string later on. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 28 ++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 08fd3f2265..1544dded5c 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -117,7 +117,9 @@ static int qcow2GetFeatures(virBitmap **features, int format, static int vmdk4GetBackingStore(char **, int *, const char *, size_t); static int -qedGetBackingStore(char **, int *, const char *, size_t); +qedGetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size); #define QCOWX_HDR_VERSION (4) #define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4) @@ -329,7 +331,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { /* https://wiki.qemu.org/Features/QED */ 0, "QED", LV_LITTLE_ENDIAN, -2, 0, {0}, - QED_HDR_IMAGE_SIZE, 8, 1, NULL, NULL, qedGetBackingStore, NULL, NULL, NULL + QED_HDR_IMAGE_SIZE, 8, 1, NULL, NULL, NULL, NULL, NULL, qedGetImageSpecific }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", @@ -656,21 +658,22 @@ vmdk4GetBackingStore(char **res, } static int -qedGetBackingStore(char **res, - int *format, - const char *buf, - size_t buf_size) +qedGetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size) { unsigned long long flags; unsigned long offset, size; - *res = NULL; + g_clear_pointer(&meta->backingStoreRaw, g_free); + /* Check if this image has a backing file */ if (buf_size < QED_HDR_FEATURES_OFFSET+8) return 0; + flags = virReadBufInt64LE(buf + QED_HDR_FEATURES_OFFSET); if (!(flags & QED_F_BACKING_FILE)) { - *format = VIR_STORAGE_FILE_NONE; + meta->backingStoreRawFormat = VIR_STORAGE_FILE_NONE; return 0; } @@ -685,14 +688,13 @@ qedGetBackingStore(char **res, return 0; if (offset + size > buf_size || offset + size < offset) return 0; - *res = g_new0(char, size + 1); - memcpy(*res, buf + offset, size); - (*res)[size] = '\0'; + + meta->backingStoreRaw = g_strndup(buf + offset, size); if (flags & QED_F_BACKING_FORMAT_NO_PROBE) - *format = VIR_STORAGE_FILE_RAW; + meta->backingStoreRawFormat = VIR_STORAGE_FILE_RAW; else - *format = VIR_STORAGE_FILE_AUTO_SAFE; + meta->backingStoreRawFormat = VIR_STORAGE_FILE_AUTO_SAFE; return 0; } -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Change to the new function prototype and adjust the code to fill the fields in virStorageSource directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 1544dded5c..e741bccd44 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -114,8 +114,10 @@ static int qcowXGetBackingStore(char **, int *, static int qcow2GetDataFile(char **, virBitmap *, char *, size_t); static int qcow2GetFeatures(virBitmap **features, int format, char *buf, ssize_t len); -static int vmdk4GetBackingStore(char **, int *, - const char *, size_t); +static int +vmdk4GetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size); static int qedGetImageSpecific(virStorageSource *meta, const char *buf, @@ -336,7 +338,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", LV_LITTLE_ENDIAN, 4, 4, {1, 2, 3}, - 4+4+4, 8, 512, NULL, NULL, vmdk4GetBackingStore, NULL, NULL, NULL + 4+4+4, 8, 512, NULL, NULL, NULL, NULL, NULL, vmdk4GetImageSpecific }, }; G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST); @@ -607,10 +609,9 @@ qcow2GetDataFile(char **res, static int -vmdk4GetBackingStore(char **res, - int *format, - const char *buf, - size_t buf_size) +vmdk4GetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size) { static const char prefix[] = "parentFileNameHint=\""; char *start, *end; @@ -619,7 +620,7 @@ vmdk4GetBackingStore(char **res, desc = g_new0(char, VIR_STORAGE_MAX_HEADER); - *res = NULL; + g_clear_pointer(&meta->backingStoreRaw, g_free); /* * Technically this should have been VMDK, since * VMDK spec / VMware impl only support VMDK backed @@ -627,7 +628,7 @@ vmdk4GetBackingStore(char **res, * does probing on VMDK backing files, hence we set * AUTO */ - *format = VIR_STORAGE_FILE_AUTO; + meta->backingStoreRawFormat = VIR_STORAGE_FILE_AUTO; if (buf_size <= 0x200) return 0; @@ -639,7 +640,7 @@ vmdk4GetBackingStore(char **res, desc[len] = '\0'; start = strstr(desc, prefix); if (start == NULL) { - *format = VIR_STORAGE_FILE_NONE; + meta->backingStoreRawFormat = VIR_STORAGE_FILE_NONE; return 0; } start += strlen(prefix); @@ -648,11 +649,11 @@ vmdk4GetBackingStore(char **res, return 0; if (end == start) { - *format = VIR_STORAGE_FILE_NONE; + meta->backingStoreRawFormat = VIR_STORAGE_FILE_NONE; return 0; } *end = '\0'; - *res = g_strdup(start); + meta->backingStoreRaw = g_strdup(start); return 0; } -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Change qcowXGetBackingStore to use the new function prototype which fills virStorageSource directly. Convert the copying of the backing file string from 'g_new0' + 'memcpy' to 'g_strndup' as we treat it as a string. Introduce qcowGetImageSpecific (as a wrapper for qcowXGetBackingStore) and qcow2GetImageSpecific. The latter of the two will be used to collect all the qcow2-specific code later on, but for now it just parses the backing store and the format. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 69 +++++++++++++++++++-------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index e741bccd44..ce8ba4b884 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -106,11 +106,17 @@ static int cowGetImageSpecific(virStorageSource *meta, const char *buf, size_t buf_size); +static int +qcowGetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size); +static int +qcow2GetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size); static unsigned long long qcow2GetClusterSize(const char *buf, size_t buf_size); -static int qcowXGetBackingStore(char **, int *, - const char *, size_t); static int qcow2GetDataFile(char **, virBitmap *, char *, size_t); static int qcow2GetFeatures(virBitmap **features, int format, char *buf, ssize_t len); @@ -316,7 +322,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_BIG_ENDIAN, 4, 4, {1}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow1EncryptionInfo, - NULL, qcowXGetBackingStore, NULL, NULL, NULL + NULL, NULL, NULL, NULL, qcowGetImageSpecific }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", @@ -324,10 +330,10 @@ static struct FileTypeInfo const fileTypeInfo[] = { QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow2EncryptionInfo, qcow2GetClusterSize, - qcowXGetBackingStore, + NULL, qcow2GetDataFile, qcow2GetFeatures, - NULL + qcow2GetImageSpecific }, [VIR_STORAGE_FILE_QED] = { /* https://wiki.qemu.org/Features/QED */ @@ -545,45 +551,66 @@ qcow2GetClusterSize(const char *buf, static int -qcowXGetBackingStore(char **res, - int *format, +qcowXGetBackingStore(virStorageSource *meta, const char *buf, size_t buf_size) { unsigned long long offset; unsigned int size; - *res = NULL; - *format = VIR_STORAGE_FILE_AUTO; + g_clear_pointer(&meta->backingStoreRaw, g_free); + meta->backingStoreRawFormat = VIR_STORAGE_FILE_AUTO; if (buf_size < QCOWX_HDR_BACKING_FILE_OFFSET+8+4) return 0; offset = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET); - if (offset > buf_size) - return 0; + size = virReadBufInt32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE); - if (offset == 0) { - *format = VIR_STORAGE_FILE_NONE; + if (offset == 0 || size == 0) { + meta->backingStoreRawFormat = VIR_STORAGE_FILE_NONE; return 0; } - size = virReadBufInt32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE); - if (size == 0) { - *format = VIR_STORAGE_FILE_NONE; + if (offset > buf_size) return 0; - } if (size > 1023) return 0; if (offset + size > buf_size || offset + size < offset) return 0; - *res = g_new0(char, size + 1); - memcpy(*res, buf + offset, size); - (*res)[size] = '\0'; - if (qcow2GetExtensions(buf, buf_size, format, NULL) < 0) + meta->backingStoreRaw = g_strndup(buf + offset, size); + + return 0; +} + + +static int +qcowGetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size) +{ + return qcowXGetBackingStore(meta, buf, buf_size); +} + + +static int +qcow2GetImageSpecific(virStorageSource *meta, + const char *buf, + size_t buf_size) +{ + int format; + + if (qcowXGetBackingStore(meta, buf, buf_size) < 0) + return -1; + + format = meta->backingStoreRawFormat; + + if (qcow2GetExtensions(buf, buf_size, &format, NULL) < 0) return 0; + meta->backingStoreRawFormat = format; + return 0; } -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Move the cluster size parser into the image specific code for qcow2, which will later allow us to remove the callback for cluster size which is not used by any other format. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 31 ++++++++------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index ce8ba4b884..21a1013102 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -114,9 +114,6 @@ static int qcow2GetImageSpecific(virStorageSource *meta, const char *buf, size_t buf_size); -static unsigned long long -qcow2GetClusterSize(const char *buf, - size_t buf_size); static int qcow2GetDataFile(char **, virBitmap *, char *, size_t); static int qcow2GetFeatures(virBitmap **features, int format, char *buf, ssize_t len); @@ -329,7 +326,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_BIG_ENDIAN, 4, 4, {2, 3}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow2EncryptionInfo, - qcow2GetClusterSize, + NULL, NULL, qcow2GetDataFile, qcow2GetFeatures, @@ -532,24 +529,6 @@ qcow2GetExtensions(const char *buf, } -static unsigned long long -qcow2GetClusterSize(const char *buf, - size_t buf_size) -{ - int clusterBits = 0; - - if ((QCOWX_HDR_CLUSTER_BITS_OFFSET + 4) > buf_size) - return 0; - - clusterBits = virReadBufInt32BE(buf + QCOWX_HDR_CLUSTER_BITS_OFFSET); - - if (clusterBits > 0) - return 1ULL << clusterBits; - - return 0; -} - - static int qcowXGetBackingStore(virStorageSource *meta, const char *buf, @@ -601,6 +580,14 @@ qcow2GetImageSpecific(virStorageSource *meta, { int format; + meta->clusterSize = 0; + if (buf_size > (QCOWX_HDR_CLUSTER_BITS_OFFSET + 4)) { + int clusterBits = virReadBufInt32BE(buf + QCOWX_HDR_CLUSTER_BITS_OFFSET); + + if (clusterBits > 0) + meta->clusterSize = 1ULL << clusterBits; + } + if (qcowXGetBackingStore(meta, buf, buf_size) < 0) return -1; -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Move the two functions to where they will be used. Subsequent patches will refactor the control flow so that they will no longer be declared ahead of time. Moving them in a separate patch will make the changes in the refactor more clear to see. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 96 +++++++++++++-------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 21a1013102..b357fdeb66 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -573,6 +573,54 @@ qcowGetImageSpecific(virStorageSource *meta, } +static void +qcow2GetFeaturesProcessGroup(uint64_t bits, + const virStorageFileFeature *featuremap, + size_t nfeatures, + virBitmap *features) +{ + size_t i; + + for (i = 0; i < nfeatures; i++) { + if ((bits & ((uint64_t) 1 << i)) && + featuremap[i] != VIR_STORAGE_FILE_FEATURE_LAST) + ignore_value(virBitmapSetBit(features, featuremap[i])); + } +} + + +static int +qcow2GetFeatures(virBitmap **features, + int format, + char *buf, + ssize_t len) +{ + int version = -1; + + version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); + + if (version == 2) + return 0; + + if (len < QCOW2v3_HDR_SIZE) + return -1; + + *features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST); + + qcow2GetFeaturesProcessGroup(virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_COMPATIBLE), + qcow2CompatibleFeatureArray, + G_N_ELEMENTS(qcow2CompatibleFeatureArray), + *features); + + qcow2GetFeaturesProcessGroup(virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_INCOMPATIBLE), + qcow2IncompatibleFeatureArray, + G_N_ELEMENTS(qcow2IncompatibleFeatureArray), + *features); + + return 0; +} + + static int qcow2GetImageSpecific(virStorageSource *meta, const char *buf, @@ -832,54 +880,6 @@ virStorageFileProbeFormatFromBuf(const char *path, } -static void -qcow2GetFeaturesProcessGroup(uint64_t bits, - const virStorageFileFeature *featuremap, - size_t nfeatures, - virBitmap *features) -{ - size_t i; - - for (i = 0; i < nfeatures; i++) { - if ((bits & ((uint64_t) 1 << i)) && - featuremap[i] != VIR_STORAGE_FILE_FEATURE_LAST) - ignore_value(virBitmapSetBit(features, featuremap[i])); - } -} - - -static int -qcow2GetFeatures(virBitmap **features, - int format, - char *buf, - ssize_t len) -{ - int version = -1; - - version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); - - if (version == 2) - return 0; - - if (len < QCOW2v3_HDR_SIZE) - return -1; - - *features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST); - - qcow2GetFeaturesProcessGroup(virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_COMPATIBLE), - qcow2CompatibleFeatureArray, - G_N_ELEMENTS(qcow2CompatibleFeatureArray), - *features); - - qcow2GetFeaturesProcessGroup(virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_INCOMPATIBLE), - qcow2IncompatibleFeatureArray, - G_N_ELEMENTS(qcow2IncompatibleFeatureArray), - *features); - - return 0; -} - - static bool virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info, char *buf, -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Parse qcow2 feature flags from qcow2GetImageSpecific. To achieve that qcow2GetFeatures is refactored to take virStorageSource directly and fill the data. To avoid the need to pass 'format' the parsing of the qcow2 version is changed to access the offset directly (same as in qcow2GetExtensions) Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index b357fdeb66..10e159c27b 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -115,8 +115,6 @@ qcow2GetImageSpecific(virStorageSource *meta, const char *buf, size_t buf_size); static int qcow2GetDataFile(char **, virBitmap *, char *, size_t); -static int qcow2GetFeatures(virBitmap **features, int format, - char *buf, ssize_t len); static int vmdk4GetImageSpecific(virStorageSource *meta, const char *buf, @@ -329,7 +327,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { NULL, NULL, qcow2GetDataFile, - qcow2GetFeatures, + NULL, qcow2GetImageSpecific }, [VIR_STORAGE_FILE_QED] = { @@ -590,14 +588,13 @@ qcow2GetFeaturesProcessGroup(uint64_t bits, static int -qcow2GetFeatures(virBitmap **features, - int format, - char *buf, +qcow2GetFeatures(virStorageSource *meta, + const char *buf, ssize_t len) { - int version = -1; + int version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); - version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); + g_clear_pointer(&meta->features, virBitmapFree); if (version == 2) return 0; @@ -605,17 +602,17 @@ qcow2GetFeatures(virBitmap **features, if (len < QCOW2v3_HDR_SIZE) return -1; - *features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST); + meta->features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST); qcow2GetFeaturesProcessGroup(virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_COMPATIBLE), qcow2CompatibleFeatureArray, G_N_ELEMENTS(qcow2CompatibleFeatureArray), - *features); + meta->features); qcow2GetFeaturesProcessGroup(virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_INCOMPATIBLE), qcow2IncompatibleFeatureArray, G_N_ELEMENTS(qcow2IncompatibleFeatureArray), - *features); + meta->features); return 0; } @@ -639,6 +636,9 @@ qcow2GetImageSpecific(virStorageSource *meta, if (qcowXGetBackingStore(meta, buf, buf_size) < 0) return -1; + if (qcow2GetFeatures(meta, buf, buf_size) < 0) + return -1; + format = meta->backingStoreRawFormat; if (qcow2GetExtensions(buf, buf_size, &format, NULL) < 0) -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Refactor qcow2GetExtensions to parse everything at once and directly assign it into fields in the parsed virStorageSource. This removes the need for qcow2GetDataFile as it will be parsed directly. The patch also simplifies the juggling of variables which was needed to parse the backing file format filed, when it was passed via pointer in argument. qcow2GetExtensions is now invoked on qcow2 images so we can remove the version check for qcow(v1) images. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 68 +++++---------------------- 1 file changed, 13 insertions(+), 55 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 10e159c27b..73751c7a2f 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -114,7 +114,6 @@ static int qcow2GetImageSpecific(virStorageSource *meta, const char *buf, size_t buf_size); -static int qcow2GetDataFile(char **, virBitmap *, char *, size_t); static int vmdk4GetImageSpecific(virStorageSource *meta, const char *buf, @@ -326,7 +325,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { qcow2EncryptionInfo, NULL, NULL, - qcow2GetDataFile, + NULL, NULL, qcow2GetImageSpecific }, @@ -403,21 +402,16 @@ cowGetImageSpecific(virStorageSource *meta, static int -qcow2GetExtensions(const char *buf, - size_t buf_size, - int *backingFormat, - char **dataFilePath) +qcow2GetExtensions(virStorageSource *meta, + const char *buf, + size_t buf_size) { size_t offset; size_t extension_start; size_t extension_end; int version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); - if (version < 2) { - /* QCow1 doesn't have the extensions capability - * used to store backing format */ - return 0; - } + g_clear_pointer(&meta->dataFileRaw, g_free); if (version == 2) extension_start = QCOW2_HDR_TOTAL_SIZE; @@ -486,13 +480,7 @@ qcow2GetExtensions(const char *buf, switch (magic) { case QCOW2_HDR_EXTENSION_BACKING_FORMAT: { - g_autofree char *tmp = NULL; - if (!backingFormat) - break; - - tmp = g_new0(char, len + 1); - memcpy(tmp, buf + offset, len); - tmp[len] = '\0'; + g_autofree char *tmp = g_strndup(buf + offset, len); /* qemu and qemu-img allow using the protocol driver name inside * of the format field in cases when the dummy 'raw' driver should @@ -500,20 +488,16 @@ qcow2GetExtensions(const char *buf, * doesn't look like a format driver name to be a protocol driver * directly and thus the image is in fact still considered raw */ - *backingFormat = virStorageFileFormatTypeFromString(tmp); - if (*backingFormat <= VIR_STORAGE_FILE_NONE) - *backingFormat = VIR_STORAGE_FILE_RAW; + meta->backingStoreRawFormat = virStorageFileFormatTypeFromString(tmp); + if (meta->backingStoreRawFormat <= VIR_STORAGE_FILE_NONE) + meta->backingStoreRawFormat = VIR_STORAGE_FILE_RAW; break; } - case QCOW2_HDR_EXTENSION_DATA_FILE_NAME: { - if (!dataFilePath) - break; - - *dataFilePath = g_new0(char, len + 1); - memcpy(*dataFilePath, buf + offset, len); + case QCOW2_HDR_EXTENSION_DATA_FILE_NAME: + if (virBitmapIsBitSet(meta->features, VIR_STORAGE_FILE_FEATURE_DATA_FILE)) + meta->dataFileRaw = g_strndup(buf + offset, len); break; - } case QCOW2_HDR_EXTENSION_END: return 0; @@ -623,8 +607,6 @@ qcow2GetImageSpecific(virStorageSource *meta, const char *buf, size_t buf_size) { - int format; - meta->clusterSize = 0; if (buf_size > (QCOWX_HDR_CLUSTER_BITS_OFFSET + 4)) { int clusterBits = virReadBufInt32BE(buf + QCOWX_HDR_CLUSTER_BITS_OFFSET); @@ -639,33 +621,9 @@ qcow2GetImageSpecific(virStorageSource *meta, if (qcow2GetFeatures(meta, buf, buf_size) < 0) return -1; - format = meta->backingStoreRawFormat; - - if (qcow2GetExtensions(buf, buf_size, &format, NULL) < 0) + if (qcow2GetExtensions(meta, buf, buf_size) < 0) return 0; - meta->backingStoreRawFormat = format; - - return 0; -} - - -static int -qcow2GetDataFile(char **res, - virBitmap *features, - char *buf, - size_t buf_size) -{ - *res = NULL; - - if (buf_size < QCOW2v3_HDR_FEATURES_INCOMPATIBLE + 8) - return 0; - - if (features && virBitmapIsBitSet(features, VIR_STORAGE_FILE_FEATURE_DATA_FILE)) { - if (qcow2GetExtensions(buf, buf_size, NULL, res) < 0) - return -1; - } - return 0; } -- 2.49.0
On a Thursday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Refactor qcow2GetExtensions to parse everything at once and directly assign it into fields in the parsed virStorageSource.
This removes the need for qcow2GetDataFile as it will be parsed directly.
The patch also simplifies the juggling of variables which was needed to parse the backing file format filed, when it was passed via pointer in
extra "filed"?
argument.
qcow2GetExtensions is now invoked on qcow2 images so we can remove the version check for qcow(v1) images.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 68 +++++---------------------- 1 file changed, 13 insertions(+), 55 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Peter Krempa <pkrempa@redhat.com> Since the 'compat' field is set based on qcow2 features it belongs to the qcow2 code rather than to the main metadata probing function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 73751c7a2f..9fcc052ea3 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -579,6 +579,7 @@ qcow2GetFeatures(virStorageSource *meta, int version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); g_clear_pointer(&meta->features, virBitmapFree); + g_clear_pointer(&meta->compat, g_free); if (version == 2) return 0; @@ -587,6 +588,7 @@ qcow2GetFeatures(virStorageSource *meta, return -1; meta->features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST); + meta->compat = g_strdup("1.1"); qcow2GetFeaturesProcessGroup(virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_COMPATIBLE), qcow2CompatibleFeatureArray, @@ -997,10 +999,6 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, buf, len); } - VIR_FREE(meta->compat); - if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features) - meta->compat = g_strdup("1.1"); - return 0; } -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Remove the old now-unused infrastructure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 66 +++++++-------------------- 1 file changed, 16 insertions(+), 50 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 9fcc052ea3..26f8d63e9a 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -89,13 +89,6 @@ struct FileTypeInfo { * or NULL if there is no COW base image, to RES; * return BACKING_STORE_* */ const struct FileEncryptionInfo *cryptInfo; /* Encryption info */ - unsigned long long (*getClusterSize)(const char *buf, - size_t buf_size); - int (*getBackingStore)(char **res, int *format, - const char *buf, size_t buf_size); - int (*getDataFile)(char **res, virBitmap *features, char *buf, size_t buf_size); - int (*getFeatures)(virBitmap **features, int format, - char *buf, ssize_t len); int (*getImageSpecific)(virStorageSource *meta, const char *buf, size_t buf_size); @@ -250,18 +243,18 @@ static struct FileEncryptionInfo const qcow2EncryptionInfo[] = { static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_RAW] = { 0, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, 0, 0, 0, luksEncryptionInfo, - NULL, NULL, NULL, NULL, NULL }, + NULL }, [VIR_STORAGE_FILE_DIR] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, 64, 4, {0x20000}, - 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL, NULL, NULL, NULL + 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL }, [VIR_STORAGE_FILE_CLOOP] = { /* #!/bin/sh @@ -270,7 +263,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, @@ -278,67 +271,63 @@ static struct FileTypeInfo const fileTypeInfo[] = { * would have to match) but then disables that check. */ 0, NULL, 0, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", LV_LITTLE_ENDIAN, -2, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_VPC] = { 0, "conectix", LV_BIG_ENDIAN, 12, 4, {0x10000}, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL, NULL, NULL, NULL + 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL }, /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { 64, "\x7f\x10\xda\xbe", LV_LITTLE_ENDIAN, 68, 4, {0x00010001}, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL, NULL, NULL, NULL}, + 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL }, /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", LV_LITTLE_ENDIAN, -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 8, - PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL, NULL, NULL, NULL }, + PLOOP_SIZE_MULTIPLIER, NULL, NULL }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { 0, "OOOM", LV_BIG_ENDIAN, 4, 4, {2}, - 4+4+1024+4, 8, 1, NULL, NULL, NULL, NULL, NULL, cowGetImageSpecific + 4+4+1024+4, 8, 1, NULL, cowGetImageSpecific }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", LV_BIG_ENDIAN, 4, 4, {1}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow1EncryptionInfo, - NULL, NULL, NULL, NULL, qcowGetImageSpecific + qcowGetImageSpecific }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", LV_BIG_ENDIAN, 4, 4, {2, 3}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow2EncryptionInfo, - NULL, - NULL, - NULL, - NULL, qcow2GetImageSpecific }, [VIR_STORAGE_FILE_QED] = { /* https://wiki.qemu.org/Features/QED */ 0, "QED", LV_LITTLE_ENDIAN, -2, 0, {0}, - QED_HDR_IMAGE_SIZE, 8, 1, NULL, NULL, NULL, NULL, NULL, qedGetImageSpecific + QED_HDR_IMAGE_SIZE, 8, 1, NULL, qedGetImageSpecific }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", LV_LITTLE_ENDIAN, 4, 4, {1, 2, 3}, - 4+4+4, 8, 512, NULL, NULL, NULL, NULL, NULL, vmdk4GetImageSpecific + 4+4+4, 8, 512, NULL, vmdk4GetImageSpecific }, }; G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST); @@ -911,7 +900,6 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, char *buf, size_t len) { - int format; size_t i; VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", @@ -973,32 +961,10 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier; } - VIR_FREE(meta->backingStoreRaw); - g_clear_pointer(&meta->features, virBitmapFree); - VIR_FREE(meta->dataFileRaw); - if (fileTypeInfo[meta->format].getImageSpecific && fileTypeInfo[meta->format].getImageSpecific(meta, buf, len) < 0) return -1; - if (fileTypeInfo[meta->format].getClusterSize != NULL) - meta->clusterSize = fileTypeInfo[meta->format].getClusterSize(buf, len); - - if (fileTypeInfo[meta->format].getBackingStore != NULL) { - fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, - &format, buf, len); - meta->backingStoreRawFormat = format; - } - - if (fileTypeInfo[meta->format].getFeatures != NULL && - fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0) - return -1; - - if (fileTypeInfo[meta->format].getDataFile != NULL) { - fileTypeInfo[meta->format].getDataFile(&meta->dataFileRaw, meta->features, - buf, len); - } - return 0; } -- 2.49.0
On a Thursday in 2025, Peter Krempa via Devel wrote:
Patch 3 fixes an almost 15 year old bug in the qcow2 header extension parser which breaks when the qcow2 image has more than 1 header extension. For us it caused problems for qcow2 images with data file and backing file which we didn't use before, and the only field we cared about was always put first by qemu.
Ironically we did have a test file that had such config but it was missed in the test output.
Patches 1, 2 are refinment of debug tools I used to see what's happening.
Patch 4 adds bitmaps to some test images. We don't parse them but just to be sure.
The rest of the series refactors the metadata parser callbacks with the end goal to not parse the qcow2 header extensions twice.
It's good that we no longer parse them twice. It's good that we no longer parse them twice.
Peter Krempa (15): qcow2GetExtensions: Add debug logs for interesting fields in qcow2 header extension parser virstoragetest: Reformat output to hilight dataFile relationship storage_file_probe: qcow2GetExtensions: Fix qcow2 header extension parsing virstoragetest: Add qcow2 bitmaps to some images storage_file_probe: Add image specific callback taking the whole virStorageSource storage_file_probe: Refactor cowGetBackingStore into cowGetImageSpecific storage_file_probe: Refactor qedGetBackingStore into qedGetImageSpecific storage_file_probe: Refactor vmdk4GetBackingStore into vmdk4GetImageSpecific storage_file_probe: Refactor qcowXGetBackingStore into specific callbacks for qcow and qcow2 storage_file_probe: Move logic from qcow2GetClusterSize to qcow2GetImageSpecific storage_file_probe: Move qcow2GetFeatures(ProcessGroup) functions storage_file_probe: Call qcow2GetFeatures from qcow2GetImageSpecific storage_file_probe: Parse all qcow2 extensions at once storage_file_probe: Move setting of 'compat' attribute to qcow2GetFeatures storage_file_probe: Remove unused image probing callbacks
src/storage_file/storage_file_probe.c | 439 ++++++++---------- tests/virstoragetest.c | 20 +- .../virstoragetestdata/images/datafile.qcow2 | Bin 327680 -> 393256 bytes .../images/qcow2_qcow2-qcow2_qcow2-auto.qcow2 | Bin 196616 -> 327720 bytes .../images/qcow2datafile-datafile.qcow2 | Bin 196616 -> 327720 bytes tests/virstoragetestdata/out/directory-dir | 1 + tests/virstoragetestdata/out/directory-none | 1 + tests/virstoragetestdata/out/directory-raw | 1 + .../out/qcow2-auto_qcow2-qcow2_raw-raw | 1 + .../out/qcow2-auto_raw-raw-relative | 1 + tests/virstoragetestdata/out/qcow2-datafile | 15 +- .../out/qcow2-protocol-backing-file | 2 + .../out/qcow2-protocol-backing-nbd | 2 + .../out/qcow2-qcow2_nbd-raw | 2 + .../out/qcow2-qcow2_qcow2-auto | 2 + .../out/qcow2-qcow2_qcow2-qcow2_qcow2-auto | 3 + .../out/qcow2-qcow2_qcow2-qcow2_raw-auto | 3 + .../out/qcow2-qcow2_qcow2-qcow2_raw-raw | 3 + .../out/qcow2-qcow2_raw-raw-relative | 2 + tests/virstoragetestdata/out/qcow2-symlinks | 3 + .../out/qcow2datafile-qcow2_qcow2-datafile | 24 +- tests/virstoragetestdata/out/qed-auto_raw | 1 + tests/virstoragetestdata/out/qed-qed_raw | 2 + tests/virstoragetestdata/out/raw-auto | 1 + tests/virstoragetestdata/out/raw-raw | 1 + 25 files changed, 263 insertions(+), 267 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa