[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade
From: |
Max Reitz |
Subject: |
Re: [PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade |
Date: |
Mon, 14 Oct 2019 16:09:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
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
signature.asc
Description: OpenPGP digital signature
- [PATCH v3 03/16] qcow2: Add Error ** to qcow2_read_snapshots(), (continued)
- [PATCH v3 03/16] qcow2: Add Error ** to qcow2_read_snapshots(), Max Reitz, 2019/10/11
- [PATCH v3 04/16] qcow2: Keep unknown extra snapshot data, Max Reitz, 2019/10/11
- [PATCH v3 05/16] qcow2: Make qcow2_write_snapshots() public, Max Reitz, 2019/10/11
- [PATCH v3 06/16] qcow2: Put qcow2_upgrade() into its own function, Max Reitz, 2019/10/11
- [PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade, Max Reitz, 2019/10/11
[PATCH v3 08/16] qcow2: Separate qcow2_check_read_snapshot_table(), Max Reitz, 2019/10/11
[PATCH v3 09/16] qcow2: Add qcow2_check_fix_snapshot_table(), Max Reitz, 2019/10/11
[PATCH v3 10/16] qcow2: Fix broken snapshot table entries, Max Reitz, 2019/10/11
[PATCH v3 11/16] qcow2: Keep track of the snapshot table length, Max Reitz, 2019/10/11
[PATCH v3 12/16] qcow2: Fix overly long snapshot tables, Max Reitz, 2019/10/11
[PATCH v3 13/16] qcow2: Repair snapshot table with too many entries, Max Reitz, 2019/10/11
[PATCH v3 14/16] qcow2: Fix v3 snapshot table entry compliancy, Max Reitz, 2019/10/11
[PATCH v3 15/16] iotests: Add peek_file* functions, Max Reitz, 2019/10/11