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
