Hi,

As Eric reports in https://bugzilla.redhat.com/show_bug.cgi?id=1727347,
qemu-img amend has a bug when it comes to converting qcow2 v2 images to
v3: In v3, every snapshot table entry requires at least 16 bytes of
extra metadata to be present, which isn’t the case for v2 images.
Currently, qemu-img amend doesn’t take care of updating the snapshot
table, so the image is a bit corrupt afterwards (luckily, qemu doesn’t
take notice, though).

This yields the following patches:
- Patch 3: Helper patch
- Patch 4: Helper patch, so we can actually do more than just to bump up
  the version number when upgrading a qcow2 image from v2 to v3
- Patch 5: The fix

Eric also points out that qemu-img check does not see any problem with
such images and doesn’t fix them, so:
- Patch 11: Makes qemu-img check report if a snapshot table entry has
  too little extra data, and repair it with -r all
  (Patches 6 and 7 add the necessary infrastructure so we can check and
  repair the snapshot table at all.)

Then I got the glorious idea of “Hey, if I want to see how much extra
data a snapshot table entry has outside of qcow2_read_snapshots(), I
should add a field that reports that value to QCowSnapshot.  And if I do
that, I might as well make the qcow2 driver interpret the specification
a bit more literally, namely it should ignore all unknown extra data,
that is (as I interpret it), keep it in memory and write it back when
updating the snapshot table.”

That led to patch 2.  Maybe you find that stupid, in which case we can
totally drop patch 2 (with some changes to other patches).

Anyway.  Because of this, qcow2_read_snapshots() suddenly got more error
case, so I thought now would be a good time to give it an Error **
parameter.  Cue patch 1.

At this point:
(1) I had infrastructure for repairing a snapshot table in
    qemu-img check -r all, and
(2) I had added a new error case if a snapshot table entry has a
    suspiciously large amount of extra data.

I decided that this should be repairable, too.  This is done by patch 8.
(If we drop patch 2, this will go, too.)

Now I was really into it, so I decided even more things needed fixing!
Namely the final two reasons why we would reject a snapshot table:
(1) It has too many snapshots (patch 10),
(2) It is too long overall (patch 9).


Finally, patch 13 adds an overly complicated test (using the new
peek_file* functions added in patch 12).


Max Reitz (13):
  qcow2: Add Error ** to qcow2_read_snapshots()
  qcow2: Keep unknown extra snapshot data
  qcow2: Make qcow2_write_snapshots() public
  qcow2: Put qcow2_upgrade() into an own function
  qcow2: Write v3-compliant snapshot list on upgrade
  qcow2: Separate qcow2_check_read_snapshot_table()
  qcow2: Add qcow2_check_fix_snapshot_table()
  qcow2: Fix broken snapshot table entries
  qcow2: Fix overly long snapshot tables
  qcow2: Repair snapshot table with too many entries
  qcow2: Fix v3 snapshot table entry compliancy
  iotests: Add peek_file* functions
  iotests: Test qcow2's snapshot table handling

 block/qcow2.h                |  15 +-
 block/qcow2-snapshot.c       | 302 +++++++++++++++++++++--
 block/qcow2.c                | 152 ++++++++++--
 tests/qemu-iotests/261       | 449 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/261.out   | 321 +++++++++++++++++++++++++
 tests/qemu-iotests/common.rc |  20 ++
 tests/qemu-iotests/group     |   1 +
 7 files changed, 1219 insertions(+), 41 deletions(-)
 create mode 100755 tests/qemu-iotests/261
 create mode 100644 tests/qemu-iotests/261.out

-- 
2.21.0

On 7/30/19 12:24 PM, Max Reitz wrote:
> Hi,
> 
> As Eric reports in https://bugzilla.redhat.com/show_bug.cgi?id=1727347,
> qemu-img amend has a bug when it comes to converting qcow2 v2 images to
> v3: In v3, every snapshot table entry requires at least 16 bytes of
> extra metadata to be present, which isn’t the case for v2 images.
> Currently, qemu-img amend doesn’t take care of updating the snapshot
> table, so the image is a bit corrupt afterwards (luckily, qemu doesn’t
> take notice, though).

And if anyone wants to work on an obvious followup series after this: we
should allow 'qemu-img resize' to work on v3 images with internal
snapshots, while still continuing to forbid it on v2 images (with v2,
ALL internal snapshots are assumed to have the same size as the image
itself, so you can't resize the image without also resizing internal
snapshots, but that doesn't preserve proper guest history; but with v3,
since all internal snapshots have their own proper size recorded,
changing the image size doesn't impact the snapshots).

> 
> This yields the following patches:
> - Patch 3: Helper patch
> - Patch 4: Helper patch, so we can actually do more than just to bump up
>   the version number when upgrading a qcow2 image from v2 to v3
> - Patch 5: The fix

Quite this history of how the series came to be. Thanks for tackling it!


> Then I got the glorious idea of “Hey, if I want to see how much extra
> data a snapshot table entry has outside of qcow2_read_snapshots(), I
> should add a field that reports that value to QCowSnapshot.  And if I do
> that, I might as well make the qcow2 driver interpret the specification
> a bit more literally, namely it should ignore all unknown extra data,
> that is (as I interpret it), keep it in memory and write it back when
> updating the snapshot table.”

Indeed. If the extra data ever becomes essential to correct
interpretation of the image, then we would add an incompatible feature
bit in the qcow2 header (older actors would refuse to open the image
because they don't understand the incompatible feature, newer actors
will know to use the longer extra data as mandatory).  Otherwise, extra
data should be preserved intact insofar as is possible, but can safely
be ignored or truncated by older actors without breaking the proper
image interpretation in the newer actor that wrote that extra data.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org