Re: [RFC PATCH v2 01/26] qcow2: Add calculate_l2_meta()

[Alberto Garcia]

On Wed 30 Oct 2019 01:04:02 PM CET, Max Reitz wrote:
> (I intended not to comment on such things on an RFC, but here I am...)

No problem with that :-)

> I’d call it host_cluster_offset to make clear that it points to a
> cluster and isn’t the host offset for @guest_offset.

Sure, why not. We can also accept the exact offset within the cluster
and then use start_of_cluster(), but I prefer this one.

> And now that I’ve gone this far already, I might as well say that I’d
> like if it the comment noted that this function not only creates the
> L2Meta structure but also adds it to the cluster_allocs list.

I'll update the comment.

>> + * @guest_offset and @bytes indicate the offset and length of the
>> + * request.
>> + *
>> + * If @keep_old is true it means that the clusters were already
>> + * allocated and will be overwritten. If false then the clusters are
>> + * new and we have to decrease the reference count of the old ones.
>> + */
>> +static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
>> +                              uint64_t guest_offset, uint64_t bytes,
>
> And now I’m so deep into non-RFC-level review territory, I might as
> well say I’d prefer @bytes to be an unsigned (or maybe even a plain
> int), because anything more wouldn’t work.  (Not least because
> cow_end_to is an unsigned).

True, I'll correct that too.

Thanks!

Berto