Re: [PATCH v5 5/5] parallels: Image repairing in parallels_open()

[Hanna Czenczek <hreitz@redhat.com>]

On 29.05.23 17:15, Alexander Ivanov wrote:
> Repair an image at opening if the image is unclean or out-of-image
> corruption was detected.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 65 +++++++++++++++++++++++++----------------------
>   1 file changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index d64e8007d5..7bbd5cb112 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c

[...]

> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           goto fail;
>       }
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        sector = bat2sect(s, i);
> +        if (sector + s->tracks > s->data_end) {
> +            s->data_end = sector + s->tracks;
> +        }
> +    }
> +
> +    /*
> +     * We don't repair the image here if it's opened for checks. Also we don't
> +     * want to change inactive images and can't change readonly images.
> +     */
> +    if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Repair the image if it's dirty or
> +     * out-of-image corruption was detected.
> +     */
> +    if (s->data_end > file_nb_sectors || s->header_unclean) {
> +        BdrvCheckResult res;
> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0) {

Should we also verify that res->corruptions == res->corruptions_fixed && 
res->check_errors == 0?

> +            error_free(s->migration_blocker);

I’d move this clean-up to a new error path below, then we could even 
reuse that where migrate_add_blocker() fails.

Anyway, not wrong as-is, just suggestion, so:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> +            error_setg_errno(errp, -ret, "Could not repair corrupted image");
> +            goto fail;
> +        }
> +    }
> +
>       return 0;
>   
>   fail_format: