Re: [Qemu-devel] [PATCH 2/8] qcow2: Factor out refcount comparison for check

[Max Reitz <mreitz@redhat.com>]

On 14.08.2014 14:02, Benoît Canet wrote:
> The Wednesday 13 Aug 2014 à 23:01:44 (+0200), Max Reitz wrote :
>> Put the code for comparing the calculated refcounts against the on-disk
>> refcounts during qemu-img check into an own function.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-refcount.c | 91 ++++++++++++++++++++++++++++----------------------
>>   1 file changed, 52 insertions(+), 39 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 9793c27..d1da8d5 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1629,46 +1629,22 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>   }
>>   
>>   /*
>> - * Checks an image for refcount consistency.
>> - *
>> - * Returns 0 if no errors are found, the number of errors in case the image is
>> - * detected as corrupted, and -errno when an internal error occurred.
>> + * Compares the actual reference count for each cluster in the image against the
>> + * refcount as reported by the refcount structures on-disk.
>>    */
>> -int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>> -                          BdrvCheckMode fix)
>> +static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>> +                              BdrvCheckMode fix, int64_t *highest_cluster,
>> +                              uint16_t *refcount_table, int64_t nb_clusters)
>>   {
>>       BDRVQcowState *s = bs->opaque;
>> -    int64_t size, i, highest_cluster, nb_clusters;
>> -    int refcount1, refcount2;
>> -    uint16_t *refcount_table = NULL;
>> -    int ret;
>> -
>> -    size = bdrv_getlength(bs->file);
>> -    if (size < 0) {
>> -        res->check_errors++;
>> -        return size;
>> -    }
>> -
>> -    nb_clusters = size_to_clusters(s, size);
>> -    if (nb_clusters > INT_MAX) {
>> -        res->check_errors++;
>> -        return -EFBIG;
>> -    }
>> -
>> -    res->bfi.total_clusters =
>> -        size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE);
>> -
>> -    ret = calculate_refcounts(bs, res, fix, &refcount_table, &nb_clusters);
>> -    if (ret < 0) {
>> -        goto fail;
>> -    }
>> +    int64_t i;
>> +    int refcount1, refcount2, ret;
>>   
>> -    /* compare ref counts */
>> -    for (i = 0, highest_cluster = 0; i < nb_clusters; i++) {
>> +    for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) {
>>           refcount1 = get_refcount(bs, i);
>>           if (refcount1 < 0) {
>>               fprintf(stderr, "Can't get refcount for cluster %" PRId64 ": %s\n",
>> -                i, strerror(-refcount1));
>> +                    i, strerror(-refcount1));
> Free coding fix.
>
>>               res->check_errors++;
>>               continue;
>>           }
>> @@ -1676,11 +1652,10 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>           refcount2 = refcount_table[i];
>>   
>>           if (refcount1 > 0 || refcount2 > 0) {
>> -            highest_cluster = i;
>> +            *highest_cluster = i;
> idem.
>>           }
>>   
>>           if (refcount1 != refcount2) {
>> -
> Spurious blank line removal.
>
>>               /* Check if we're allowed to fix the mismatch */
>>               int *num_fixed = NULL;
>>               if (refcount1 > refcount2 && (fix & BDRV_FIX_LEAKS)) {
>> @@ -1690,10 +1665,10 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>               }
>>   
>>               fprintf(stderr, "%s cluster %" PRId64 " refcount=%d reference=%d\n",
>> -                   num_fixed != NULL     ? "Repairing" :
>> -                   refcount1 < refcount2 ? "ERROR" :
>> -                                           "Leaked",
>> -                   i, refcount1, refcount2);
>> +                    num_fixed != NULL     ? "Repairing" :
>> +                    refcount1 < refcount2 ? "ERROR" :
>> +                                            "Leaked",
>> +                    i, refcount1, refcount2);
> Gratuitous coding style fix.
>
> Patch 1 have similar issues.
> I think the patchs would be smaller and cleaner without these spurious coding style fixes.

Well, I did them on purpose to avoid an additional patch. I'll rework it 
for v2.

Max

>>   
>>               if (num_fixed) {
>>                   ret = update_refcount(bs, i << s->cluster_bits, 1,
>> @@ -1713,6 +1688,44 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>               }
>>           }
>>       }
>> +}
>> +
>> +/*
>> + * Checks an image for refcount consistency.
>> + *
>> + * Returns 0 if no errors are found, the number of errors in case the image is
>> + * detected as corrupted, and -errno when an internal error occurred.
>> + */
>> +int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>> +                          BdrvCheckMode fix)
>> +{
>> +    BDRVQcowState *s = bs->opaque;
>> +    int64_t size, highest_cluster, nb_clusters;
>> +    uint16_t *refcount_table = NULL;
>> +    int ret;
>> +
>> +    size = bdrv_getlength(bs->file);
>> +    if (size < 0) {
>> +        res->check_errors++;
>> +        return size;
>> +    }
>> +
>> +    nb_clusters = size_to_clusters(s, size);
>> +    if (nb_clusters > INT_MAX) {
>> +        res->check_errors++;
>> +        return -EFBIG;
>> +    }
>> +
>> +    res->bfi.total_clusters =
>> +        size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE);
>> +
>> +    ret = calculate_refcounts(bs, res, fix, &refcount_table, &nb_clusters);
>> +    if (ret < 0) {
>> +        goto fail;
>> +    }
>> +
>> +    compare_refcounts(bs, res, fix, &highest_cluster, refcount_table,
>> +                      nb_clusters);
>>   
>>       /* check OFLAG_COPIED */
>>       ret = check_oflag_copied(bs, res, fix);
>> -- 
>> 2.0.3
>>
>>