[Qemu-devel] [PATCH 0/7] vmdk: Misc fixes

[Qemu-devel] [PATCH 0/7] vmdk: Misc fixes

[Max Reitz]

I made the mistake of trying to run the iotests with all non-default
subformats our vmdk driver has to offer:
- monolithicFlat
- twoGbMaxExtentSparse
- twoGbMaxExtentFlat
- streamOptimized

Many things broke, so this series fixes what I found.  It’s mostly just
iotest fixes, but there are actually two real fixes in here.


Max Reitz (7):
  iotests: Fix _filter_img_create()
  vmdk: Use bdrv_dirname() for relative extent paths
  iotests: Keep testing broken relative extent paths
  vmdk: Reject invalid compressed writes
  iotests: Disable broken streamOptimized tests
  iotests: Disable 110 for vmdk.twoGbMaxExtentSparse
  iotests: Disable 126 for some vmdk subformats

 block/vmdk.c                     | 64 ++++++++++++++++++++++----------
 tests/qemu-iotests/002           |  1 +
 tests/qemu-iotests/003           |  1 +
 tests/qemu-iotests/005           |  3 +-
 tests/qemu-iotests/009           |  1 +
 tests/qemu-iotests/010           |  1 +
 tests/qemu-iotests/011           |  1 +
 tests/qemu-iotests/017           |  3 +-
 tests/qemu-iotests/018           |  3 +-
 tests/qemu-iotests/019           |  3 +-
 tests/qemu-iotests/020           |  3 +-
 tests/qemu-iotests/027           |  1 +
 tests/qemu-iotests/032           |  1 +
 tests/qemu-iotests/033           |  1 +
 tests/qemu-iotests/034           |  3 +-
 tests/qemu-iotests/037           |  3 +-
 tests/qemu-iotests/059           | 34 ++++++++++++++++-
 tests/qemu-iotests/059.out       | 24 +++++++-----
 tests/qemu-iotests/063           |  3 +-
 tests/qemu-iotests/072           |  1 +
 tests/qemu-iotests/105           |  3 +-
 tests/qemu-iotests/110           |  3 +-
 tests/qemu-iotests/126           |  6 +++
 tests/qemu-iotests/197           |  1 +
 tests/qemu-iotests/215           |  1 +
 tests/qemu-iotests/251           |  1 +
 tests/qemu-iotests/common.filter |  4 +-
 27 files changed, 131 insertions(+), 43 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/7] iotests: Fix _filter_img_create()

[Max Reitz]

fe646693acc changed qemu-img create's output so that it no longer prints
single quotes around parameter values.  The subformat and adapter_type
filters in _filter_img_create() have never been adapted to that change.

Fixes: fe646693acc13ac48b98435d14149ab04dc597bc
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/059.out       | 16 ++++++++--------
 tests/qemu-iotests/common.filter |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4fab42a28c..77d8984428 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -13,17 +13,17 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 qemu-io: can't open device TEST_DIR/t.vmdk: L1 size too big
 
 === Testing monolithicFlat creation and opening ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648 subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
 virtual size: 2 GiB (2147483648 bytes)
 
 === Testing monolithicFlat with zeroed_grain ===
 qemu-img: TEST_DIR/t.IMGFMT: Flat image can't enable zeroed grain
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648 subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
 
 === Testing big twoGbMaxExtentFlat ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824000 subformat=twoGbMaxExtentFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824000
 image: TEST_DIR/t.vmdk
 file format: vmdk
 virtual size: 0.977 TiB (1073741824000 bytes)
@@ -2038,7 +2038,7 @@ Format specific information:
 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912 VMFS "dummy.IMGFMT" 1
 
 === Testing truncated sparse ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 subformat=monolithicSparse
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': File truncated, expecting at least 13172736 bytes
 
 === Converting to streamOptimized from image with small cluster size===
@@ -2049,7 +2049,7 @@ wrote 512/512 bytes at offset 10240
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing monolithicFlat with internally generated JSON file name ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 qemu-io: can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
 
 === Testing version 3 ===
@@ -2259,7 +2259,7 @@ read 512/512 bytes at offset 64931328
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing 4TB monolithicFlat creation and IO ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4398046511104 subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4398046511104
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
 virtual size: 4 TiB (4398046511104 bytes)
@@ -2333,7 +2333,7 @@ read 1024/1024 bytes at offset 966367641600
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing qemu-img map on extents ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544 subformat=monolithicSparse
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544
 wrote 1024/1024 bytes at offset 65024
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1024/1024 bytes at offset 2147483136
@@ -2344,7 +2344,7 @@ Offset          Length          Mapped to       File
 0               0x20000         0x3f0000        TEST_DIR/t.vmdk
 0x7fff0000      0x20000         0x410000        TEST_DIR/t.vmdk
 0x140000000     0x10000         0x430000        TEST_DIR/t.vmdk
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544 subformat=twoGbMaxExtentSparse
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544
 wrote 1024/1024 bytes at offset 65024
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1024/1024 bytes at offset 2147483136
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 35fddc746f..64dd9200f3 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -130,8 +130,8 @@ _filter_img_create()
         -e "s# compat6=\\(on\\|off\\)##g" \
         -e "s# static=\\(on\\|off\\)##g" \
         -e "s# zeroed_grain=\\(on\\|off\\)##g" \
-        -e "s# subformat='[^']*'##g" \
-        -e "s# adapter_type='[^']*'##g" \
+        -e "s# subformat=[^ ]*##g" \
+        -e "s# adapter_type=[^ ]*##g" \
         -e "s# hwversion=[^ ]*##g" \
         -e "s# lazy_refcounts=\\(on\\|off\\)##g" \
         -e "s# block_size=[0-9]\\+##g" \
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] iotests: Fix _filter_img_create()

[John Snow]

On 7/25/19 11:57 AM, Max Reitz wrote:
> fe646693acc changed qemu-img create's output so that it no longer prints
> single quotes around parameter values.  The subformat and adapter_type
> filters in _filter_img_create() have never been adapted to that change.
> 
> Fixes: fe646693acc13ac48b98435d14149ab04dc597bc
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/059.out       | 16 ++++++++--------
>  tests/qemu-iotests/common.filter |  4 ++--
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 

either way, the test passes. and it was apparently our intent to filter
out the subformat, so I suppose this is correct.

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 2/7] vmdk: Use bdrv_dirname() for relative extent paths

[Max Reitz]

This makes iotest 033 pass with e.g. subformat=monolithicFlat.  It also
turns a former error in 059 into success.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vmdk.c               | 54 ++++++++++++++++++++++++--------------
 tests/qemu-iotests/059     |  7 +++--
 tests/qemu-iotests/059.out |  4 ++-
 3 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index bd36ece125..db6acfc31e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1076,8 +1076,7 @@ static const char *next_line(const char *s)
 }
 
 static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
-                              const char *desc_file_path, QDict *options,
-                              Error **errp)
+                              QDict *options, Error **errp)
 {
     int ret;
     int matches;
@@ -1087,6 +1086,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     const char *p, *np;
     int64_t sectors = 0;
     int64_t flat_offset;
+    char *desc_file_dir = NULL;
     char *extent_path;
     BdrvChild *extent_file;
     BDRVVmdkState *s = bs->opaque;
@@ -1130,16 +1130,23 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             continue;
         }
 
-        if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
-            !desc_file_path[0])
-        {
-            bdrv_refresh_filename(bs->file->bs);
-            error_setg(errp, "Cannot use relative extent paths with VMDK "
-                       "descriptor file '%s'", bs->file->bs->filename);
-            return -EINVAL;
-        }
+        if (path_is_absolute(fname) || path_has_protocol(fname)) {
+            extent_path = g_strdup(fname);
+        } else {
+            if (!desc_file_dir) {
+                desc_file_dir = bdrv_dirname(bs->file->bs, errp);
+                if (!desc_file_dir) {
+                    bdrv_refresh_filename(bs->file->bs);
+                    error_prepend(errp, "Cannot use relative paths with VMDK "
+                                  "descriptor file '%s': ",
+                                  bs->file->bs->filename);
+                    ret = -EINVAL;
+                    goto out;
+                }
+            }
 
-        extent_path = path_combine(desc_file_path, fname);
+            extent_path = g_strconcat(desc_file_dir, fname, NULL);
+        }
 
         ret = snprintf(extent_opt_prefix, 32, "extents.%d", s->num_extents);
         assert(ret < 32);
@@ -1149,7 +1156,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         g_free(extent_path);
         if (local_err) {
             error_propagate(errp, local_err);
-            return -EINVAL;
+            ret = -EINVAL;
+            goto out;
         }
 
         /* save to extents array */
@@ -1160,7 +1168,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                             0, 0, 0, 0, 0, &extent, errp);
             if (ret < 0) {
                 bdrv_unref_child(bs, extent_file);
-                return ret;
+                goto out;
             }
             extent->flat_start_offset = flat_offset << 9;
         } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
