Re: [PATCH v2 1/3] parallels: Put the image checks in separate functions

[Vladimir Sementsov-Ogievskiy]

On 8/5/22 18:47, alexander.ivanov@virtuozzo.com wrote:
> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

A bit strange to see this. Is your name set up correctly in ~/.gitconfig ?

> We will add more and more checks of images so we need to reorganize the code.
> Put each check to a separate helper function with a separate loop.
> Add two helpers: truncate_file() and sync_header(). They will be used
> in multiple functions.

It would be a lot simpler to review if you split out one check_ function per 
patch :)

> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---

[..]

> -            /*
> -             * In order to really repair the image, we must shrink it.
> -             * That means we have to pass exact=true.
> -             */
> -            ret = bdrv_truncate(bs->file, res->image_end_offset, true,
> -                                PREALLOC_MODE_OFF, 0, &local_err);
> +            ret = truncate_file(bs, res->image_end_offset);
>               if (ret < 0) {
> -                error_report_err(local_err);
> -                res->check_errors++;

Why you remove check_errors++ ? It's an error of truncate_file counted.
With it kept here:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> -                goto out;
> +                return ret;
>               }
>               res->leaks_fixed += count;
>           }

[..]

> +static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> +                                           BdrvCheckResult *res,
> +                                           BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int ret;
> +
> +    qemu_co_mutex_lock(&s->lock);

If touch this anyway, I'd move to QEMU_LOCK_GUARD().

> +
> +    check_unclean(bs, res, fix);
> +
> +    ret = check_cluster_outside_image(bs, res, fix);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    check_fragmentation(bs, res, fix);
> +
> +    ret = check_leak(bs, res, fix);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    collect_statistics(bs, res);

probably, chack_fragmentation() should be part of collect_statistics() (called 
from it, or just merged into same loop, as it fills same res->bfi)

Also while reviewing I noted that when FIX_ERRORS is not enabled we count broken 
clusters (with off > size) as allocated, and also they participate in 
fragmentation statistics. That's preexisting still..


--
Best regards,
Vladimir