[Top][All Lists]

[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.


Reviewed-by: Max Reitz <mreitz@redhat.com>

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]