@@ -1175,24 +1183,27 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             g_free(buf);
             if (ret) {
                 bdrv_unref_child(bs, extent_file);
-                return ret;
+                goto out;
             }
             extent = &s->extents[s->num_extents - 1];
         } else if (!strcmp(type, "SESPARSE")) {
             ret = vmdk_open_se_sparse(bs, extent_file, bs->open_flags, errp);
             if (ret) {
                 bdrv_unref_child(bs, extent_file);
-                return ret;
+                goto out;
             }
             extent = &s->extents[s->num_extents - 1];
         } else {
             error_setg(errp, "Unsupported extent type '%s'", type);
             bdrv_unref_child(bs, extent_file);
-            return -ENOTSUP;
+            ret = -ENOTSUP;
+            goto out;
         }
         extent->type = g_strdup(type);
     }
-    return 0;
+
+    ret = 0;
+    goto out;
 
 invalid:
     np = next_line(p);
@@ -1201,7 +1212,11 @@ invalid:
         np--;
     }
     error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
-    return -EINVAL;
+    ret = -EINVAL;
+
+out:
+    g_free(desc_file_dir);
+    return ret;
 }
 
 static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
@@ -1228,8 +1243,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
     }
     s->create_type = g_strdup(ct);
     s->desc_offset = 0;
-    ret = vmdk_parse_extents(buf, bs, bs->file->bs->exact_filename, options,
-                             errp);
+    ret = vmdk_parse_extents(buf, bs, options, errp);
 exit:
     return ret;
 }
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 279aee6815..fbed5f9483 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -114,9 +114,12 @@ $QEMU_IMG convert -f qcow2 -O vmdk -o subformat=streamOptimized "$TEST_IMG.qcow2
 
 echo
 echo "=== Testing monolithicFlat with internally generated JSON file name ==="
+# Should work, because bdrv_dirname() works fine with blkdebug
 IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
-$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" 2>&1 \
-    | _filter_testdir | _filter_imgfmt
+$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" \
+         -c info \
+    2>&1 \
+    | _filter_testdir | _filter_imgfmt | _filter_img_info
 _cleanup_test_img
 
 echo
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 77d8984428..120cddd207 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2050,7 +2050,9 @@ wrote 512/512 bytes at offset 10240
 
 === Testing monolithicFlat with internally generated JSON file name ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
+format name: IMGFMT
+cluster size: 0 bytes
+vm state offset: 0 bytes
 
 === Testing version 3 ===
 image: TEST_DIR/iotest-version3.IMGFMT
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] vmdk: Use bdrv_dirname() for relative extent paths

[John Snow]

On 7/25/19 11:57 AM, Max Reitz wrote:
> This makes iotest 033 pass with e.g. subformat=monolithicFlat.  It also
> turns a former error in 059 into success.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Seems roughly correct, but I only really gave it a cursory look; my
trust in you knowing the exact semantics of filename and path variables
because of those lengthy series is doing the heavy lifting here:

Reviewed-by: John Snow <jsnow@redhat.com>

(And if it breaks, it's for 4.2, and it's just vmdk, we'll figure it out.)

> ---
>  block/vmdk.c               | 54 ++++++++++++++++++++++++--------------
>  tests/qemu-iotests/059     |  7 +++--
>  tests/qemu-iotests/059.out |  4 ++-
>  3 files changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index bd36ece125..db6acfc31e 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1076,8 +1076,7 @@ static const char *next_line(const char *s)
>  }
>  
>  static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> -                              const char *desc_file_path, QDict *options,
> -                              Error **errp)
> +                              QDict *options, Error **errp)
>  {
>      int ret;
>      int matches;
> @@ -1087,6 +1086,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>      const char *p, *np;
>      int64_t sectors = 0;
>      int64_t flat_offset;
> +    char *desc_file_dir = NULL;
>      char *extent_path;
>      BdrvChild *extent_file;
>      BDRVVmdkState *s = bs->opaque;
> @@ -1130,16 +1130,23 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>              continue;
>          }
>  
> -        if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
> -            !desc_file_path[0])
> -        {
> -            bdrv_refresh_filename(bs->file->bs);
> -            error_setg(errp, "Cannot use relative extent paths with VMDK "
> -                       "descriptor file '%s'", bs->file->bs->filename);
> -            return -EINVAL;
> -        }
> +        if (path_is_absolute(fname) || path_has_protocol(fname)) {
> +            extent_path = g_strdup(fname);
> +        } else {
> +            if (!desc_file_dir) {
> +                desc_file_dir = bdrv_dirname(bs->file->bs, errp);
> +                if (!desc_file_dir) {
> +                    bdrv_refresh_filename(bs->file->bs);
> +                    error_prepend(errp, "Cannot use relative paths with VMDK "
> +                                  "descriptor file '%s': ",
> +                                  bs->file->bs->filename);
> +                    ret = -EINVAL;
> +                    goto out;
> +                }
> +            }
>  
> -        extent_path = path_combine(desc_file_path, fname);
> +            extent_path = g_strconcat(desc_file_dir, fname, NULL);
> +        }
>  
>          ret = snprintf(extent_opt_prefix, 32, "extents.%d", s->num_extents);
>          assert(ret < 32);
> @@ -1149,7 +1156,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>          g_free(extent_path);
>          if (local_err) {
>              error_propagate(errp, local_err);
> -            return -EINVAL;
> +            ret = -EINVAL;
> +            goto out;
>          }
>  
>          /* save to extents array */
> @@ -1160,7 +1168,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>                              0, 0, 0, 0, 0, &extent, errp);
>              if (ret < 0) {
>                  bdrv_unref_child(bs, extent_file);
> -                return ret;
> +                goto out;
>              }
>              extent->flat_start_offset = flat_offset << 9;
>          } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
> @@ -1175,24 +1183,27 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>              g_free(buf);
>              if (ret) {
>                  bdrv_unref_child(bs, extent_file);
> -                return ret;
> +                goto out;
>              }
>              extent = &s->extents[s->num_extents - 1];
>          } else if (!strcmp(type, "SESPARSE")) {
>              ret = vmdk_open_se_sparse(bs, extent_file, bs->open_flags, errp);
>              if (ret) {
>                  bdrv_unref_child(bs, extent_file);
> -                return ret;
> +                goto out;
>              }
>              extent = &s->extents[s->num_extents - 1];
>          } else {
>              error_setg(errp, "Unsupported extent type '%s'", type);
>              bdrv_unref_child(bs, extent_file);
> -            return -ENOTSUP;
> +            ret = -ENOTSUP;
> +            goto out;
>          }
>          extent->type = g_strdup(type);
>      }
> -    return 0;
> +
> +    ret = 0;
> +    goto out;
>  
>  invalid:
>      np = next_line(p);
> @@ -1201,7 +1212,11 @@ invalid:
>          np--;
>      }
>      error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
> -    return -EINVAL;
> +    ret = -EINVAL;
> +
> +out:
> +    g_free(desc_file_dir);
> +    return ret;
>  }
>  
>  static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
> @@ -1228,8 +1243,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
>      }
>      s->create_type = g_strdup(ct);
>      s->desc_offset = 0;
> -    ret = vmdk_parse_extents(buf, bs, bs->file->bs->exact_filename, options,
> -                             errp);
> +    ret = vmdk_parse_extents(buf, bs, options, errp);
>  exit:
>      return ret;
>  }
> diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
> index 279aee6815..fbed5f9483 100755
> --- a/tests/qemu-iotests/059
> +++ b/tests/qemu-iotests/059
> @@ -114,9 +114,12 @@ $QEMU_IMG convert -f qcow2 -O vmdk -o subformat=streamOptimized "$TEST_IMG.qcow2
>  
>  echo
>  echo "=== Testing monolithicFlat with internally generated JSON file name ==="
> +# Should work, because bdrv_dirname() works fine with blkdebug
>  IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
> -$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" 2>&1 \
> -    | _filter_testdir | _filter_imgfmt
> +$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" \
> +         -c info \
> +    2>&1 \
> +    | _filter_testdir | _filter_imgfmt | _filter_img_info
>  _cleanup_test_img
>  
>  echo
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index 77d8984428..120cddd207 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -2050,7 +2050,9 @@ wrote 512/512 bytes at offset 10240
>  
>  === Testing monolithicFlat with internally generated JSON file name ===
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> -qemu-io: can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
> +format name: IMGFMT
> +cluster size: 0 bytes
> +vm state offset: 0 bytes
>  
>  === Testing version 3 ===
>  image: TEST_DIR/iotest-version3.IMGFMT
> 

[Qemu-devel] [PATCH 3/7] iotests: Keep testing broken relative extent paths

[Max Reitz]

We had a test for a case where relative extent paths did not work, but
unfortunately we just fixed the underlying problem, so it works now.
This patch adds a new test case that still fails.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/059     | 27 +++++++++++++++++++++++++++
 tests/qemu-iotests/059.out |  4 ++++
 2 files changed, 31 insertions(+)

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index fbed5f9483..2a883d0f21 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -114,6 +114,8 @@ $QEMU_IMG convert -f qcow2 -O vmdk -o subformat=streamOptimized "$TEST_IMG.qcow2
 
 echo
 echo "=== Testing monolithicFlat with internally generated JSON file name ==="
+
+echo '--- blkdebug ---'
 # Should work, because bdrv_dirname() works fine with blkdebug
 IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
 $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" \
@@ -122,6 +124,31 @@ $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TE
     | _filter_testdir | _filter_imgfmt | _filter_img_info
 _cleanup_test_img
 
