[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 14/34] qcow2: Add QCow2SubclusterType and qcow2_get_subclu
From: |
Max Reitz |
Subject: |
Re: [PATCH v9 14/34] qcow2: Add QCow2SubclusterType and qcow2_get_subcluster_type() |
Date: |
Thu, 2 Jul 2020 11:57:46 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 |
On 01.07.20 18:26, Alberto Garcia wrote:
> On Wed 01 Jul 2020 02:52:14 PM CEST, Max Reitz wrote:
>>> if (l2_entry & QCOW_OFLAG_COMPRESSED) {
>>> return QCOW2_CLUSTER_COMPRESSED;
>>> - } else if (l2_entry & QCOW_OFLAG_ZERO) {
>>> + } else if ((l2_entry & QCOW_OFLAG_ZERO) && !has_subclusters(s)) {
>>
>> OK, so now qcow2_get_cluster_type() reports zero clusters to be normal
>> or unallocated clusters when there are subclusters. Seems weird to
>> me, because zero clusters are invalid clusters then.
>
> I'm actually hesitant about this.
>
> In extended L2 entries QCOW_OFLAG_ZERO does not have any meaning so
> technically it doesn't need to be checked any more than the other
> reserved bits (1 to 8).
Good point. That convinces me.
> The reason why we would want to check it is, of course, because that bit
> does have a meaning in regular L2 entries.
>
> But that bit is ignored in images with subclusters so the only reason
> why we would check it is to report corruption, not because we need to
> know its value.
Sure. But isn’t that the whole point of having QCOW2_SUBCLUSTER_INVALID
in the first place?
> It's true that we do check it in v2 images, although in that case the
> entries are otherwise identical and there is a way to convert between
> both types.
>
>> I preferred just reporting them as zero clusters and letting the
>> caller deal with it, because it does mean an error in the image and so
>> it should be reported.
>
> Another alternative would be to add QCOW2_CLUSTER_INVALID and we could
> even include there other cases like unaligned offsets and things like
> that. But that would also affect the code that repairs corrupted images.
Interesting. Well, and that’d be definitely too much for this series,
as you already said.
So:
Reviewed-by: Max Reitz <mreitz@redhat.com>
signature.asc
Description: OpenPGP digital signature