diff mbox series

[03/21] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts

Message ID 20230915184130.403366-5-den@openvz.org (mailing list archive)
State New, archived
Headers show
Series implement discard operation for Parallels images | expand

Commit Message

Denis V. Lunev Sept. 15, 2023, 6:41 p.m. UTC 
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(-)

Comments

Mike Maslenkin Sept. 17, 2023, 11:40 a.m. UTC | #1 
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(&parallels_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(&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,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
>
Denis V. Lunev Sept. 17, 2023, 1:06 p.m. UTC | #2 
On 9/17/23 13:40, Mike Maslenkin wrote:
> This is not introduced by this patch,
> but looks like qemu_opts_del(opts) missed.
nice spot!


> 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(&parallels_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.
>

apparently no specific reason. Just all variables here
dealing with offsets are int64_t. Will change.

Thanks,
     Den
Alexander Ivanov Sept. 18, 2023, 3:21 p.m. UTC | #3 
On 9/15/23 20:41, 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 | 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(&parallels_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;
> +    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(&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,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:
>       /*

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

Patch

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(&parallels_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;
+    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(&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,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:
     /*