+echo '--- quorum ---'
+# Should not work, because bdrv_dirname() does not work with blkdebug
+IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
+cp "$TEST_IMG" "$TEST_IMG.orig"
+
+filename="json:{
+    \"driver\": \"$IMGFMT\",
+    \"file\": {
+        \"driver\": \"quorum\",
+        \"children\": [ {
+            \"driver\": \"file\",
+            \"filename\": \"$TEST_IMG\"
+        }, {
+            \"driver\": \"file\",
+            \"filename\": \"$TEST_IMG.orig\"
+        } ],
+        \"vote-threshold\": 1
+    } }"
+
+filename=$(echo "$filename" | tr '\n' ' ' | sed -e 's/\s\+/ /g')
+$QEMU_IMG info "$filename" 2>&1 \
+    | sed -e "s/'json:[^']*'/\$QUORUM_FILE/g" \
+    | _filter_testdir | _filter_imgfmt | _filter_img_info
+
+
 echo
 echo "=== Testing version 3 ==="
 _use_sample_img iotest-version3.vmdk.bz2
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 120cddd207..f8895ba434 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2049,10 +2049,14 @@ wrote 512/512 bytes at offset 10240
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing monolithicFlat with internally generated JSON file name ===
+--- blkdebug ---
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 format name: IMGFMT
 cluster size: 0 bytes
 vm state offset: 0 bytes
+--- quorum ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qemu-img: Could not open $QUORUM_FILE: Cannot use relative paths with VMDK descriptor file $QUORUM_FILE: Cannot generate a base directory for quorum nodes
 
 === Testing version 3 ===
 image: TEST_DIR/iotest-version3.IMGFMT
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] iotests: Keep testing broken relative extent paths

[John Snow]

On 7/25/19 11:57 AM, Max Reitz wrote:
> We had a test for a case where relative extent paths did not work, but
> unfortunately we just fixed the underlying problem, so it works now.
> This patch adds a new test case that still fails.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/059     | 27 +++++++++++++++++++++++++++
>  tests/qemu-iotests/059.out |  4 ++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
> index fbed5f9483..2a883d0f21 100755
> --- a/tests/qemu-iotests/059
> +++ b/tests/qemu-iotests/059
> @@ -114,6 +114,8 @@ $QEMU_IMG convert -f qcow2 -O vmdk -o subformat=streamOptimized "$TEST_IMG.qcow2
>  
>  echo
>  echo "=== Testing monolithicFlat with internally generated JSON file name ==="
> +
> +echo '--- blkdebug ---'
>  # Should work, because bdrv_dirname() works fine with blkdebug

^

>  IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
>  $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" \
> @@ -122,6 +124,31 @@ $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TE
>      | _filter_testdir | _filter_imgfmt | _filter_img_info
>  _cleanup_test_img
>  
> +echo '--- quorum ---'
> +# Should not work, because bdrv_dirname() does not work with blkdebug

^ ? So uh, which is it?

(you wanted: s/blkdebug/quorum/)

> +IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
> +cp "$TEST_IMG" "$TEST_IMG.orig"
> +
> +filename="json:{
> +    \"driver\": \"$IMGFMT\",
> +    \"file\": {
> +        \"driver\": \"quorum\",
> +        \"children\": [ {
> +            \"driver\": \"file\",
> +            \"filename\": \"$TEST_IMG\"
> +        }, {
> +            \"driver\": \"file\",
> +            \"filename\": \"$TEST_IMG.orig\"
> +        } ],
> +        \"vote-threshold\": 1
> +    } }"
> +
> +filename=$(echo "$filename" | tr '\n' ' ' | sed -e 's/\s\+/ /g')
> +$QEMU_IMG info "$filename" 2>&1 \
> +    | sed -e "s/'json:[^']*'/\$QUORUM_FILE/g" \
> +    | _filter_testdir | _filter_imgfmt | _filter_img_info
> +
> +
>  echo
>  echo "=== Testing version 3 ==="
>  _use_sample_img iotest-version3.vmdk.bz2
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index 120cddd207..f8895ba434 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -2049,10 +2049,14 @@ wrote 512/512 bytes at offset 10240
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  
>  === Testing monolithicFlat with internally generated JSON file name ===
> +--- blkdebug ---
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  format name: IMGFMT
>  cluster size: 0 bytes
>  vm state offset: 0 bytes
> +--- quorum ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +qemu-img: Could not open $QUORUM_FILE: Cannot use relative paths with VMDK descriptor file $QUORUM_FILE: Cannot generate a base directory for quorum nodes
>  
>  === Testing version 3 ===
>  image: TEST_DIR/iotest-version3.IMGFMT
> 

With the paste-o fixed:

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 4/7] vmdk: Reject invalid compressed writes

[Max Reitz]

Compressed writes generally have to write full clusters, not just in
theory but also in practice when it comes to vmdk's streamOptimized
subformat.  It currently is just silently broken for writes with
non-zero in-cluster offsets:

$ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
$ qemu-io -c 'write 4k 4k' -c 'read 4k 4k' foo.vmdk
wrote 4096/4096 bytes at offset 4096
4 KiB, 1 ops; 00.01 sec (443.724 KiB/sec and 110.9309 ops/sec)
read failed: Invalid argument

(The technical reason is that vmdk_write_extent() just writes the
incomplete compressed data actually to offset 4k.  When reading the
data, vmdk_read_extent() looks at offset 0 and finds the compressed data
size to be 0, because that is what it reads from there.  This yields an
error.)

For incomplete writes with zero in-cluster offsets, the error path when
reading the rest of the cluster is a bit different, but the result is
the same:

$ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
$ qemu-io -c 'write 0k 4k' -c 'read 4k 4k' foo.vmdk
wrote 4096/4096 bytes at offset 0
4 KiB, 1 ops; 00.01 sec (362.641 KiB/sec and 90.6603 ops/sec)
read failed: Invalid argument

(Here, vmdk_read_extent() finds the data and then sees that the
uncompressed data is short.)

