Re: [PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2288 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]