Re: [Qemu-devel] [PATCH 3/4] block: vhdx - change .vhdx_create default block state to ZERO

[Max Reitz <mreitz@redhat.com>]

On 2014-12-08 at 07:07, Jeff Cody wrote:
> The VHDX spec specifies that the default new block state is
> PAYLOAD_BLOCK_NOT_PRESENT for a dynamic VHDX image, and
> PAYLOAD_BLOCK_FULLY_PRESENT for a fixed VHDX image.
>
> However, in order to create space-efficient VHDX images with qemu-img
> convert, it is desirable to be able to set has_zero_init to true for
> VHDX.
>
> There is currently an option when creating VHDX images, to use block
> state ZERO for new blocks.  However, this currently defaults to 'off'.
> In order to be able to eventually set has_zero_init to true for VHDX,
> this needs to default to 'on'.
>
> This patch changes the default to 'on', and provides some help
> information to warn against setting it to 'off' when using qemu-img
> convert.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/vhdx.c  | 6 ++++--
>   qemu-doc.texi | 6 +++++-
>   2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index bec10bd..ddefc2a 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1774,7 +1774,7 @@ static int vhdx_create(const char *filename, QemuOpts *opts, Error **errp)
>       log_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_LOG_SIZE, 0);
>       block_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_BLOCK_SIZE, 0);
>       type = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> -    use_zero_blocks = qemu_opt_get_bool_del(opts, VHDX_BLOCK_OPT_ZERO, false);
> +    use_zero_blocks = qemu_opt_get_bool_del(opts, VHDX_BLOCK_OPT_ZERO, true);
>   
>       if (image_size > VHDX_MAX_IMAGE_SIZE) {
>           error_setg_errno(errp, EINVAL, "Image size too large; max of 64TB");
> @@ -1936,7 +1936,9 @@ static QemuOptsList vhdx_create_opts = {
>          {
>              .name = VHDX_BLOCK_OPT_ZERO,
>              .type = QEMU_OPT_BOOL,
> -           .help = "Force use of payload blocks of type 'ZERO'.  Non-standard."
> +           .help = "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"

Full stop missing?

>          },
>          { NULL }
>       }
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index ad418f8..aabe8df 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -698,7 +698,11 @@ Supported options:
>   Specifies which VHDX subformat to use. Valid options are
>   @code{dynamic} (default) and @code{fixed}.
>   @item block_state_zero
> -Force use of payload blocks of type 'ZERO'.
> +Force use of payload blocks of type 'ZERO'.  Can be set to @code{on} (default)
> +or @code{off}.  When set to @code{off}, new blocks will be created as
> +@code{PAYLOAD_BLOCK_NOT_PRESENT}, which means parsers are free to return
> +arbitrary data for those blocks.  Do not set to @code{off} when using
> +@code{qemu-img convert} with @code{subformat=dynamic}.

Well, it's fine to use block_state_zero=off with -S 0, but then you 
won't get much usage out of having set it to off because all blocks will 
be allocated anyway.

>   @item block_size
>   Block size; min 1 MB, max 256 MB.  0 means auto-calculate based on image size.
>   @item log_size

With the full stop added:

Reviewed-by: Max Reitz <mreitz@redhat.com>