It is better to reject invalid writes than to make the user believe they
might have succeeded and then fail when trying to read it back.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vmdk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index db6acfc31e..641acacfe0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1731,6 +1731,16 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
     if (extent->compressed) {
         void *compressed_data;
 
+        /* Only whole clusters */
+        if (offset_in_cluster ||
+            n_bytes > (extent->cluster_sectors * SECTOR_SIZE) ||
+            (n_bytes < (extent->cluster_sectors * SECTOR_SIZE) &&
+             offset + n_bytes != extent->end_sector * SECTOR_SIZE))
+        {
+            ret = -EINVAL;
+            goto out;
+        }
+
         if (!extent->has_marker) {
             ret = -EINVAL;
             goto out;
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] vmdk: Reject invalid compressed writes

[John Snow]

On 7/25/19 11:57 AM, Max Reitz wrote:
> Compressed writes generally have to write full clusters, not just in
> theory but also in practice when it comes to vmdk's streamOptimized
> subformat.  It currently is just silently broken for writes with
> non-zero in-cluster offsets:
> 
> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
> $ qemu-io -c 'write 4k 4k' -c 'read 4k 4k' foo.vmdk
> wrote 4096/4096 bytes at offset 4096
> 4 KiB, 1 ops; 00.01 sec (443.724 KiB/sec and 110.9309 ops/sec)
> read failed: Invalid argument
> 
> (The technical reason is that vmdk_write_extent() just writes the
> incomplete compressed data actually to offset 4k.  When reading the
> data, vmdk_read_extent() looks at offset 0 and finds the compressed data
> size to be 0, because that is what it reads from there.  This yields an
> error.)
> 
> For incomplete writes with zero in-cluster offsets, the error path when
> reading the rest of the cluster is a bit different, but the result is
> the same:
> 
> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
> $ qemu-io -c 'write 0k 4k' -c 'read 4k 4k' foo.vmdk
> wrote 4096/4096 bytes at offset 0
> 4 KiB, 1 ops; 00.01 sec (362.641 KiB/sec and 90.6603 ops/sec)
> read failed: Invalid argument
> 
> (Here, vmdk_read_extent() finds the data and then sees that the
> uncompressed data is short.)
> 
> It is better to reject invalid writes than to make the user believe they
> might have succeeded and then fail when trying to read it back.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index db6acfc31e..641acacfe0 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1731,6 +1731,16 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
>      if (extent->compressed) {
>          void *compressed_data;
>  
> +        /* Only whole clusters */
> +        if (offset_in_cluster ||
> +            n_bytes > (extent->cluster_sectors * SECTOR_SIZE) ||
> +            (n_bytes < (extent->cluster_sectors * SECTOR_SIZE) &&
> +             offset + n_bytes != extent->end_sector * SECTOR_SIZE))
> +        {
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
>          if (!extent->has_marker) {
>              ret = -EINVAL;
>              goto out;
> 

What does this look like from a guest's perspective? Is there something
that enforces the alignment in the graph for us?

Or is it the case that indeed guests (or users via qemu-io) can request
invalid writes and we will halt the VM in those cases (in preference to
corrupting the disk)?

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] vmdk: Reject invalid compressed writes

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4128 bytes --]

On 12.08.19 22:26, John Snow wrote:
> 
> 
> On 7/25/19 11:57 AM, Max Reitz wrote:
>> Compressed writes generally have to write full clusters, not just in
>> theory but also in practice when it comes to vmdk's streamOptimized
>> subformat.  It currently is just silently broken for writes with
>> non-zero in-cluster offsets:
>>
>> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
>> $ qemu-io -c 'write 4k 4k' -c 'read 4k 4k' foo.vmdk
>> wrote 4096/4096 bytes at offset 4096
>> 4 KiB, 1 ops; 00.01 sec (443.724 KiB/sec and 110.9309 ops/sec)
>> read failed: Invalid argument
>>
>> (The technical reason is that vmdk_write_extent() just writes the
>> incomplete compressed data actually to offset 4k.  When reading the
>> data, vmdk_read_extent() looks at offset 0 and finds the compressed data
>> size to be 0, because that is what it reads from there.  This yields an
>> error.)
>>
>> For incomplete writes with zero in-cluster offsets, the error path when
>> reading the rest of the cluster is a bit different, but the result is
>> the same:
>>
>> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
>> $ qemu-io -c 'write 0k 4k' -c 'read 4k 4k' foo.vmdk
>> wrote 4096/4096 bytes at offset 0
>> 4 KiB, 1 ops; 00.01 sec (362.641 KiB/sec and 90.6603 ops/sec)
>> read failed: Invalid argument
>>
>> (Here, vmdk_read_extent() finds the data and then sees that the
>> uncompressed data is short.)
>>
>> It is better to reject invalid writes than to make the user believe they
>> might have succeeded and then fail when trying to read it back.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/vmdk.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index db6acfc31e..641acacfe0 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1731,6 +1731,16 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
>>      if (extent->compressed) {
>>          void *compressed_data;
>>  
>> +        /* Only whole clusters */
>> +        if (offset_in_cluster ||
>> +            n_bytes > (extent->cluster_sectors * SECTOR_SIZE) ||
>> +            (n_bytes < (extent->cluster_sectors * SECTOR_SIZE) &&
>> +             offset + n_bytes != extent->end_sector * SECTOR_SIZE))
>> +        {
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +
>>          if (!extent->has_marker) {
>>              ret = -EINVAL;
>>              goto out;
>>
> 
> What does this look like from a guest's perspective? Is there something
> that enforces the alignment in the graph for us?
> 
> Or is it the case that indeed guests (or users via qemu-io) can request
> invalid writes and we will halt the VM in those cases (in preference to
> corrupting the disk)?

Have you ever tried using a streamOptimized VMDK disk with a guest?

I haven’t, but I know that it won’t work. O:-)  If you try to write to
an already allocated cluster, you’ll get an EIO and an error message via
error_report() (“Could not write to allocated cluster for
streamOptimized”).  So really, the only use of streamOptimized is as a
qemu-img convert source/target, or as a backup/mirror target.  (Just
like compressed clusters in qcow2 images.)

I suppose if I introduced streamOptimized support today, I wouldn’t just
forward vmdk_co_pwritev_compressed() to vmdk_co_pwritev(), but instead
make vmdk_co_pwritev_compressed() only work on streamOptimized images,
and vmdk_co_pwritev() only on everything else.  Then it would be more clear.

Hm.  In fact, that’s a bug, isn’t it?  vmdk will accept compressed
writes for any subformat, even if it doesn’t support compression.  So if
you use -c and convert to vmdk, it will succeed, but the result won’t be
compressed,

It’s also a bit weird to accept normal writes for streamOptimized, but
I’m not sure whether that’s really a bug?  In any case, changing this
behavior would not be backwards-compatible...  Should we deprecate
normal writes to streamOptimized?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] vmdk: Reject invalid compressed writes

[John Snow]

On 8/12/19 5:03 PM, Max Reitz wrote:
> On 12.08.19 22:26, John Snow wrote:
>>
>>
>> On 7/25/19 11:57 AM, Max Reitz wrote:
>>> Compressed writes generally have to write full clusters, not just in
>>> theory but also in practice when it comes to vmdk's streamOptimized
>>> subformat.  It currently is just silently broken for writes with
>>> non-zero in-cluster offsets:
>>>
>>> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
>>> $ qemu-io -c 'write 4k 4k' -c 'read 4k 4k' foo.vmdk
>>> wrote 4096/4096 bytes at offset 4096
>>> 4 KiB, 1 ops; 00.01 sec (443.724 KiB/sec and 110.9309 ops/sec)
>>> read failed: Invalid argument
>>>
>>> (The technical reason is that vmdk_write_extent() just writes the
>>> incomplete compressed data actually to offset 4k.  When reading the
>>> data, vmdk_read_extent() looks at offset 0 and finds the compressed data
>>> size to be 0, because that is what it reads from there.  This yields an
>>> error.)
>>>
>>> For incomplete writes with zero in-cluster offsets, the error path when
>>> reading the rest of the cluster is a bit different, but the result is
>>> the same:
>>>
>>> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
>>> $ qemu-io -c 'write 0k 4k' -c 'read 4k 4k' foo.vmdk
>>> wrote 4096/4096 bytes at offset 0
>>> 4 KiB, 1 ops; 00.01 sec (362.641 KiB/sec and 90.6603 ops/sec)
>>> read failed: Invalid argument
>>>
>>> (Here, vmdk_read_extent() finds the data and then sees that the
>>> uncompressed data is short.)
>>>
>>> It is better to reject invalid writes than to make the user believe they
>>> might have succeeded and then fail when trying to read it back.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  block/vmdk.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/block/vmdk.c b/block/vmdk.c
>>> index db6acfc31e..641acacfe0 100644
>>> --- a/block/vmdk.c
>>> +++ b/block/vmdk.c
>>> @@ -1731,6 +1731,16 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
>>>      if (extent->compressed) {
>>>          void *compressed_data;
>>>  
>>> +        /* Only whole clusters */
>>> +        if (offset_in_cluster ||
>>> +            n_bytes > (extent->cluster_sectors * SECTOR_SIZE) ||
>>> +            (n_bytes < (extent->cluster_sectors * SECTOR_SIZE) &&
>>> +             offset + n_bytes != extent->end_sector * SECTOR_SIZE))
>>> +        {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>>          if (!extent->has_marker) {
>>>              ret = -EINVAL;
>>>              goto out;
>>>
>>
>> What does this look like from a guest's perspective? Is there something
>> that enforces the alignment in the graph for us?
>>
>> Or is it the case that indeed guests (or users via qemu-io) can request
>> invalid writes and we will halt the VM in those cases (in preference to
>> corrupting the disk)?
> 
> Have you ever tried using a streamOptimized VMDK disk with a guest?
> 

Nope! It's why I'm asking. I have no idea what the whole picture before
and after is.

> I haven’t, but I know that it won’t work. O:-)  If you try to write to
> an already allocated cluster, you’ll get an EIO and an error message via
> error_report() (“Could not write to allocated cluster for
> streamOptimized”).  So really, the only use of streamOptimized is as a
> qemu-img convert source/target, or as a backup/mirror target.  (Just
> like compressed clusters in qcow2 images.)
> 

OK, makes sense. Someone's going to try to use it in cases where it
doesn't make sense though, for sure.

> I suppose if I introduced streamOptimized support today, I wouldn’t just
> forward vmdk_co_pwritev_compressed() to vmdk_co_pwritev(), but instead
> make vmdk_co_pwritev_compressed() only work on streamOptimized images,
> and vmdk_co_pwritev() only on everything else.  Then it would be more clear.
> 
> Hm.  In fact, that’s a bug, isn’t it?  vmdk will accept compressed
> writes for any subformat, even if it doesn’t support compression.  So if
> you use -c and convert to vmdk, it will succeed, but the result won’t be
> compressed,
> 
> It’s also a bit weird to accept normal writes for streamOptimized, but
> I’m not sure whether that’s really a bug?  In any case, changing this
> behavior would not be backwards-compatible...  Should we deprecate
> normal writes to streamOptimized?
> 

If it's supposed to be the case that streamOptimized *only* gets
compressed, aligned writes -- it probably is a bug to do normal,
uncompressed writes, isn't it? Does that produce a usable image?

> Max
> 

Anyway, I'm fine with this patch because things aren't any worse, and
our support for non-native formats has always been kind of best-attempt.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] vmdk: Reject invalid compressed writes

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5304 bytes --]

On 12.08.19 23:16, John Snow wrote:
> 
> 
> On 8/12/19 5:03 PM, Max Reitz wrote:
>> On 12.08.19 22:26, John Snow wrote:
>>>
>>>
>>> On 7/25/19 11:57 AM, Max Reitz wrote:
>>>> Compressed writes generally have to write full clusters, not just in
>>>> theory but also in practice when it comes to vmdk's streamOptimized
>>>> subformat.  It currently is just silently broken for writes with
>>>> non-zero in-cluster offsets:
>>>>
>>>> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
>>>> $ qemu-io -c 'write 4k 4k' -c 'read 4k 4k' foo.vmdk
>>>> wrote 4096/4096 bytes at offset 4096
>>>> 4 KiB, 1 ops; 00.01 sec (443.724 KiB/sec and 110.9309 ops/sec)
>>>> read failed: Invalid argument
>>>>
>>>> (The technical reason is that vmdk_write_extent() just writes the
>>>> incomplete compressed data actually to offset 4k.  When reading the
>>>> data, vmdk_read_extent() looks at offset 0 and finds the compressed data
>>>> size to be 0, because that is what it reads from there.  This yields an
>>>> error.)
>>>>
>>>> For incomplete writes with zero in-cluster offsets, the error path when
>>>> reading the rest of the cluster is a bit different, but the result is
>>>> the same:
>>>>
>>>> $ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
>>>> $ qemu-io -c 'write 0k 4k' -c 'read 4k 4k' foo.vmdk
>>>> wrote 4096/4096 bytes at offset 0
>>>> 4 KiB, 1 ops; 00.01 sec (362.641 KiB/sec and 90.6603 ops/sec)
>>>> read failed: Invalid argument
>>>>
>>>> (Here, vmdk_read_extent() finds the data and then sees that the
>>>> uncompressed data is short.)
>>>>
>>>> It is better to reject invalid writes than to make the user believe they
>>>> might have succeeded and then fail when trying to read it back.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  block/vmdk.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/block/vmdk.c b/block/vmdk.c
>>>> index db6acfc31e..641acacfe0 100644
>>>> --- a/block/vmdk.c
>>>> +++ b/block/vmdk.c
>>>> @@ -1731,6 +1731,16 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
>>>>      if (extent->compressed) {
>>>>          void *compressed_data;
>>>>  
>>>> +        /* Only whole clusters */
>>>> +        if (offset_in_cluster ||
>>>> +            n_bytes > (extent->cluster_sectors * SECTOR_SIZE) ||
>>>> +            (n_bytes < (extent->cluster_sectors * SECTOR_SIZE) &&
>>>> +             offset + n_bytes != extent->end_sector * SECTOR_SIZE))
>>>> +        {
>>>> +            ret = -EINVAL;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>>          if (!extent->has_marker) {
>>>>              ret = -EINVAL;
>>>>              goto out;
>>>>
>>>
>>> What does this look like from a guest's perspective? Is there something
>>> that enforces the alignment in the graph for us?
>>>
>>> Or is it the case that indeed guests (or users via qemu-io) can request
>>> invalid writes and we will halt the VM in those cases (in preference to
>>> corrupting the disk)?
>>
>> Have you ever tried using a streamOptimized VMDK disk with a guest?
>>
> 
> Nope! It's why I'm asking. I have no idea what the whole picture before
> and after is.
> 
>> I haven’t, but I know that it won’t work. O:-)  If you try to write to
>> an already allocated cluster, you’ll get an EIO and an error message via
>> error_report() (“Could not write to allocated cluster for
>> streamOptimized”).  So really, the only use of streamOptimized is as a
>> qemu-img convert source/target, or as a backup/mirror target.  (Just
>> like compressed clusters in qcow2 images.)
>>
> 
> OK, makes sense. Someone's going to try to use it in cases where it
> doesn't make sense though, for sure.
> 
>> I suppose if I introduced streamOptimized support today, I wouldn’t just
>> forward vmdk_co_pwritev_compressed() to vmdk_co_pwritev(), but instead
>> make vmdk_co_pwritev_compressed() only work on streamOptimized images,
>> and vmdk_co_pwritev() only on everything else.  Then it would be more clear.
>>
>> Hm.  In fact, that’s a bug, isn’t it?  vmdk will accept compressed
>> writes for any subformat, even if it doesn’t support compression.  So if
>> you use -c and convert to vmdk, it will succeed, but the result won’t be
>> compressed,
>>
>> It’s also a bit weird to accept normal writes for streamOptimized, but
>> I’m not sure whether that’s really a bug?  In any case, changing this
>> behavior would not be backwards-compatible...  Should we deprecate
>> normal writes to streamOptimized?
>>
> 
> If it's supposed to be the case that streamOptimized *only* gets
> compressed, aligned writes -- it probably is a bug to do normal,
> uncompressed writes, isn't it? Does that produce a usable image?

Well, all writes are silently done as compressed writes.  (With the
alignment requirements added in this patch.)  The image is useful, as
long as you only full clusters, and you may only write to each cluster
once...

>> Max
>>
> 
> Anyway, I'm fine with this patch because things aren't any worse, and
> our support for non-native formats has always been kind of best-attempt.
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 5/7] iotests: Disable broken streamOptimized tests

[Max Reitz]

streamOptimized does not support writes that do not span exactly one
cluster.  Furthermore, it cannot rewrite already allocated clusters.
As such, many iotests do not work with it.  Disable them.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/002 | 1 +
 tests/qemu-iotests/003 | 1 +
 tests/qemu-iotests/005 | 3 ++-
 tests/qemu-iotests/009 | 1 +
 tests/qemu-iotests/010 | 1 +
 tests/qemu-iotests/011 | 1 +
 tests/qemu-iotests/017 | 3 ++-
 tests/qemu-iotests/018 | 3 ++-
 tests/qemu-iotests/019 | 3 ++-
 tests/qemu-iotests/020 | 3 ++-
 tests/qemu-iotests/027 | 1 +
 tests/qemu-iotests/032 | 1 +
 tests/qemu-iotests/033 | 1 +
 tests/qemu-iotests/034 | 3 ++-
 tests/qemu-iotests/037 | 3 ++-
 tests/qemu-iotests/063 | 3 ++-
 tests/qemu-iotests/072 | 1 +
 tests/qemu-iotests/105 | 3 ++-
 tests/qemu-iotests/197 | 1 +
 tests/qemu-iotests/215 | 1 +
 tests/qemu-iotests/251 | 1 +
 21 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/002 b/tests/qemu-iotests/002
index fd413bce48..1a0d411df5 100755
--- a/tests/qemu-iotests/002
+++ b/tests/qemu-iotests/002
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt generic
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 
 size=128M
diff --git a/tests/qemu-iotests/003 b/tests/qemu-iotests/003
index ccd3a39dfb..33eeade0de 100755
--- a/tests/qemu-iotests/003
+++ b/tests/qemu-iotests/003
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt generic
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 size=128M
 offset=67M
diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005
index 9c7681c19b..58442762fe 100755
--- a/tests/qemu-iotests/005
+++ b/tests/qemu-iotests/005
@@ -43,7 +43,8 @@ _supported_fmt generic
 _supported_proto generic
 _supported_os Linux
 _unsupported_imgopts "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+                     "subformat=twoGbMaxExtentSparse" \
+                     "subformat=streamOptimized"
 
 # vpc is limited to 127GB, so we can't test it here
 if [ "$IMGFMT" = "vpc" ]; then
diff --git a/tests/qemu-iotests/009 b/tests/qemu-iotests/009
index 51b200db1d..4dc7d210f9 100755
--- a/tests/qemu-iotests/009
+++ b/tests/qemu-iotests/009
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt generic
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 
 size=6G
diff --git a/tests/qemu-iotests/010 b/tests/qemu-iotests/010
index 48c533f632..df809b3088 100755
--- a/tests/qemu-iotests/010
+++ b/tests/qemu-iotests/010
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt generic
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 
 size=6G
diff --git a/tests/qemu-iotests/011 b/tests/qemu-iotests/011
index 8b1fce069a..d02c38c461 100755
--- a/tests/qemu-iotests/011
+++ b/tests/qemu-iotests/011
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt generic
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 
 size=6G
diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
index 79875de454..0a4b854e65 100755
--- a/tests/qemu-iotests/017
+++ b/tests/qemu-iotests/017
@@ -41,7 +41,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow qcow2 vmdk qed
 _supported_proto generic
 _unsupported_proto vxhs
-_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
+_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
+                     "subformat=streamOptimized"
 
 TEST_OFFSETS="0 4294967296"
 
diff --git a/tests/qemu-iotests/018 b/tests/qemu-iotests/018
index 78169838ba..c69ce09209 100755
--- a/tests/qemu-iotests/018
+++ b/tests/qemu-iotests/018
@@ -41,7 +41,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow qcow2 vmdk qed
 _supported_proto file
 _supported_os Linux
-_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
+_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
+                     "streamOptimized"
 
 TEST_OFFSETS="0 4294967296"
 
diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019
index a56dd30bed..b4f5234609 100755
--- a/tests/qemu-iotests/019
+++ b/tests/qemu-iotests/019
@@ -47,7 +47,8 @@ _supported_proto file
 _supported_os Linux
 _unsupported_imgopts "subformat=monolithicFlat" \
                      "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+                     "subformat=twoGbMaxExtentSparse" \
+                     "subformat=streamOptimized"
 
 TEST_OFFSETS="0 4294967296"
 CLUSTER_SIZE=65536
diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index 6b0ebb37d2..f41b92f35f 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -44,7 +44,8 @@ _supported_fmt qcow qcow2 vmdk qed
 _supported_proto file
 _unsupported_imgopts "subformat=monolithicFlat" \
                      "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+                     "subformat=twoGbMaxExtentSparse" \
+                     "subformat=streamOptimized"
 
 TEST_OFFSETS="0 4294967296"
 
diff --git a/tests/qemu-iotests/027 b/tests/qemu-iotests/027
index 4cb638022a..494be0921f 100755
--- a/tests/qemu-iotests/027
+++ b/tests/qemu-iotests/027
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt vmdk qcow qcow2 qed
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 
 size=128M
diff --git a/tests/qemu-iotests/032 b/tests/qemu-iotests/032
index 23c216c549..5c998dbd7d 100755
--- a/tests/qemu-iotests/032
+++ b/tests/qemu-iotests/032
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # This works for any image format (though unlikely to segfault for raw)
 _supported_fmt generic
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 echo
 echo === Prepare image ===
diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033
index 362a48c0a0..8b40991d55 100755
--- a/tests/qemu-iotests/033
+++ b/tests/qemu-iotests/033
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt generic
 _supported_proto generic
+_unsupported_imgopts "subformat=streamOptimized"
 
 
 size=128M
diff --git a/tests/qemu-iotests/034 b/tests/qemu-iotests/034
index 324bed28c6..da4cea1571 100755
--- a/tests/qemu-iotests/034
+++ b/tests/qemu-iotests/034
@@ -41,7 +41,8 @@ _supported_proto file
 _supported_os Linux
 _unsupported_imgopts "subformat=monolithicFlat" \
                      "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+                     "subformat=twoGbMaxExtentSparse" \
+                     "subformat=streamOptimized"
 
 CLUSTER_SIZE=4k
 size=128M
diff --git a/tests/qemu-iotests/037 b/tests/qemu-iotests/037
index 819a2a52d2..b650c52b09 100755
--- a/tests/qemu-iotests/037
+++ b/tests/qemu-iotests/037
@@ -40,7 +40,8 @@ _supported_fmt qcow qcow2 vmdk qed
 _supported_proto file
 _unsupported_imgopts "subformat=monolithicFlat" \
                      "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+                     "subformat=twoGbMaxExtentSparse" \
+                     "subformat=streamOptimized"
 
 CLUSTER_SIZE=4k
 size=128M
diff --git a/tests/qemu-iotests/063 b/tests/qemu-iotests/063
index fe4892e467..7cf0427af4 100755
--- a/tests/qemu-iotests/063
+++ b/tests/qemu-iotests/063
@@ -43,7 +43,8 @@ _supported_fmt qcow qcow2 vmdk qed raw
 _supported_proto file
 _unsupported_imgopts "subformat=monolithicFlat" \
                      "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+                     "subformat=twoGbMaxExtentSparse" \
+                     "subformat=streamOptimized"
 
 _make_test_img 4M
 
diff --git a/tests/qemu-iotests/072 b/tests/qemu-iotests/072
index 661b36da2d..f0b73e7e65 100755
--- a/tests/qemu-iotests/072
+++ b/tests/qemu-iotests/072
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt vpc vmdk vhdx vdi qed qcow2 qcow
 _supported_proto file
+_unsupported_imgopts "subformat=streamOptimized"
 
 IMG_SIZE=64M
 
diff --git a/tests/qemu-iotests/105 b/tests/qemu-iotests/105
index 3346e8cb25..4d55a2d3ef 100755
--- a/tests/qemu-iotests/105
+++ b/tests/qemu-iotests/105
@@ -39,7 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2 vmdk vhdx qed
 _supported_proto generic
 _unsupported_imgopts "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+                     "subformat=twoGbMaxExtentSparse" \
+                     "subformat=streamOptimized"
 
 echo
 echo "creating large image"
diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
index 383d7d7f61..1d4f6786db 100755
--- a/tests/qemu-iotests/197
+++ b/tests/qemu-iotests/197
@@ -53,6 +53,7 @@ _supported_fmt generic
 _supported_proto generic
 # LUKS support may be possible, but it complicates things.
 _unsupported_fmt luks
+_unsupported_imgopts "subformat=streamOptimized"
 
 echo
 echo '=== Copy-on-read ==='
diff --git a/tests/qemu-iotests/215 b/tests/qemu-iotests/215
index 958c14f5a0..2eb377d682 100755
--- a/tests/qemu-iotests/215
+++ b/tests/qemu-iotests/215
@@ -50,6 +50,7 @@ _supported_fmt generic
 _supported_proto generic
 # LUKS support may be possible, but it complicates things.
 _unsupported_fmt luks
+_unsupported_imgopts "subformat=streamOptimized"
 
 echo
 echo '=== Copy-on-read ==='
diff --git a/tests/qemu-iotests/251 b/tests/qemu-iotests/251
index 13f85de9cd..7918ba3559 100755
--- a/tests/qemu-iotests/251
+++ b/tests/qemu-iotests/251
@@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt generic
 _supported_proto file
 _supported_os Linux
+_unsupported_imgopts "subformat=streamOptimized"
 
 if [ "$IMGOPTSSYNTAX" = "true" ]; then
     # We use json:{} filenames here, so we cannot work with additional options.
-- 
2.21.0

[Qemu-devel] [PATCH 6/7] iotests: Disable 110 for vmdk.twoGbMaxExtentSparse

[Max Reitz]

The error message for the test case where we have a quorum node for
which no directory name can be generated is different: For
twoGbMaxExtentSparse, it complains that it cannot open the extent file.
For other (sub)formats, it just notes that it cannot determine the
backing file path.  Both are fine, but just disable twoGbMaxExtentSparse
for simplicity's sake.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/110 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index 2cdc7c8a72..2ef516baf1 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -40,7 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # Any format supporting backing files
 _supported_fmt qed qcow qcow2 vmdk
 _supported_proto file
-_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
+_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
+                     "subformat=twoGbMaxExtentSparse"
 
 TEST_IMG_REL=$(basename "$TEST_IMG")
 
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] iotests: Disable 110 for vmdk.twoGbMaxExtentSparse

[John Snow]

On 7/25/19 11:57 AM, Max Reitz wrote:
> The error message for the test case where we have a quorum node for
> which no directory name can be generated is different: For
> twoGbMaxExtentSparse, it complains that it cannot open the extent file.
> For other (sub)formats, it just notes that it cannot determine the
> backing file path.  Both are fine, but just disable twoGbMaxExtentSparse
> for simplicity's sake.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/110 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
> index 2cdc7c8a72..2ef516baf1 100755
> --- a/tests/qemu-iotests/110
> +++ b/tests/qemu-iotests/110
> @@ -40,7 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  # Any format supporting backing files
>  _supported_fmt qed qcow qcow2 vmdk
>  _supported_proto file
> -_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
> +_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
> +                     "subformat=twoGbMaxExtentSparse"
>  
>  TEST_IMG_REL=$(basename "$TEST_IMG")
>  
> 

Hm, fine. This is the second patch I've seen today that tries to work
around error messages changing because of configuration options, though.

Wonder if we need a more general purpose solution to this kind of thing.

Ah, well, regardless... good enough for now, I think, probably.

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[Max Reitz]

Several vmdk subformats do not work with iotest 126, so disable them.

(twoGbMaxExtentSparse actually should work, but fixing that is a bit
difficult.  The problem is that the vmdk descriptor file will contain a
referenc to "image:base.vmdk", which the block layer cannot open because
it does not know the protocol "image".  This is not trivial to solve,
because I suppose real protocols like "http://" should be supported.
Making vmdk treat all paths with a potential protocol prefix that the
block layer does not recognize as plain files seems a bit weird,
though.  Ignoring this problem does not seem too bad.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/126 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
index 9b0dcf9255..8e55d7c843 100755
--- a/tests/qemu-iotests/126
+++ b/tests/qemu-iotests/126
@@ -33,6 +33,12 @@ status=1	# failure is the default!
 
 # Needs backing file support
 _supported_fmt qcow qcow2 qed vmdk
+# (1) Flat vmdk images do not support backing files
+# (2) Split vmdk images simply fail this test right now.  Fixing that
+#     is left for another day.
+_unsupported_imgopts "subformat=monolithicFlat" \
+                     "subformat=twoGbMaxExtentFlat" \
+                     "subformat=twoGbMaxExtentSparse"
 # This is the default protocol (and we want to test the difference between
 # colons which separate a protocol prefix from the rest and colons which are
 # just part of the filename, so we cannot test protocols which require a prefix)
-- 
2.21.0

Re: [Qemu-devel] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1047 bytes --]

On 7/25/19 10:57 AM, Max Reitz wrote:
> Several vmdk subformats do not work with iotest 126, so disable them.
> 
> (twoGbMaxExtentSparse actually should work, but fixing that is a bit
> difficult.  The problem is that the vmdk descriptor file will contain a
> referenc to "image:base.vmdk", which the block layer cannot open because

reference

> it does not know the protocol "image".  This is not trivial to solve,
> because I suppose real protocols like "http://" should be supported.
> Making vmdk treat all paths with a potential protocol prefix that the
> block layer does not recognize as plain files seems a bit weird,
> though.  Ignoring this problem does not seem too bad.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/126 | 6 ++++++
>  1 file changed, 6 insertions(+)

Are you aiming to get any of this series in -rc3, or is it firmly 4.2
material?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1269 bytes --]

On 25.07.19 19:00, Eric Blake wrote:
> On 7/25/19 10:57 AM, Max Reitz wrote:
>> Several vmdk subformats do not work with iotest 126, so disable them.
>>
>> (twoGbMaxExtentSparse actually should work, but fixing that is a bit
>> difficult.  The problem is that the vmdk descriptor file will contain a
>> referenc to "image:base.vmdk", which the block layer cannot open because
> 
> reference
> 
>> it does not know the protocol "image".  This is not trivial to solve,
>> because I suppose real protocols like "http://" should be supported.
>> Making vmdk treat all paths with a potential protocol prefix that the
>> block layer does not recognize as plain files seems a bit weird,
>> though.  Ignoring this problem does not seem too bad.)
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/126 | 6 ++++++
>>  1 file changed, 6 insertions(+)
> 
> Are you aiming to get any of this series in -rc3, or is it firmly 4.2
> material?

No bug fix is ever firmly -next material.  However, the bugs aren’t
regressions and not dramatic, so we don’t need to force it into 4.1.

If I had a strong opinion myself on where to place this series, I’d have
given it a for-4.x tag.  I didn’t, because I don’t.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[John Snow]

On 7/25/19 11:57 AM, Max Reitz wrote:
> Several vmdk subformats do not work with iotest 126, so disable them.
> 
> (twoGbMaxExtentSparse actually should work, but fixing that is a bit
> difficult.  The problem is that the vmdk descriptor file will contain a
> referenc to "image:base.vmdk", which the block layer cannot open because

reference

> it does not know the protocol "image".  This is not trivial to solve,
> because I suppose real protocols like "http://" should be supported.
> Making vmdk treat all paths with a potential protocol prefix that the
> block layer does not recognize as plain files seems a bit weird,
> though.  Ignoring this problem does not seem too bad.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/126 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
> index 9b0dcf9255..8e55d7c843 100755
> --- a/tests/qemu-iotests/126
> +++ b/tests/qemu-iotests/126
> @@ -33,6 +33,12 @@ status=1	# failure is the default!
>  
>  # Needs backing file support
>  _supported_fmt qcow qcow2 qed vmdk
> +# (1) Flat vmdk images do not support backing files
> +# (2) Split vmdk images simply fail this test right now.  Fixing that
> +#     is left for another day.

Which one? :)

> +_unsupported_imgopts "subformat=monolithicFlat" \
> +                     "subformat=twoGbMaxExtentFlat" \
> +                     "subformat=twoGbMaxExtentSparse"
>  # This is the default protocol (and we want to test the difference between
>  # colons which separate a protocol prefix from the rest and colons which are
>  # just part of the filename, so we cannot test protocols which require a prefix)
> 

What exactly fails? Does the VMDK driver see `image:` and think it's a
special filename it needs to handle and fails to do so?

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5224 bytes --]

On 12.08.19 23:33, John Snow wrote:
> 
> 
> On 7/25/19 11:57 AM, Max Reitz wrote:
>> Several vmdk subformats do not work with iotest 126, so disable them.
>>
>> (twoGbMaxExtentSparse actually should work, but fixing that is a bit
>> difficult.  The problem is that the vmdk descriptor file will contain a
>> referenc to "image:base.vmdk", which the block layer cannot open because
> 
> reference
> 
>> it does not know the protocol "image".  This is not trivial to solve,
>> because I suppose real protocols like "http://" should be supported.
>> Making vmdk treat all paths with a potential protocol prefix that the
>> block layer does not recognize as plain files seems a bit weird,
>> though.  Ignoring this problem does not seem too bad.)
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/126 | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
>> index 9b0dcf9255..8e55d7c843 100755
>> --- a/tests/qemu-iotests/126
>> +++ b/tests/qemu-iotests/126
>> @@ -33,6 +33,12 @@ status=1	# failure is the default!
>>  
>>  # Needs backing file support
>>  _supported_fmt qcow qcow2 qed vmdk
>> +# (1) Flat vmdk images do not support backing files
>> +# (2) Split vmdk images simply fail this test right now.  Fixing that
>> +#     is left for another day.
> 
> Which one? :)

Hmmmm?  Fixing refers to #2.  #1 is not a bug or missing feature, it’s
just how it is.  (This test needs backing files, so...)

If you mean “which are which“, then the ones with *Flat are flat images
(:-)), and the ones with twoGbMaxExtent* are split.

