[Qemu-devel] [PATCH] vmdk: Fix converting to streamOptimized

[Qemu-devel] [PATCH] vmdk: Fix converting to streamOptimized

[Fam Zheng]

Commit d62d9dc4b8 lifted streamOptimized images's version to 3, but we
now refuse to open version 3 images read-write.  We need to make
streamOptimized an exception to allow converting to it. This fixes the
accidentally broken iotests case 059 for the same reason.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 698679d..4a5850b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -571,6 +571,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     VmdkExtent *extent;
     BDRVVmdkState *s = bs->opaque;
     int64_t l1_backup_offset = 0;
+    bool compressed;
 
     ret = bdrv_pread(file->bs, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
@@ -645,6 +646,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         header = footer.header;
     }
 
+    compressed =
+        le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE;
     if (le32_to_cpu(header.version) > 3) {
         char buf[64];
         snprintf(buf, sizeof(buf), "VMDK version %" PRId32,
@@ -652,7 +655,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
                    bdrv_get_device_or_node_name(bs), "vmdk", buf);
         return -ENOTSUP;
-    } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) {
+    } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR) &&
+               !compressed) {
         /* VMware KB 2064959 explains that version 3 added support for
          * persistent changed block tracking (CBT), and backup software can
          * read it as version=1 if it doesn't care about the changed area
-- 
2.4.3

Re: [Qemu-devel] [PATCH] vmdk: Fix converting to streamOptimized

[Kevin Wolf]

Am 25.01.2016 um 03:26 hat Fam Zheng geschrieben:
> Commit d62d9dc4b8 lifted streamOptimized images's version to 3, but we
> now refuse to open version 3 images read-write.  We need to make
> streamOptimized an exception to allow converting to it. This fixes the
> accidentally broken iotests case 059 for the same reason.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

How different are version 3 images for other subformats? Are we
arbitrarily restrictring their use or is it really that they don't work
with our driver? And if they don't work with our driver, are we sure
that streamOptimized images can't use the features we don't support?

Or is the version defined per subformat and doesn't necessarily exist
for other types?

Kevin

Re: [Qemu-devel] [PATCH] vmdk: Fix converting to streamOptimized

[Fam Zheng]

On Mon, 01/25 12:16, Kevin Wolf wrote:
> Am 25.01.2016 um 03:26 hat Fam Zheng geschrieben:
> > Commit d62d9dc4b8 lifted streamOptimized images's version to 3, but we
> > now refuse to open version 3 images read-write.  We need to make
> > streamOptimized an exception to allow converting to it. This fixes the
> > accidentally broken iotests case 059 for the same reason.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> How different are version 3 images for other subformats? Are we
> arbitrarily restrictring their use or is it really that they don't work
> with our driver? And if they don't work with our driver, are we sure
> that streamOptimized images can't use the features we don't support?
> 
> Or is the version defined per subformat and doesn't necessarily exist
> for other types?

Version 3 images are undocumented except in the VMware KB article mentioned in
the comment around this line (http://kb.vmware.com/kb/2064959). A few years
ago, when users complained that QEMU doesn't support version 3 images, we
presumed from the article that reading is okay, as the new feature is
"persistent changed block tracking" (although it didn't say it is the only
feature enabled by version 3), and went ahead enabling it.

This time, it seems newer VMware products only accept version 3 if the
subformat is streamOptimized.  Again, without any documentation/specification
update. Then our users complains again, so we add another exception to
mitigate. As this subformat doesn't allow overwrite, the only use case is
qemu-img converting to it.  So this is pretty safe - it's always operating a
new image - and the approach is tested by multiple users (both upstream and
downstream).

Fam

Re: [Qemu-devel] [PATCH] vmdk: Fix converting to streamOptimized

[Kevin Wolf]

Am 26.01.2016 um 04:16 hat Fam Zheng geschrieben:
> On Mon, 01/25 12:16, Kevin Wolf wrote:
> > Am 25.01.2016 um 03:26 hat Fam Zheng geschrieben:
> > > Commit d62d9dc4b8 lifted streamOptimized images's version to 3, but we
> > > now refuse to open version 3 images read-write.  We need to make
> > > streamOptimized an exception to allow converting to it. This fixes the
> > > accidentally broken iotests case 059 for the same reason.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > How different are version 3 images for other subformats? Are we
> > arbitrarily restrictring their use or is it really that they don't work
> > with our driver? And if they don't work with our driver, are we sure
> > that streamOptimized images can't use the features we don't support?
> > 
> > Or is the version defined per subformat and doesn't necessarily exist
> > for other types?
> 
> Version 3 images are undocumented except in the VMware KB article mentioned in
> the comment around this line (http://kb.vmware.com/kb/2064959). A few years
> ago, when users complained that QEMU doesn't support version 3 images, we
> presumed from the article that reading is okay, as the new feature is
> "persistent changed block tracking" (although it didn't say it is the only
> feature enabled by version 3), and went ahead enabling it.
> 
> This time, it seems newer VMware products only accept version 3 if the
> subformat is streamOptimized.  Again, without any documentation/specification
> update. Then our users complains again, so we add another exception to
> mitigate. As this subformat doesn't allow overwrite, the only use case is
> qemu-img converting to it.  So this is pretty safe - it's always operating a
> new image - and the approach is tested by multiple users (both upstream and
> downstream).

I see. Then I guess we can't do much else.

Thanks, applied to the block branch.

Kevin