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

[Max Reitz <mreitz@redhat.com>]

On 15.08.2014 14:31, Max Reitz wrote:
> 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.

Okay, if I add an explicit patch for fixing the coding style issues, I 
probably need to fix all of qcow2-refcount.c. But looking at it, this 
patch would probably be rather large, so I guess I'll just keep the 
issues and don't fix them at all.

Max

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