>> +_unsupported_imgopts "subformat=monolithicFlat" \
>> +                     "subformat=twoGbMaxExtentFlat" \
>> +                     "subformat=twoGbMaxExtentSparse"
>>  # This is the default protocol (and we want to test the difference between
>>  # colons which separate a protocol prefix from the rest and colons which are
>>  # just part of the filename, so we cannot test protocols which require a prefix)
>>
> 
> What exactly fails?

Interestingly I only now noticed that the test passes with “vmdk: Use
bdrv_dirname() for relative extent paths” (patch 2) reverted...

>                     Does the VMDK driver see `image:` and think it's a
> special filename it needs to handle and fails to do so?
No.  Whenever the block layer sees a parsee filename[1] with a colon
before a slash, it thinks everything before the colon is a protocol
prefix.  For example:

$ qemu-img info foo:bar
qemu-img: Could not open 'foo:bar': Unknown protocol 'foo'

This test is precisely for this.  How can you specify an image filename
that has a colon in it (without using -blockdev)?  One way is to prepend
it with “./”, the other is “file:”.

Now with split VMDKs, we must write something in the header file to
reference the extents.  What vmdk does for an image like
“image:foo.vmdk” is it writes “image:foo-s001.vmdk” there.

When it tries to open that extent, what happens depends on whether
“vmdk: Use bdrv_dirname() for relative extent paths” (patch 2) is applied:

