From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:49390)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1axOX0-00078h-3T
	for qemu-devel@nongnu.org; Mon, 02 May 2016 20:49:48 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1axOWo-0005GU-Dq
	for qemu-devel@nongnu.org; Mon, 02 May 2016 20:49:36 -0400
Received: from mx1.redhat.com ([209.132.183.28]:51273)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1axOWo-0005EO-7Z
	for qemu-devel@nongnu.org; Mon, 02 May 2016 20:49:30 -0400
Date: Tue, 3 May 2016 08:49:21 +0800
From: Fam Zheng <famz@redhat.com>
Message-ID: <20160503004921.GR1421@ad.usersys.redhat.com>
References: <1461266977-6045-1-git-send-email-Janne.Karhunen@gmail.com>
	<20160429074107.GM1421@ad.usersys.redhat.com>
	<CAE=NcrZb4CC6Lny2hqUF9yvbkOCgKMSAFPXL-4Sr0rULnC1T-g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAE=NcrZb4CC6Lny2hqUF9yvbkOCgKMSAFPXL-4Sr0rULnC1T-g@mail.gmail.com>
Subject: Re: [Qemu-devel] [PATCH] Allow users to specify the vmdk virtual
 hardware version.
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Janne Karhunen <janne.karhunen@gmail.com>
Cc: qemu-devel@nongnu.org

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