Re: [Qemu-devel] [PATCH v13 05/12] qcow2: Name typedef for cluster type

[Max Reitz]

On 08.05.2017 17:43, Max Reitz wrote:
> On 07.05.2017 02:05, Eric Blake wrote:
>> Although it doesn't add all that much type safety (this is C, after
>> all), it does add a bit of legibility to use the name QCow2ClusterType
>> instead of a plain int.
>>
>> In particular, qcow2_get_cluster_offset() has an overloaded return
>> type; a QCow2ClusterType on success, and -errno on failure; keeping
>> the cluster type in a separate variable makes it slightly easier for
>> the next patch to make further computations based on the type.
>>
>> Suggested-by: Max Reitz <address@hidden>
>> Signed-off-by: Eric Blake <address@hidden>
>>
>> ---
>> v13: new patch
>> ---
>>  block/qcow2.h          |  6 +++---
>>  block/qcow2-cluster.c  | 17 +++++++++--------
>>  block/qcow2-refcount.c |  2 +-
>>  3 files changed, 13 insertions(+), 12 deletions(-)
> 
> [...]
> 
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index f3bfce6..ed78a30 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
> 
> Above this hunk, there is count_contiguous_clusters() with a variable
> "first_cluster_type" that could be a QCow2ClusterType as well.
> 
>> @@ -340,7 +340,7 @@ static int count_contiguous_clusters(int nb_clusters, 
>> int cluster_size,
>>   */
>>  static int count_contiguous_clusters_unallocated(int nb_clusters,
>>                                                   uint64_t *l2_table,
>> -                                                 int wanted_type)
>> +                                                 QCow2ClusterType 
>> wanted_type)
>>  {
>>      int i;
>>
> 
> And some lines below this (in this function), there is a "type" variable
> that (c|sh)ould be a QCow2ClusterType, too.

I see you're doing this in the next patch... Well, that's not really
where it belongs, though.

> Although it's quite a functional change, I would be willing to change
> both when applying, if you allowed me to.

I guess I'm extending my offer to handle the rebase fallout of the next
patch, too...?

Max

> (Once again, "no good deed shall go unpunished", as Markus likes to say :-))
> 
> Max
>

signature.asc
Description: OpenPGP digital signature