qcow2 v3 requires every snapshot table entry to have two extra data
fields: The 64-bit VM state size, and the virtual disk size.  Both are
optional for v2 images, so they may not be present.

qcow2_upgrade() therefore should update the snapshot table to ensure all
entries have these extra data fields.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 99e9dad798..75d41683a1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4731,7 +4731,9 @@ static int qcow2_upgrade(BlockDriverState *bs, int target_version,
                          Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
+    bool need_snapshot_update;
     int current_version = s->qcow_version;
+    int i;
     int ret;
 
     /* This is qcow2_upgrade(), not qcow2_downgrade() */
@@ -4740,7 +4742,30 @@ static int qcow2_upgrade(BlockDriverState *bs, int target_version,
     /* There are no other versions (yet) that you can upgrade to */
     assert(target_version == 3);
 
-    status_cb(bs, 0, 1, cb_opaque);
+    status_cb(bs, 0, 2, cb_opaque);
+
+    /*
+     * In v2, snapshots do not need to have extra data.  v3 requires
+     * the 64-bit VM state size and the virtual disk size to be
+     * present.
+     * qcow2_write_snapshots() will always write the list in the
+     * v3-compliant format.
+     */
+    need_snapshot_update = false;
+    for (i = 0; i < s->nb_snapshots; i++) {
+        if (s->snapshots[i].extra_data_size < 16) {
+            need_snapshot_update = true;
+            break;
+        }
+    }
+    if (need_snapshot_update) {
+        ret = qcow2_write_snapshots(bs);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to update the snapshot table");
+            return ret;
+        }
+    }
+    status_cb(bs, 1, 2, cb_opaque);
 
     s->qcow_version = target_version;
     ret = qcow2_update_header(bs);
@@ -4749,7 +4774,7 @@ static int qcow2_upgrade(BlockDriverState *bs, int target_version,
         error_setg_errno(errp, -ret, "Failed to update the image header");
         return ret;
     }
-    status_cb(bs, 1, 1, cb_opaque);
+    status_cb(bs, 2, 2, cb_opaque);
 
     return 0;
 }
-- 
2.21.0

On 7/30/19 12:25 PM, Max Reitz wrote:
> qcow2 v3 requires every snapshot table entry to have two extra data
> fields: The 64-bit VM state size, and the virtual disk size.  Both are
> optional for v2 images, so they may not be present.
> 
> qcow2_upgrade() therefore should update the snapshot table to ensure all
> entries have these extra data fields.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 

> +
> +    /*
> +     * In v2, snapshots do not need to have extra data.  v3 requires
> +     * the 64-bit VM state size and the virtual disk size to be
> +     * present.
> +     * qcow2_write_snapshots() will always write the list in the
> +     * v3-compliant format.
> +     */
> +    need_snapshot_update = false;
> +    for (i = 0; i < s->nb_snapshots; i++) {
> +        if (s->snapshots[i].extra_data_size < 16) {

s/16/sizeof(extra)/ looks a bit nicer, but doesn't change semantics.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

On 30.07.19 20:10, Eric Blake wrote:
> On 7/30/19 12:25 PM, Max Reitz wrote:
>> qcow2 v3 requires every snapshot table entry to have two extra data
>> fields: The 64-bit VM state size, and the virtual disk size.  Both are
>> optional for v2 images, so they may not be present.
>>
>> qcow2_upgrade() therefore should update the snapshot table to ensure all
>> entries have these extra data fields.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
>> Reported-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2.c | 29 +++++++++++++++++++++++++++--
>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>
> 
>> +
>> +    /*
>> +     * In v2, snapshots do not need to have extra data.  v3 requires
>> +     * the 64-bit VM state size and the virtual disk size to be
>> +     * present.
>> +     * qcow2_write_snapshots() will always write the list in the
>> +     * v3-compliant format.
>> +     */
>> +    need_snapshot_update = false;
>> +    for (i = 0; i < s->nb_snapshots; i++) {
>> +        if (s->snapshots[i].extra_data_size < 16) {
> 
> s/16/sizeof(extra)/ looks a bit nicer, but doesn't change semantics.

Hm, but it’s not quite right.  I mean, right now it is, but if we were
to add a new field to snapshot metadata, it wouldn’t be.  It should be 16.

I can make it something like sizeof(extra.vm_state_size_large) +
sizeof(extra.disk_size), though.

Max

> Reviewed-by: Eric Blake <eblake@redhat.com>