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

[Eric Blake]

On 05/08/2017 10:43 AM, 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.

Hmm, I guess I didn't catch them all. Yes, that one definitely qualified.

> 
>> @@ -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.

That one I blame on poor rebasing on my side.

> 
> Although it's quite a functional change, I would be willing to change
> both when applying, if you allowed me to.
> 
> (Once again, "no good deed shall go unpunished", as Markus likes to say :-))

Yes, both changes are appropriate, and I'm fine with you making the
tweaks as part of putting it on your tree (and the corresponding fallout
on 6/12).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature