[PATCH 00/15] storage_file_probe: Fix ancient bug in qcow2 header extensions parser and refactor the image probing callbacks

Peter Krempa via Devel posted 15 patches 3 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cover.1748507931.git.pkrempa@redhat.com
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(-)
[PATCH 00/15] storage_file_probe: Fix ancient bug in qcow2 header extensions parser and refactor the image probing callbacks
Posted by Peter Krempa via Devel 3 months, 1 week ago
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
Re: [PATCH 00/15] storage_file_probe: Fix ancient bug in qcow2 header extensions parser and refactor the image probing callbacks
Posted by Ján Tomko via Devel 3 months, 1 week ago
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