Re: [Qemu-devel] [PATCH 6/7] vhdx: Support .bdrv_co_create

[Max Reitz]

On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to vhdx, which
> enables image creation over QMP.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  qapi/block-core.json |  37 ++++++++++-
>  block/vhdx.c         | 174 
> ++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 167 insertions(+), 44 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2eba0eef7e..3a65909c47 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3699,6 +3699,41 @@
>              '*static':          'bool' } }
>  
>  ##
> +# @BlockdevVhdxSubformat:
> +#
> +# @dynamic: Growing image file
> +# @fixed:   Preallocated fixed-size imge file
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'BlockdevVhdxSubformat',
> +  'data': [ 'dynamic', 'fixed' ] }
> +
> +##
> +# @BlockdevCreateOptionsVhdx:
> +#
> +# Driver specific image creation options for vhdx.
> +#
> +# @file             Node to create the image format on
> +# @size             Size of the virtual disk in bytes
> +# @log-size         Log size in bytes (default: 1 MB)
> +# @block-size       Block size in bytes (default: 1 MB)

Is it?  Currently, the default size is actually 0 and you keep that by a
simple "block_size = vhdx_opts->block_size;" assignment.  But the old
help text also states: "0 means auto-calculate based on image size."
This is reflected in the code, even after this patch.  0 can mean 8, 16,
32, or 64 MB.

> +# @subformat        vhdx subformat (default: dynamic)
> +# @block-state-zero Force use of payload blocks of type 'ZERO'. Non-standard,
> +#                   but default.  Do not set to 'off' when using 'qemu-img
> +#                   convert' with subformat=dynamic.
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsVhdx',
> +  'data': { 'file':                 'BlockdevRef',
> +            'size':                 'size',
> +            '*log-size':            'size',
> +            '*block-size':          'size',
> +            '*subformat':           'BlockdevVhdxSubformat',
> +            '*block-state-zero':    'bool' } }
> +
> +##
>  # @BlockdevCreateNotSupported:
>  #
>  # This is used for all drivers that don't support creating images.

[...]

> index d82350d07c..0ce972381f 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c

[...]

> @@ -1792,54 +1797,63 @@ exit:
>   *    .---- ~ ----------- ~ ------------ ~ ---------------- ~ -----------.
>   *   1MB
>   */
> -static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts 
> *opts,
> -                                            Error **errp)
> +static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
> +                                       Error **errp)
>  {

[...]

>  
> -    if (!strcmp(type, "dynamic")) {
> +    switch (vhdx_opts->subformat) {
> +    case BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC:
>          image_type = VHDX_TYPE_DYNAMIC;
> -    } else if (!strcmp(type, "fixed")) {
> +        break;
> +    case BLOCKDEV_VHDX_SUBFORMAT_FIXED:
>          image_type = VHDX_TYPE_FIXED;
> -    } else if (!strcmp(type, "differencing")) {
> -        error_setg_errno(errp, ENOTSUP,
> -                         "Differencing files not yet supported");
> -        ret = -ENOTSUP;
> -        goto exit;
> -    } else {
> -        error_setg(errp, "Invalid subformat '%s'", type);
> -        ret = -EINVAL;
> -        goto exit;
> +        break;
> +    default:
> +        g_assert_not_reached();
>      }

As a follow-up, it might be reasonable to replace VHDXImageType by
BlockdevVhdxSubformat.

>  
>      /* These are pretty arbitrary, and mainly designed to keep the BAT
> @@ -1865,21 +1879,17 @@ static int coroutine_fn vhdx_co_create_opts(const 
> char *filename, QemuOpts *opts

There is some code not shown here that silently rounds the log_size and
the block_size to 1 MB, and...

>      block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX :
>                                                      block_size;

...this.  In other drivers you seemed to follow the approach of not
doing this kind of rounding in the blockdev-create path but only in the
legacy interface.  Is there a reason for doing it differently here?

Max

>  
> -    ret = bdrv_create_file(filename, opts, &local_err);
> -    if (ret < 0) {
> -        error_propagate(errp, local_err);
> -        goto exit;
> +    /* Create BlockBackend to write to the image */
> +    bs = bdrv_open_blockdev_ref(vhdx_opts->file, errp);
> +    if (bs == NULL) {
> +        return -EIO;
>      }
>  
> -    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 = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
> +    ret = blk_insert_bs(blk, bs, errp);
> +    if (ret < 0) {
> +        goto delete_and_exit;
>      }
> -
>      blk_set_allow_write_beyond_eof(blk, true);
>  
>      /* Create (A) */

signature.asc
Description: OpenPGP digital signature