--- Before that patch ---

vmdk takes the descriptor filename, which, thanks to some magic in the
block layer, is always “./image:foo.vmdk”, even when you gave it as
“file:image:foo.vmdk” (the “file:” is stripped because it does nothing,
generally, and the “./” is then prepended because of the false protocol
prefix “image:”).

It then invokes path_combine() with that path and the path given in the
descriptor file (“image:foo-s001.vmdk”).  This yields
“./image:foo-s001.vmdk”, which actually works.

--- After that patch ---

OK, what I messed up is that I just took the extent path to be an
absolute path if it has a protocol prefix.  (Because that’s how we
usually do it.)  Turns out that vmdk never did that, and path_combine()
actually completely ignores protocol prefixes in the relative filename.

I suppose I could do the same and just drop the path_has_protocol() from
patch 2.  But that’d be a bit broken, as I wrote in the commit
message...  If the descriptor file refers to an extent on
“http://example.com/extent.vmdk”, I suppose that should not be
interpreted as a relative path, but actually work...

But anyway, I guess if it’s a bit broken already, I might just keep it
that way.


tl;dr: Turns out patch 2 broke this test, because it (accidentally)
tried to fix something that I consider broken.  If I just keep it broken
(I didn’t know it was), this test will continue to work and probably
nobody will care because, well, it already is broken and nobody cares.

