Patchwork [v3,1/4] vmdk: Refactor vmdk_create_extent

login
register
mail settings
Submitter Kevin Wolf
Date Dec. 6, 2018, 3:13 p.m.
Message ID <20181206151304.8388-2-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/674225/
State New
Headers show

Comments

Kevin Wolf - Dec. 6, 2018, 3:13 p.m.
From: Fam Zheng <famz@redhat.com>

The extracted vmdk_init_extent takes a BlockBackend object and
initializes the format metadata. It is the common part between "qemu-img
create" and "blockdev-create".

Add a "BlockBackend *pbb" parameter to vmdk_create_extent, to return the
opened BB to the caller in the next patch.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 69 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 26 deletions(-)
Markus Armbruster - Dec. 7, 2018, 6:40 a.m.
Kevin Wolf <kwolf@redhat.com> writes:

> From: Fam Zheng <famz@redhat.com>
>
> The extracted vmdk_init_extent takes a BlockBackend object and
> initializes the format metadata. It is the common part between "qemu-img
> create" and "blockdev-create".
>
> Add a "BlockBackend *pbb" parameter to vmdk_create_extent, to return the
> opened BB to the caller in the next patch.

I'd do this part in the next patch, when it has a user.  Matter of
taste.

>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vmdk.c | 69 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 43 insertions(+), 26 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2c9e86d98f..32fc2c84b3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1741,35 +1741,17 @@ static int coroutine_fn vmdk_co_pwrite_zeroes(BlockDriverState *bs,
>      return ret;
>  }
>  
> -static int vmdk_create_extent(const char *filename, int64_t filesize,
> -                              bool flat, bool compress, bool zeroed_grain,
> -                              QemuOpts *opts, Error **errp)
> +static int vmdk_init_extent(BlockBackend *blk,
> +                            int64_t filesize, bool flat,
> +                            bool compress, bool zeroed_grain,
> +                            Error **errp)
>  {
>      int ret, i;
> -    BlockBackend *blk = NULL;
>      VMDK4Header header;
> -    Error *local_err = NULL;
>      uint32_t tmp, magic, grains, gd_sectors, gt_size, gt_count;
>      uint32_t *gd_buf = NULL;
>      int gd_buf_size;
>  
> -    ret = bdrv_create_file(filename, opts, &local_err);
> -    if (ret < 0) {
> -        error_propagate(errp, local_err);
> -        goto exit;
> -    }
> -
> -    blk = blk_new_open(filename, NULL, NULL,
> -                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> -                       &local_err);
> -    if (blk == NULL) {
> -        error_propagate(errp, local_err);
> -        ret = -EIO;
> -        goto exit;
> -    }
> -
> -    blk_set_allow_write_beyond_eof(blk, true);
> -
>      if (flat) {
>          ret = blk_truncate(blk, filesize, PREALLOC_MODE_OFF, errp);
>          goto exit;
> @@ -1863,15 +1845,50 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>                       gd_buf, gd_buf_size, 0);
>      if (ret < 0) {
>          error_setg(errp, QERR_IO_ERROR);
> -        goto exit;
>      }
>  
>      ret = 0;
> +exit:
> +    g_free(gd_buf);
> +    return ret;
> +}
> +
> +static int vmdk_create_extent(const char *filename, int64_t filesize,
> +                              bool flat, bool compress, bool zeroed_grain,
> +                              BlockBackend **pbb,
> +                              QemuOpts *opts, Error **errp)
> +{
> +    int ret;
> +    BlockBackend *blk = NULL;
> +    Error *local_err = NULL;
> +
> +    ret = bdrv_create_file(filename, opts, &local_err);
> +    if (ret < 0) {
> +        error_propagate(errp, local_err);
> +        goto exit;
> +    }
> +
> +    blk = blk_new_open(filename, NULL, NULL,
> +                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> +                       &local_err);
> +    if (blk == NULL) {
> +        error_propagate(errp, local_err);
> +        ret = -EIO;
> +        goto exit;
> +    }
> +
> +    blk_set_allow_write_beyond_eof(blk, true);
> +
> +    ret = vmdk_init_extent(blk, filesize, flat, compress, zeroed_grain, errp);
>  exit:
>      if (blk) {
> -        blk_unref(blk);
> +        if (pbb) {
> +            *pbb = blk;
> +        } else {
> +            blk_unref(blk);
> +            blk = NULL;

Dead assignment.  Matter of taste.

> +        }
>      }
> -    g_free(gd_buf);
>      return ret;
>  }
>  
> @@ -2094,7 +2111,7 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>          snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
>  
>          if (vmdk_create_extent(ext_filename, size,
> -                               flat, compress, zeroed_grain, opts, errp)) {
> +                               flat, compress, zeroed_grain, NULL, opts, errp)) {
>              ret = -EINVAL;
>              goto exit;
>          }

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Kevin Wolf - Dec. 7, 2018, 10:24 a.m.
Am 07.12.2018 um 07:40 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > From: Fam Zheng <famz@redhat.com>
> >
> > The extracted vmdk_init_extent takes a BlockBackend object and
> > initializes the format metadata. It is the common part between "qemu-img
> > create" and "blockdev-create".
> >
> > Add a "BlockBackend *pbb" parameter to vmdk_create_extent, to return the
> > opened BB to the caller in the next patch.
> 
> I'd do this part in the next patch, when it has a user.  Matter of
> taste.

