Re: [Qemu-devel] [PATCH] Allow users to specify the vmdk virtual hardware version.

[Fam Zheng]

On Thu, 04/21 12:29, Janne Karhunen wrote:
> From: Janne Karhunen <address@hidden>
> 
> Vmdk images have metadata to indicate the vmware virtual
> hardware version image was created/tested to run with.
> Allow users to specify that version via new 'hwversion'
> option.
> 
> Signed-off-by: Janne Karhunen <address@hidden>

Sorry for the late reply, I was distracted..

> ---
>  block/vmdk.c              | 23 +++++++++++++++++++----
>  include/block/block_int.h |  2 +-
>  qemu-doc.texi             |  3 +++
>  3 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 45f9d3c..4b5cac8 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1829,8 +1829,8 @@ static int vmdk_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>      int64_t total_size = 0, filesize;
>      char *adapter_type = NULL;
>      char *backing_file = NULL;
> +    char *hw_version = NULL;
>      char *fmt = NULL;
> -    int flags = 0;
>      int ret = 0;
>      bool flat, split, compress;
>      GString *ext_desc_lines;
> @@ -1861,7 +1861,7 @@ static int vmdk_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>          "# The Disk Data Base\n"
>          "#DDB\n"
>          "\n"
> -        "ddb.virtualHWVersion = \"%d\"\n"
> +        "ddb.virtualHWVersion = \"%s\"\n"
>          "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
>          "ddb.geometry.heads = \"%" PRIu32 "\"\n"
>          "ddb.geometry.sectors = \"63\"\n"
> @@ -1878,8 +1878,16 @@ static int vmdk_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>                            BDRV_SECTOR_SIZE);
>      adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
>      backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> +    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
>      if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
> -        flags |= BLOCK_FLAG_COMPAT6;

Please remove BLOCK_FLAG_COMPAT6 from include/block/block_int.h| as well.

> +        if (strcmp(hw_version, "4")) {

Doesn't compat6 mean hw_version is 6? I think you want s/4/6/ here.

> +            error_setg(errp,
> +                       "compat6 and hwversion are mutually exclusive");
> +            ret = -EINVAL;
> +            goto exit;
> +        }
> +        g_free(hw_version);
> +        hw_version = g_strdup("6");
>      }
>      fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
>      if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
> @@ -2001,7 +2009,7 @@ static int vmdk_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>                             fmt,
>                             parent_desc_line,
>                             ext_desc_lines->str,
> -                           (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
> +                           hw_version,
>                             total_size /
>                                 (int64_t)(63 * number_heads * 
> BDRV_SECTOR_SIZE),
>                             number_heads,
> @@ -2047,6 +2055,7 @@ exit:
>      }
>      g_free(adapter_type);
>      g_free(backing_file);
> +    g_free(hw_version);
>      g_free(fmt);
>      g_free(desc);
>      g_free(path);
> @@ -2298,6 +2307,12 @@ static QemuOptsList vmdk_create_opts = {
>              .def_value_str = "off"
>          },
>          {
> +            .name = BLOCK_OPT_HWVERSION,
> +            .type = QEMU_OPT_STRING,
> +            .help = "VMDK hardware version",
> +            .def_value_str = "4"
> +        },
> +        {
>              .name = BLOCK_OPT_SUBFMT,
>              .type = QEMU_OPT_STRING,
>              .help =
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 10d8759..931a412 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -38,12 +38,12 @@
>  #include "qemu/throttle.h"
>  
>  #define BLOCK_FLAG_ENCRYPT          1
> -#define BLOCK_FLAG_COMPAT6          4
>  #define BLOCK_FLAG_LAZY_REFCOUNTS   8
>  
>  #define BLOCK_OPT_SIZE              "size"
>  #define BLOCK_OPT_ENCRYPT           "encryption"
>  #define BLOCK_OPT_COMPAT6           "compat6"
> +#define BLOCK_OPT_HWVERSION         "hwversion"
>  #define BLOCK_OPT_BACKING_FILE      "backing_file"
>  #define BLOCK_OPT_BACKING_FMT       "backing_fmt"
>  #define BLOCK_OPT_CLUSTER_SIZE      "cluster_size"
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 79141d3..d3a5f94 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -693,6 +693,9 @@ Supported options:
>  File name of a base image (see @option{create} subcommand).
>  @item compat6
>  Create a VMDK version 6 image (instead of version 4)
> address@hidden hwversion
> +Specify vmdk virtual hardware version. Option is mutually exclusive
> +with the compat6 flag.

The code is not really treating them exclusively, it only checks for
contradiction of values.  So I suggest either fix the document or the code.

Fam

>  @item subformat
>  Specifies which VMDK subformat to use. Valid options are
>  @code{monolithicSparse} (default),
> -- 
> 1.9.1
> 
>