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

[Fam Zheng <famz@redhat.com>]

On Mon, 05/02 04:30, Janne Karhunen wrote:
> >>      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.
> 
> Wasn't it removed?
> 
> -#define BLOCK_FLAG_COMPAT6          4

Yes you are right, it's my oversight. :)

> 
> 
> >> +        if (strcmp(hw_version, "4")) {
> >
> > Doesn't compat6 mean hw_version is 6? I think you want s/4/6/ here.
> 
> Yes and that's the functionality here. So the logic is that if
> BLOCK_OPT_COMPAT6 was defined and hw_version had something other than
> "4" we fail saying can't do both.
> 
> Test case:
> qemu-img convert -f vmdk -O vmdk img.vmdk img.vmdk-output
> ddb.virtualHWVersion = "4" (default)

Yes.

> 
> qemu-img convert -f vmdk -O vmdk -o compat6=on img.vmdk img.vmdk-output
> ddb.virtualHWVersion = "6" (compat6 default)

Yes.

> 
> qemu-img convert -f vmdk -O vmdk -o hwversion=9 img.vmdk img.vmdk-output
> ddb.virtualHWVersion = "9" (the new feature)

Yes.

But you get:

1)
    $ qemu-img create -f vmdk -o compat6=on,hwversion=4 /tmp/a.vmdk 1G; grep -a
    virtualHWVersion /tmp/a.vmdk Formatting '/tmp/a.vmdk', fmt=vmdk size=1073741824 compat6=on hwversion=4
    ddb.virtualHWVersion = "6"

which is wrong.

2) IMO if the documentation says "the options are mutually exclusive" instead
of "specific value combinations are invalid", this:

    $ qemu-img create -f vmdk -o compat6=off,hwversion=9 /tmp/a.vmdk 1G; grep -a virtualHWVersion /tmp/a.vmdk
    Formatting '/tmp/a.vmdk', fmt=vmdk size=1073741824 compat6=off hwversion=9
    ddb.virtualHWVersion = "9"

should better be rejected:

    $ qemu-img create -f vmdk -o compat6=off,hwversion=9 /tmp/a.vmdk 1G; grep -a virtualHWVersion /tmp/a.vmdk
    Formatting '/tmp/a.vmdk', fmt=vmdk size=1073741824 compat6=off hwversion=9
    qemu-img: /tmp/a.vmdk: compat6 and hwversion are mutually exclusive

1) is a bug in your code, and 2) is a documenting problem.

Fam