[v3,07/16] qcow2: Write v3-compliant snapshot list on upgrade
diff mbox series

Message ID 20191011152814.14791-8-mreitz@redhat.com
State New
Headers show
Series
  • qcow2: Let check -r all repair some snapshot bits
Related show

Commit Message

Max Reitz Oct. 11, 2019, 3:28 p.m. UTC
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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

Comments

Eric Blake Oct. 11, 2019, 4:23 p.m. UTC | #1
On 10/11/19 10:28 AM, 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block/qcow2.c | 32 ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
> 

> +    need_snapshot_update = false;
> +    for (i = 0; i < s->nb_snapshots; i++) {
> +        if (s->snapshots[i].extra_data_size <
> +            sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
> +            sizeof_field(QCowSnapshotExtraData, disk_size))

Shorter as:
if (s->snapshots[i].extra_data_size < sizeof(QCowSnapshotExtraData))

but that's stylistic, so R-b still stands.
Max Reitz Oct. 14, 2019, 8:45 a.m. UTC | #2
On 11.10.19 18:23, Eric Blake wrote:
> On 10/11/19 10:28 AM, 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/qcow2.c | 32 ++++++++++++++++++++++++++++++--
>>   1 file changed, 30 insertions(+), 2 deletions(-)
>>
> 
>> +    need_snapshot_update = false;
>> +    for (i = 0; i < s->nb_snapshots; i++) {
>> +        if (s->snapshots[i].extra_data_size <
>> +            sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
>> +            sizeof_field(QCowSnapshotExtraData, disk_size))
> 
> Shorter as:
> if (s->snapshots[i].extra_data_size < sizeof(QCowSnapshotExtraData))
> 
> but that's stylistic, so R-b still stands.

Yes, but if we ever add fields to QCowSnapshotExtraData, we shouldn’t
count them here.  Therefore, I think we need to count exactly the fields
that the standard says are mandatory in v3.

Max
Eric Blake Oct. 14, 2019, 1:53 p.m. UTC | #3
On 10/14/19 3:45 AM, Max Reitz wrote:

>>> +    need_snapshot_update = false;
>>> +    for (i = 0; i < s->nb_snapshots; i++) {
>>> +        if (s->snapshots[i].extra_data_size <
>>> +            sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
>>> +            sizeof_field(QCowSnapshotExtraData, disk_size))
>>
>> Shorter as:
>> if (s->snapshots[i].extra_data_size < sizeof(QCowSnapshotExtraData))
>>
>> but that's stylistic, so R-b still stands.
> 
> Yes, but if we ever add fields to QCowSnapshotExtraData, we shouldn’t
> count them here.  Therefore, I think we need to count exactly the fields
> that the standard says are mandatory in v3.

If we ever add more fields, I'd prefer that we did something like:

struct QCowSnapshotExtraV3Minimum {
     uint64_t vm_state_size_large;
     uint64_t disk_size;
};
struct QCow3SnapshotExtraFull {
     struct QCowSnapshotExtraV3Minimum base;
     new fields...;
};

and use sane naming to get at extra members based on the expected types, 
rather than trying to piecemeal portions of a type based on size.

Until we actually DO add more fields, why do we have to complicate the 
current code?
Max Reitz Oct. 14, 2019, 2:09 p.m. UTC | #4
On 14.10.19 15:53, Eric Blake wrote:
> On 10/14/19 3:45 AM, Max Reitz wrote:
> 
>>>> +    need_snapshot_update = false;
>>>> +    for (i = 0; i < s->nb_snapshots; i++) {
>>>> +        if (s->snapshots[i].extra_data_size <
>>>> +            sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
>>>> +            sizeof_field(QCowSnapshotExtraData, disk_size))
>>>
>>> Shorter as:
>>> if (s->snapshots[i].extra_data_size < sizeof(QCowSnapshotExtraData))
>>>
>>> but that's stylistic, so R-b still stands.
>>
>> Yes, but if we ever add fields to QCowSnapshotExtraData, we shouldn’t
>> count them here.  Therefore, I think we need to count exactly the fields
>> that the standard says are mandatory in v3.
> 
> If we ever add more fields, I'd prefer that we did something like:
> 
> struct QCowSnapshotExtraV3Minimum {
>     uint64_t vm_state_size_large;
>     uint64_t disk_size;
> };
> struct QCow3SnapshotExtraFull {
>     struct QCowSnapshotExtraV3Minimum base;
>     new fields...;
> };
> 
> and use sane naming to get at extra members based on the expected types,
> rather than trying to piecemeal portions of a type based on size.
> 
> Until we actually DO add more fields, why do we have to complicate the
> current code?

I don’t think it’s complicated, I find it very expressive.  There are
two fields, we check whether they are present; why, that’s obvious,
because those are the ones mandated by the standard.

If we just checked against sizeof(QCowSnapshotExtraData), I’d (as a
naïve reader) ask myself what that has to do with the standard.  I’d
need to look into the structure definition and see that it currently
contains exactly the fields that are mandated by the standard, and then
I’d think “But what if we ever add fields to this structure?”  The more
verbose version avoids this problem.

And I’m not really inclined to take your proposal above right now,
because that would mean having to touch a lot of code.  I prefer this
more verbose code over that.


Also, you explicitly agreed that the code in this patch is preferable to
a plain sizeof(extra) in v2:

https://lists.nongnu.org/archive/html/qemu-block/2019-08/msg00942.html

Max

Patch
diff mbox series

diff --git a/block/qcow2.c b/block/qcow2.c
index d43064dca2..a2e46ce589 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4907,7 +4907,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() */
@@ -4916,7 +4918,33 @@  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 <
+            sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+            sizeof_field(QCowSnapshotExtraData, disk_size))
+        {
+            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);
@@ -4925,7 +4953,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;
 }