[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v13 05/12] qcow2: Name typedef for cluster type
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v13 05/12] qcow2: Name typedef for cluster type |
Date: |
Mon, 8 May 2017 17:45:24 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1 |
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
- [Qemu-block] [PATCH v13 00/12] qcow2 zero-cluster tweaks [was add blkdebug tests], Eric Blake, 2017/05/06
- [Qemu-block] [PATCH v13 03/12] block: Update comments on BDRV_BLOCK_* meanings, Eric Blake, 2017/05/06
- [Qemu-block] [PATCH v13 04/12] qcow2: Correctly report status of preallocated zero clusters, Eric Blake, 2017/05/06
- [Qemu-block] [PATCH v13 05/12] qcow2: Name typedef for cluster type, Eric Blake, 2017/05/06
- [Qemu-block] [PATCH v13 06/12] qcow2: Make distinction between zero cluster types obvious, Eric Blake, 2017/05/06
[Qemu-block] [PATCH v13 08/12] iotests: Improve _filter_qemu_img_map, Eric Blake, 2017/05/06
[Qemu-block] [PATCH v13 11/12] qcow2: Assert that cluster operations are aligned, Eric Blake, 2017/05/06
[Qemu-block] [PATCH v13 07/12] qcow2: Optimize zero_single_l2() to minimize L2 churn, Eric Blake, 2017/05/06
[Qemu-block] [PATCH v13 09/12] iotests: Add test 179 to cover write zeroes with unmap, Eric Blake, 2017/05/06
[Qemu-block] [PATCH v13 12/12] qcow2: Discard/zero clusters by byte count, Eric Blake, 2017/05/06