Re: [PATCH v4 18/30] qcow2: Add subcluster support to qcow2_get_host_offset()

[Alberto Garcia]

On Wed 22 Apr 2020 10:07:30 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
>> +static int count_contiguous_subclusters(BlockDriverState *bs, int 
>> nb_clusters,
>> +                                        unsigned sc_index, uint64_t 
>> *l2_slice,
>> +                                        int l2_index)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>
> preexist, but, worth asserting that nb_clusters are all in this
> l2_slice?

Ok.

>> +        for (j = (i == 0) ? sc_index : 0; j < s->subclusters_per_cluster; 
>> j++) {
>> +            if (qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, j) != 
>> type) {
>> +                goto out;
>
> why not just return count from here? And then you don't need goto at
> all. Hmm, may be out: code will be extended in further patches..

It's not extended in further patches. I generally prefer having a single
exit point but you're right that it probably doesn't make sense here.

>>           /* Compressed clusters can only be processed one by one */
>> -        c = 1;
>> +        sc = s->subclusters_per_cluster - sc_index;
>
> should not we assert here that sc_index == 0? Otherwise the caller
> definitely doing something wrong.

No, no, the guest offset doesn't need to be cluster aligned so sc_index
can perfectly be != 0.

>> +    case QCOW2_SUBCLUSTER_ZERO_ALLOC:
>> +    case QCOW2_SUBCLUSTER_NORMAL:
>> +    case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
>> +        sc = count_contiguous_subclusters(bs, nb_clusters, sc_index,
>> +                                          l2_slice, l2_index);
>>           *host_offset = l2_entry & L2E_OFFSET_MASK;
>>           if (offset_into_cluster(s, *host_offset)) {
>
> Hmm, you may move "sc = count_contiguous_subclusters" to be after the
> switch-block, as it is universal now. And keep only offset calculation
> and error checking in the switch-block.

That's actually a good idea, thanks !! (plus we actually get to use the
QCOW2_SUBCLUSTER_COMPRESSED check in count_contiguous_subclusters(),
which is currently dead code).

Berto