Re: [PATCH 04/22] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts

[Alexander Ivanov]

On 9/18/23 20:00, Denis V. Lunev wrote:
> This patch creates above mentioned helper and moves its usage to the
> beginning of parallels_open(). This simplifies parallels_open() a bit.
>
> The patch also ensures that we store prealloc_size on block driver state
> always in sectors. This makes code cleaner and avoids wrong opinion at
> the assignment that the value is in bytes.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 72 +++++++++++++++++++++++++++++------------------
>   1 file changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index af7be427c9..ae006e7fc7 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1025,6 +1025,44 @@ static int parallels_update_header(BlockDriverState *bs)
>       return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
>   }
>   
> +
> +static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
> +                                   Error **errp)
> +{
> +    int err;
> +    char *buf;
> +    int64_t bytes;
> +    BDRVParallelsState *s = bs->opaque;
> +    Error *local_err = NULL;
> +    QemuOpts *opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
> +    if (!opts) {
> +        return -ENOMEM;
> +    }
> +
> +    err = -EINVAL;
> +    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
> +        goto done;
> +    }
> +
> +    bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
> +    s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
> +    buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
> +    /* prealloc_mode can be downgraded later during allocate_clusters */
> +    s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
> +                                       PRL_PREALLOC_MODE_FALLOCATE,
> +                                       &local_err);
> +    g_free(buf);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        goto done;
> +    }
> +    err = 0;
> +
> +done:
> +    qemu_opts_del(opts);
> +    return err;
> +}
> +
>   static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>                             Error **errp)
>   {
> @@ -1033,11 +1071,13 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>       int ret, size, i;
>       int64_t file_nb_sectors, sector;
>       uint32_t data_start;
> -    QemuOpts *opts = NULL;
> -    Error *local_err = NULL;
> -    char *buf;
>       bool data_off_is_correct;
>   
> +    ret = parallels_opts_prealloc(bs, options, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
>       ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
>       if (ret < 0) {
>           return ret;
> @@ -1078,6 +1118,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>           ret = -EFBIG;
>           goto fail;
>       }
> +    s->prealloc_size = MAX(s->tracks, s->prealloc_size);
>       s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>   
>       s->bat_size = le32_to_cpu(ph.bat_entries);
> @@ -1117,29 +1158,6 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>           s->header_size = size;
>       }
>   
> -    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
> -    if (!opts) {
> -        goto fail_options;
> -    }
> -
> -    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
> -        goto fail_options;
> -    }
> -
> -    s->prealloc_size =
> -        qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
> -    s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
> -    buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
> -    /* prealloc_mode can be downgraded later during allocate_clusters */
> -    s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
> -                                       PRL_PREALLOC_MODE_FALLOCATE,
> -                                       &local_err);
> -    g_free(buf);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        goto fail_options;
> -    }
> -
>       if (ph.ext_off) {
>           if (flags & BDRV_O_RDWR) {
>               /*
> @@ -1214,10 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>   
>   fail_format:
>       error_setg(errp, "Image not in Parallels format");
> -fail_options:
>       ret = -EINVAL;
>   fail:
> -    qemu_opts_del(opts);
>       /*
>        * "s" object was allocated by g_malloc0 so we can safely
>        * try to free its fields even they were not allocated.

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

--
Best regards,
Alexander Ivanov