Max


[1] By this I mean whether it is piped through .bdrv_parse_filename().
If you specifying something with -hda or -drive file=, it will be.
These are filenames like nbd://localhost:10809 or blkdebug:conf:image.
If you pass a filename through QMP, that is, with -blockdev or
blockdev-add, it will not be parsed.  It will be given to the block
driver as is.  Protocol prefixes and other filename magic are ignored
(you need to explicitly specify the driver anyway).


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[John Snow]

On 8/13/19 10:00 AM, Max Reitz wrote:
> On 12.08.19 23:33, John Snow wrote:
>>
>>
>> On 7/25/19 11:57 AM, Max Reitz wrote:
>>> Several vmdk subformats do not work with iotest 126, so disable them.
>>>
>>> (twoGbMaxExtentSparse actually should work, but fixing that is a bit
>>> difficult.  The problem is that the vmdk descriptor file will contain a
>>> referenc to "image:base.vmdk", which the block layer cannot open because
>>
>> reference
>>
>>> it does not know the protocol "image".  This is not trivial to solve,
>>> because I suppose real protocols like "http://" should be supported.
>>> Making vmdk treat all paths with a potential protocol prefix that the
>>> block layer does not recognize as plain files seems a bit weird,
>>> though.  Ignoring this problem does not seem too bad.)
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  tests/qemu-iotests/126 | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
>>> index 9b0dcf9255..8e55d7c843 100755
>>> --- a/tests/qemu-iotests/126
>>> +++ b/tests/qemu-iotests/126
>>> @@ -33,6 +33,12 @@ status=1	# failure is the default!
>>>  
>>>  # Needs backing file support
>>>  _supported_fmt qcow qcow2 qed vmdk
>>> +# (1) Flat vmdk images do not support backing files
>>> +# (2) Split vmdk images simply fail this test right now.  Fixing that
>>> +#     is left for another day.
>>
>> Which one? :)
> 
> Hmmmm?  Fixing refers to #2.  #1 is not a bug or missing feature, it’s
> just how it is.  (This test needs backing files, so...)
> 
> If you mean “which are which“, then the ones with *Flat are flat images
> (:-)), and the ones with twoGbMaxExtent* are split.
> 

"Which day" ;)

