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

[Denis V. Lunev]

On 1/12/23 16:01, 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 | 67 +++++++++++++++++++++++++----------------------
>   1 file changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 5c9568f197..74f6d00ffb 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -753,7 +753,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState 
> *bs,
>       return ret;
>   }
>   
> -
>   static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>                                               Error **errp)
>   {
> @@ -965,8 +964,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
> -    int ret, size, i;
> -    int64_t file_size;
> +    int ret, size;
> +    int64_t file_size, high_off;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1044,34 +1043,6 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>       }
>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>   
> -    for (i = 0; i < s->bat_size; i++) {
> -        int64_t off = bat2sect(s, i);
> -        if (off >= file_size) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off, i, file_size);
> -            ret = -EINVAL;
> -            goto fail;
> -        }
> -        if (off >= s->data_end) {
> -            s->data_end = off + s->tracks;
> -        }
> -    }
> -
> -    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        /* Image was not closed correctly. The check is mandatory */
> -        s->header_unclean = true;
> -        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> -            error_setg(errp, "parallels: Image was not closed correctly; "
> -                       "cannot be opened read/write");
> -            ret = -EACCES;
> -            goto fail;
> -        }
> -    }
> -
>       opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>       if (!opts) {
>           goto fail_options;
> @@ -1133,7 +1104,41 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>           error_free(s->migration_blocker);
>           goto fail;
>       }
> +
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
> +    if (high_off >= s->data_end) {
> +        s->data_end = high_off + s->tracks;
> +    }
> +
> +    /*
> +     * We don't repair the image here if it is opened for checks and
> +     * shouldn't change the image if BDRV_O_RDWR or BDRV_O_INACTIVE
> +     * flag is present.
> +     */
> +    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_size || s->header_unclean) {
> +        BdrvCheckResult res;
> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret,
> +                             "Could not repair corrupted image");
> +            goto fail;
This leaks s->migration_blocker. Please see error path above, i.e.

    /* Disable migration until bdrv_activate method is added */
    error_setg(&s->migration_blocker, "The Parallels format used by 
node '%s' "
               "does not support live migration",
               bdrv_get_device_or_node_name(bs));
    ret = migrate_add_blocker(s->migration_blocker, errp);
    if (ret < 0) {
        error_free(s->migration_blocker); <--------------------
        goto fail;
    }

Thanks a lot for Mike Maslenkin for the note.

Den


> +        }
> +    }
> +
>       return 0;
>   
>   fail_format: