Re: [PATCH v4 02/30] qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

[Alberto Garcia]

On Thu 09 Apr 2020 09:50:52 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>       case QCOW2_CLUSTER_ZERO_PLAIN:
>>       case QCOW2_CLUSTER_UNALLOCATED:
>>           /* how many empty clusters ? */
>>           c = count_contiguous_clusters_unallocated(bs, nb_clusters,
>>                                                     &l2_slice[l2_index], 
>> type);
>> -        *cluster_offset = 0;
>> +        *host_offset = 0;
>
> Actually, dead assignment now.. But I feel that better to keep it.
>
> Hmm. May be, drop the first assignment of zero to host_offset? We
> actually don't need it, user should not rely on host_offset if we
> return an error.

Yeah, I'll drop the first one and keep this one.

>> @@ -3735,7 +3726,7 @@ static coroutine_fn int 
>> qcow2_co_pwrite_zeroes(BlockDriverState *bs,
>>           offset = QEMU_ALIGN_DOWN(offset, s->cluster_size);
>>           bytes = s->cluster_size;
>
> Unrelated to the patch, but.. Why we change bytes?? So, we can finish
> with success, but zero-out only first cluster?
>
> Ah, found, generic block-layer take care of it and never issue
> unaligned requests crossing cluster boundary.

That's right, hence the assert(head + bytes <= s->cluster_size); a few
lines before.

Berto