[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 03/21] parallels: invent parallels_opts_prealloc() helper to
From: |
Mike Maslenkin |
Subject: |
Re: [PATCH 03/21] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts |
Date: |
Sun, 17 Sep 2023 14:40:06 +0300 |
This is not introduced by this patch,
but looks like qemu_opts_del(opts) missed.
On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev <den@openvz.org> 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 | 65 +++++++++++++++++++++++++++--------------------
> 1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 428f72de1c..1d5409f2ba 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1025,6 +1025,38 @@ 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)
> +{
> + char *buf;
> + int64_t bytes;
> + BDRVParallelsState *s = bs->opaque;
> + Error *local_err = NULL;
> + QemuOpts *opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0,
> errp);
> + if (!opts) {
> + return -ENOMEM;
> + }
> +
> + if (!qemu_opts_absorb_qdict(opts, options, errp)) {
> + return -EINVAL;
> + }
> +
> + bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
> + s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
qemu_opt_get_size_del returns uint64_t, so what's a reason to declare
"bytes" variable as int64_t
and then shift it to the right? I see here it can not be negative,
but it's a common to use signed values and not to add explicit check
before shifting to right In this file
I takes time to ensure that initial values are not negative.
Regards,
Mike.
> + 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);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> Error **errp)
> {
> @@ -1033,11 +1065,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 +1112,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 +1152,6 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> s->header_size = size;
> }
>
> - opts = qemu_opts_create(¶llels_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,7 +1226,6 @@ 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:
> /*
> --
> 2.34.1
>
- [PATCH 00/21] implement discard operation for Parallels images, Denis V. Lunev, 2023/09/15
- [PATCH 04/21] parallels: return earler in fail_format branch in parallels_open(), Denis V. Lunev, 2023/09/15
- [PATCH 03/21] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts, Denis V. Lunev, 2023/09/15
- [PATCH 01/21] parallels: fix formatting in bdrv_parallels initialization, Denis V. Lunev, 2023/09/15
- [PATCH 02/21] parallels: mark driver as supporting CBT, Denis V. Lunev, 2023/09/15
- [PATCH 07/21] parallels: create mark_used() helper which sets bit in used bitmap, Denis V. Lunev, 2023/09/15
- [PATCH 08/21] tests: ensure that image validation will not cure the corruption, Denis V. Lunev, 2023/09/15