Re: [PATCH v4 11/30] qcow2: Add l2_entry_size()

[Alberto Garcia <berto@igalia.com>]

On Tue 14 Apr 2020 11:44:57 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>       /* allocate a new l2 entry */
>>   
>> -    l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
>> +    l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s));
>
> hmm. s->l2_size * l2_entry_size, isn't it just s->cluster_size always?
> Maybe, just refactor these things?

I think the patch is simpler to follow if I only do the strictly
necessary changes and don't mix them with other things.

>>           nb_new_l2_tables = DIV_ROUND_UP(nb_new_data_clusters,
>> -                                        s->cluster_size / sizeof(uint64_t));
>> +                                        s->cluster_size / l2_entry_size(s));
>
> Isn't it just s->l2_size ?

Yes, same as before.

>>           /* The cluster range may not be aligned to L2 boundaries, so add one L2
>>            * table for a potential head/tail */
>>           nb_new_l2_tables++;
>
> Conversions looks correct, but how to check that we have converted
> everything?

I went through all cases, I think I didn't miss any!

> I found this not converted chunk:
>
>      /* total size of L2 tables */
>      nl2e = aligned_total_size / cluster_size;
>      nl2e = ROUND_UP(nl2e, cluster_size / sizeof(uint64_t));
>      meta_size += nl2e * sizeof(uint64_t);

This is used by qcow2_measure() and is fixed on a later patch because,
unlike all other cases, it does not use a BlockDriverState to determine
the size of an L2 entry.

> Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all
> sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE,
> REFTABLE_ENTRY_SIZE etc?

That wouldn't be a bad thing I guess but, again, for a separate patch or
series.

> And all occurrences of pure '8' (not many of them exist)

I think most/all nowadays only refer to the number of bits per byte.

Maybe there's a couple that still need to be fixed, but we have been
removing a lot of numeric literals from the qcow2 code (see for example
b6c246942b, 3afea40243 or a35f87f50d).

Berto