Re: [Qemu-block] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious

[Eric Blake]

On 05/05/2017 03:51 PM, Max Reitz wrote:
> On 04.05.2017 05:07, Eric Blake wrote:
>> Treat plain zero clusters differently from allocated ones, so that
>> we can simplify the logic of checking whether an offset is present.
>> Do this by splitting QCOW2_CLUSTER_ZERO into two new enums,
>> QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC.
>>
>> I tried to arrange the enum so that we could use
>> 'ret <= QCOW2_CLUSTER_ZERO_PLAIN' for all unallocated types, and
>> 'ret >= QCOW2_CLUSTER_ZERO_ALLOC' for allocated types, although
>> I didn't actually end up taking advantage of the layout.
>>
>> In many cases, this leads to simpler code, by properly combining
>> cases (sometimes, both zero types pair together, other times,
>> plain zero is more like unallocated while allocated zero is more
>> like normal).
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

>> @@ -558,52 +557,32 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
>> uint64_t offset,
>>      assert(nb_clusters <= INT_MAX);
>>
>>      ret = qcow2_get_cluster_type(*cluster_offset);
>> +    if (s->qcow_version < 3 && (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
>> +                                ret == QCOW2_CLUSTER_ZERO_ALLOC)) {
>> +        qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
>> +                                " in pre-v3 image (L2 offset: %#" PRIx64
>> +                                ", L2 index: %#x)", l2_offset, l2_index);
>> +        ret = -EIO;
>> +        goto fail;
>> +    }
...
>> +    case QCOW2_CLUSTER_ZERO_PLAIN:
>>      case QCOW2_CLUSTER_UNALLOCATED:
>>          /* how many empty clusters ? */
>>          c = count_contiguous_clusters_unallocated(nb_clusters,
>> -                                                  &l2_table[l2_index],
>> -                                                  
>> QCOW2_CLUSTER_UNALLOCATED);
>> +                                                  &l2_table[l2_index], ret);
> 
> Nit pick: Using ret here is a bit weird (because it's such a meaningless
> name). It would be good if we had a separate cluster_type variable.

qcow2_get_cluster_offset() returns the cluster type on success, and
-errno on failure.  So 'ret' actually makes some sense: it really is the
value we are about to return.  But it may also work to have a separate
variable up front, then assign ret = cluster_type at the end; I'll play
with it and see which one looks better.

> 
>>          *cluster_offset = 0;
>>          break;
>> +    case QCOW2_CLUSTER_ZERO_ALLOC:
>>      case QCOW2_CLUSTER_NORMAL:
>>          /* how many allocated clusters ? */
>>          c = count_contiguous_clusters(nb_clusters, s->cluster_size,
>> -                &l2_table[l2_index], QCOW_OFLAG_ZERO);
>> +                                      &l2_table[l2_index], QCOW_OFLAG_ZERO);
>>          *cluster_offset &= L2E_OFFSET_MASK;
>>          if (offset_into_cluster(s, *cluster_offset)) {
>>              qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset 
>> %#"
> 
> Well, preallocated zero clusters are not exactly data clusters... Not
> that any user cared, but s/Data cluster/Cluster allocation/ would be
> more correct.

Good idea.

> 
> By the way, allow me to state just how much I love this hunk: Very much.
> Looks great! It gets a place on my list of favorite hunks of this year
> at least.
> 
> [...]
> 
>> @@ -1760,7 +1740,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
>> *bs, uint64_t *l1_table,
>>              int cluster_type = qcow2_get_cluster_type(l2_entry);>           
>>    bool preallocated = offset != 0;
> 
> I could get behind removing this variable and replacing all
> "if (!preallocated)" instances by
> "if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN)". Up to you, though.

Makes sense.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature