| Message ID | 20191011152814.14791-8-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: Let check -r all repair some snapshot bits | expand |
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.
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
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?
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
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; }