Re: [PATCH v9 14/34] qcow2: Add QCow2SubclusterType and qcow2_get_subcluster_type()

[Max Reitz]

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