Re: [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> From: Fam Zheng <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  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 <address@hidden>