>>> +_unsupported_imgopts "subformat=monolithicFlat" \
>>> +                     "subformat=twoGbMaxExtentFlat" \
>>> +                     "subformat=twoGbMaxExtentSparse"
>>>  # This is the default protocol (and we want to test the difference between
>>>  # colons which separate a protocol prefix from the rest and colons which are
>>>  # just part of the filename, so we cannot test protocols which require a prefix)
>>>
>>
>> What exactly fails?
> 
> Interestingly I only now noticed that the test passes with “vmdk: Use
> bdrv_dirname() for relative extent paths” (patch 2) reverted...
> 
>>                     Does the VMDK driver see `image:` and think it's a
>> special filename it needs to handle and fails to do so?
> No.  Whenever the block layer sees a parsee filename[1] with a colon
> before a slash, it thinks everything before the colon is a protocol
> prefix.  For example:
> 

Actually, I think we're on the same page here. I maybe meant to type
"block layer" instead of "VMDK driver", but it does look like it does
special processing on this sort of filename that breaks in this case.

> $ qemu-img info foo:bar
> qemu-img: Could not open 'foo:bar': Unknown protocol 'foo'
> 
> This test is precisely for this.  How can you specify an image filename
> that has a colon in it (without using -blockdev)?  One way is to prepend
> it with “./”, the other is “file:”.
> 
> Now with split VMDKs, we must write something in the header file to
> reference the extents.  What vmdk does for an image like
> “image:foo.vmdk” is it writes “image:foo-s001.vmdk” there.
> 
> When it tries to open that extent, what happens depends on whether
> “vmdk: Use bdrv_dirname() for relative extent paths” (patch 2) is applied:
> 
> --- Before that patch ---
> 
> vmdk takes the descriptor filename, which, thanks to some magic in the
> block layer, is always “./image:foo.vmdk”, even when you gave it as
> “file:image:foo.vmdk” (the “file:” is stripped because it does nothing,
> generally, and the “./” is then prepended because of the false protocol
> prefix “image:”).
> 
> It then invokes path_combine() with that path and the path given in the
> descriptor file (“image:foo-s001.vmdk”).  This yields
> “./image:foo-s001.vmdk”, which actually works.
> 
> --- After that patch ---
> 
> OK, what I messed up is that I just took the extent path to be an
> absolute path if it has a protocol prefix.  (Because that’s how we
> usually do it.)  Turns out that vmdk never did that, and path_combine()
> actually completely ignores protocol prefixes in the relative filename.
> 
> I suppose I could do the same and just drop the path_has_protocol() from
> patch 2.  But that’d be a bit broken, as I wrote in the commit
> message...  If the descriptor file refers to an extent on
> “http://example.com/extent.vmdk”, I suppose that should not be
> interpreted as a relative path, but actually work...
> 
> But anyway, I guess if it’s a bit broken already, I might just keep it
> that way.
> 
> 
> tl;dr: Turns out patch 2 broke this test, because it (accidentally)
> tried to fix something that I consider broken.  If I just keep it broken
> (I didn’t know it was), this test will continue to work and probably
> nobody will care because, well, it already is broken and nobody cares.
> 

So which kinda-broken thing are you making the case for? Are you
re-spinning in light of your discovery or... are we fine with the state
of things as they land here?

(Sorry, it wasn't clear to me which way you were leaning.)

--js

> Max
> 
> 
> [1] By this I mean whether it is piped through .bdrv_parse_filename().
> If you specifying something with -hda or -drive file=, it will be.
> These are filenames like nbd://localhost:10809 or blkdebug:conf:image.
> If you pass a filename through QMP, that is, with -blockdev or
> blockdev-add, it will not be parsed.  It will be given to the block
> driver as is.  Protocol prefixes and other filename magic are ignored
> (you need to explicitly specify the driver anyway).
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5689 bytes --]

On 14.08.19 00:26, John Snow wrote:
> 
> 
> On 8/13/19 10:00 AM, Max Reitz wrote:
>> On 12.08.19 23:33, John Snow wrote:
>>>
>>>
>>> On 7/25/19 11:57 AM, Max Reitz wrote:
>>>> Several vmdk subformats do not work with iotest 126, so disable them.
>>>>
>>>> (twoGbMaxExtentSparse actually should work, but fixing that is a bit
>>>> difficult.  The problem is that the vmdk descriptor file will contain a
>>>> referenc to "image:base.vmdk", which the block layer cannot open because
>>>
>>> reference
>>>
>>>> it does not know the protocol "image".  This is not trivial to solve,
>>>> because I suppose real protocols like "http://" should be supported.
>>>> Making vmdk treat all paths with a potential protocol prefix that the
>>>> block layer does not recognize as plain files seems a bit weird,
>>>> though.  Ignoring this problem does not seem too bad.)
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  tests/qemu-iotests/126 | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
>>>> index 9b0dcf9255..8e55d7c843 100755
>>>> --- a/tests/qemu-iotests/126
>>>> +++ b/tests/qemu-iotests/126
>>>> @@ -33,6 +33,12 @@ status=1	# failure is the default!
>>>>  
>>>>  # Needs backing file support
>>>>  _supported_fmt qcow qcow2 qed vmdk
>>>> +# (1) Flat vmdk images do not support backing files
>>>> +# (2) Split vmdk images simply fail this test right now.  Fixing that
>>>> +#     is left for another day.
>>>
>>> Which one? :)
>>
>> Hmmmm?  Fixing refers to #2.  #1 is not a bug or missing feature, it’s
>> just how it is.  (This test needs backing files, so...)
>>
>> If you mean “which are which“, then the ones with *Flat are flat images
>> (:-)), and the ones with twoGbMaxExtent* are split.
>>
> 
> "Which day" ;)
> 
>>>> +_unsupported_imgopts "subformat=monolithicFlat" \
>>>> +                     "subformat=twoGbMaxExtentFlat" \
>>>> +                     "subformat=twoGbMaxExtentSparse"
>>>>  # This is the default protocol (and we want to test the difference between
>>>>  # colons which separate a protocol prefix from the rest and colons which are
>>>>  # just part of the filename, so we cannot test protocols which require a prefix)
>>>>
>>>
>>> What exactly fails?
>>
>> Interestingly I only now noticed that the test passes with “vmdk: Use
>> bdrv_dirname() for relative extent paths” (patch 2) reverted...
>>
>>>                     Does the VMDK driver see `image:` and think it's a
>>> special filename it needs to handle and fails to do so?
>> No.  Whenever the block layer sees a parsee filename[1] with a colon
>> before a slash, it thinks everything before the colon is a protocol
>> prefix.  For example:
>>
> 
> Actually, I think we're on the same page here. I maybe meant to type
> "block layer" instead of "VMDK driver", but it does look like it does
> special processing on this sort of filename that breaks in this case.
> 
>> $ qemu-img info foo:bar
>> qemu-img: Could not open 'foo:bar': Unknown protocol 'foo'
>>
>> This test is precisely for this.  How can you specify an image filename
>> that has a colon in it (without using -blockdev)?  One way is to prepend
>> it with “./”, the other is “file:”.
>>
>> Now with split VMDKs, we must write something in the header file to
>> reference the extents.  What vmdk does for an image like
>> “image:foo.vmdk” is it writes “image:foo-s001.vmdk” there.
>>
>> When it tries to open that extent, what happens depends on whether
>> “vmdk: Use bdrv_dirname() for relative extent paths” (patch 2) is applied:
>>
>> --- Before that patch ---
>>
>> vmdk takes the descriptor filename, which, thanks to some magic in the
>> block layer, is always “./image:foo.vmdk”, even when you gave it as
>> “file:image:foo.vmdk” (the “file:” is stripped because it does nothing,
>> generally, and the “./” is then prepended because of the false protocol
>> prefix “image:”).
>>
>> It then invokes path_combine() with that path and the path given in the
>> descriptor file (“image:foo-s001.vmdk”).  This yields
>> “./image:foo-s001.vmdk”, which actually works.
>>
>> --- After that patch ---
>>
>> OK, what I messed up is that I just took the extent path to be an
>> absolute path if it has a protocol prefix.  (Because that’s how we
>> usually do it.)  Turns out that vmdk never did that, and path_combine()
>> actually completely ignores protocol prefixes in the relative filename.
>>
>> I suppose I could do the same and just drop the path_has_protocol() from
>> patch 2.  But that’d be a bit broken, as I wrote in the commit
>> message...  If the descriptor file refers to an extent on
>> “http://example.com/extent.vmdk”, I suppose that should not be
>> interpreted as a relative path, but actually work...
>>
>> But anyway, I guess if it’s a bit broken already, I might just keep it
>> that way.
>>
>>
>> tl;dr: Turns out patch 2 broke this test, because it (accidentally)
>> tried to fix something that I consider broken.  If I just keep it broken
>> (I didn’t know it was), this test will continue to work and probably
>> nobody will care because, well, it already is broken and nobody cares.
>>
> 
> So which kinda-broken thing are you making the case for? Are you
> re-spinning in light of your discovery or... are we fine with the state
> of things as they land here?
> 
> (Sorry, it wasn't clear to me which way you were leaning.)

I’m going to respin and drop the “path_has_protocol(fname)” condition
from patch 2.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]