I kept Fam as the author of the patches (except for the test case which
is effectively a rewrite with Fam's test as the spec), so I'd like to
do only the minimal editing necessary to make the patches mergeable. If
it's a matter of taste, I'll leave it as it is.

The same goes for code changes that are a matter of taste: If you
_really_ care about taste in the vmdk code, feel free to send a
follow-up and I'll merge it on top, but I'll leave Fam's patches alone
as much as I can.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

Kevin
Markus Armbruster - Dec. 7, 2018, 12:52 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 07.12.2018 um 07:40 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > From: Fam Zheng <famz@redhat.com>
>> >
>> > The extracted vmdk_init_extent takes a BlockBackend object and
>> > initializes the format metadata. It is the common part between "qemu-img
>> > create" and "blockdev-create".
>> >
>> > Add a "BlockBackend *pbb" parameter to vmdk_create_extent, to return the
>> > opened BB to the caller in the next patch.
>> 
>> I'd do this part in the next patch, when it has a user.  Matter of
>> taste.
>
> I kept Fam as the author of the patches (except for the test case which
> is effectively a rewrite with Fam's test as the spec), so I'd like to
> do only the minimal editing necessary to make the patches mergeable. If
> it's a matter of taste, I'll leave it as it is.

That's fine.  Matter of taste means I'm stating mine without demanding
you change the patch to conform to it.

> The same goes for code changes that are a matter of taste: If you
> _really_ care about taste in the vmdk code, feel free to send a
> follow-up and I'll merge it on top, but I'll leave Fam's patches alone
> as much as I can.

Makes sense.

>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks!
>
> Kevin

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index 2c9e86d98f..32fc2c84b3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1741,35 +1741,17 @@  static int coroutine_fn vmdk_co_pwrite_zeroes(BlockDriverState *bs,
     return ret;
 }
 
-static int vmdk_create_extent(const char *filename, int64_t filesize,
-                              bool flat, bool compress, bool zeroed_grain,
-                              QemuOpts *opts, Error **errp)
+static int vmdk_init_extent(BlockBackend *blk,
+                            int64_t filesize, bool flat,
+                            bool compress, bool zeroed_grain,
+                            Error **errp)
 {
     int ret, i;
-    BlockBackend *blk = NULL;
     VMDK4Header header;
-    Error *local_err = NULL;
     uint32_t tmp, magic, grains, gd_sectors, gt_size, gt_count;
     uint32_t *gd_buf = NULL;
     int gd_buf_size;
 
-    ret = bdrv_create_file(filename, opts, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        goto exit;
-    }
-
-    blk = blk_new_open(filename, NULL, NULL,
-                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                       &local_err);
-    if (blk == NULL) {
-        error_propagate(errp, local_err);
-        ret = -EIO;
-        goto exit;
-    }
-
-    blk_set_allow_write_beyond_eof(blk, true);
-
     if (flat) {
         ret = blk_truncate(blk, filesize, PREALLOC_MODE_OFF, errp);
         goto exit;
@@ -1863,15 +1845,50 @@  static int vmdk_create_extent(const char *filename, int64_t filesize,
                      gd_buf, gd_buf_size, 0);
     if (ret < 0) {
         error_setg(errp, QERR_IO_ERROR);
-        goto exit;
     }
 
     ret = 0;
+exit:
+    g_free(gd_buf);
+    return ret;
+}
+
+static int vmdk_create_extent(const char *filename, int64_t filesize,
+                              bool flat, bool compress, bool zeroed_grain,
+                              BlockBackend **pbb,
+                              QemuOpts *opts, Error **errp)
+{
+    int ret;
+    BlockBackend *blk = NULL;
+    Error *local_err = NULL;
+
+    ret = bdrv_create_file(filename, opts, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        goto exit;
+    }
+
+    blk = blk_new_open(filename, NULL, NULL,
+                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+                       &local_err);
+    if (blk == NULL) {
+        error_propagate(errp, local_err);
+        ret = -EIO;
+        goto exit;
+    }
+
+    blk_set_allow_write_beyond_eof(blk, true);
+
+    ret = vmdk_init_extent(blk, filesize, flat, compress, zeroed_grain, errp);
 exit:
     if (blk) {
-        blk_unref(blk);
+        if (pbb) {
+            *pbb = blk;
+        } else {
+            blk_unref(blk);
+            blk = NULL;
+        }
     }
-    g_free(gd_buf);
     return ret;
 }
 
@@ -2094,7 +2111,7 @@  static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
         snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
 
         if (vmdk_create_extent(ext_filename, size,
-                               flat, compress, zeroed_grain, opts, errp)) {
+                               flat, compress, zeroed_grain, NULL, opts, errp)) {
             ret = -EINVAL;
             goto exit;
         }