[Qemu-devel] [PATCH v2 0/2] vmdk: Fix error for JSON descriptor file names

[Qemu-devel] [PATCH v2 0/2] vmdk: Fix error for JSON descriptor file names

[Max Reitz]

This series improves the error message emitted when a vmdk file has a
JSON file name and the vmdk driver tries to read the extent files (which
will not work), and adds an appropriate test case.


v2:
- Patch 1: Only error out if the extent file name is not absolute
  [Kevin]
- Patch 2: Because I changed the error message in patch 1, the test
  output needs to be amended accordingly


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[0018] [FC] 'vmdk: Fix error for JSON descriptor file names'
002/2:[0002] [FC] 'iotests: Add test for vmdk JSON file names'


Max Reitz (2):
  vmdk: Fix error for JSON descriptor file names
  iotests: Add test for vmdk JSON file names

 block.c                    |  2 +-
 block/vmdk.c               | 10 +++++++++-
 include/block/block.h      |  1 +
 tests/qemu-iotests/059     |  6 ++++++
 tests/qemu-iotests/059.out |  4 ++++
 5 files changed, 21 insertions(+), 2 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/2] vmdk: Fix error for JSON descriptor file names

[Max Reitz]

If vmdk blindly tries to use path_combine() using bs->file->filename as
the base file name, this will result in a bad error message for JSON
file names when calling bdrv_open(). It is better to only try
bs->file->exact_filename; if that is empty, bs->file->filename will be
useless for path_combine() and an error should be emitted (containing
bs->file->filename because desc_file_path (which is
bs->file->exact_filename) is empty).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               |  2 +-
 block/vmdk.c          | 10 +++++++++-
 include/block/block.h |  1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 591fbe4..239ae5f 100644
--- a/block.c
+++ b/block.c
@@ -229,7 +229,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
 }
 
 /* check if the path starts with "<protocol>:" */
-static int path_has_protocol(const char *path)
+int path_has_protocol(const char *path)
 {
     const char *p;
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 2cbfd3e..65413a1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -818,6 +818,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             goto next_line;
         }
 
+        if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
+            !desc_file_path[0])
+        {
+            error_setg(errp, "Cannot use relative extent paths with VMDK "
+                       "descriptor file '%s'", bs->file->filename);
+            return -EINVAL;
+        }
+
         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
         extent_file = NULL;
@@ -894,7 +902,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->filename, errp);
+    ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp);
 exit:
     return ret;
 }
diff --git a/include/block/block.h b/include/block/block.h
index 610be9f..919c8f5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -401,6 +401,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
                                     char *dest, size_t sz);
 int bdrv_is_snapshot(BlockDriverState *bs);
 
+int path_has_protocol(const char *path);
 int path_is_absolute(const char *path);
 void path_combine(char *dest, int dest_size,
                   const char *base_path,
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/2] iotests: Add test for vmdk JSON file names

[Max Reitz]

Add a test for vmdk files which use a file with a JSON file name, and
which then try to open extents. That should fail and the error message
should at least try to look helpful.

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

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 2ed1a7f..50ca5ce 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -111,6 +111,12 @@ $QEMU_IO -f qcow2 -c "write -P 0xb 10240 512" "$TEST_IMG.qcow2" | _filter_qemu_i
 $QEMU_IMG convert -f qcow2 -O vmdk -o subformat=streamOptimized "$TEST_IMG.qcow2" "$TEST_IMG" 2>&1
 
 echo
+echo "=== Testing monolithicFlat with internally generated JSON file name ==="
+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
+
+echo
 echo "=== Testing version 3 ==="
 _use_sample_img iotest-version3.vmdk.bz2
 _img_info
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 0dadba6..cbb0de4 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2053,6 +2053,10 @@ wrote 512/512 bytes at offset 0
 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
+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 ===
 image: TEST_DIR/iotest-version3.IMGFMT
 file format: IMGFMT
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 1/2] vmdk: Fix error for JSON descriptor file names

[Fam Zheng]

On Wed, 12/03 14:57, Max Reitz wrote:
> If vmdk blindly tries to use path_combine() using bs->file->filename as
> the base file name, this will result in a bad error message for JSON
> file names when calling bdrv_open(). It is better to only try
> bs->file->exact_filename; if that is empty, bs->file->filename will be
> useless for path_combine() and an error should be emitted (containing
> bs->file->filename because desc_file_path (which is
> bs->file->exact_filename) is empty).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               |  2 +-
>  block/vmdk.c          | 10 +++++++++-
>  include/block/block.h |  1 +
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 591fbe4..239ae5f 100644
> --- a/block.c
> +++ b/block.c
> @@ -229,7 +229,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>  }
>  
>  /* check if the path starts with "<protocol>:" */
> -static int path_has_protocol(const char *path)
> +int path_has_protocol(const char *path)
>  {
>      const char *p;
>  
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2cbfd3e..65413a1 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -818,6 +818,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>              goto next_line;
>          }
>  
> +        if (!path_is_absolute(fname) && !path_has_protocol(fname) &&

If I read the VMDK spec correctly, fname can't have protocol (as a format spec
it hasn't a single word on protocol :). But it's not wrong to check. I continue
reading thinking it as "true".

fname, on the other hand, is in most cases a relative path. In this case, this
condition falls to desc_file_path, which is bs->file->exact_filename. This will
be non-empty for local files, so the path_combine below will work.

If fname is absolute, path_combine will also work.

So it is correct.

> +            !desc_file_path[0])
> +        {
> +            error_setg(errp, "Cannot use relative extent paths with VMDK "
> +                       "descriptor file '%s'", bs->file->filename);
> +            return -EINVAL;
> +        }
> +
>          path_combine(extent_path, sizeof(extent_path),
>                  desc_file_path, fname);
>          extent_file = NULL;
> @@ -894,7 +902,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->filename, errp);
> +    ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp);
>  exit:
>      return ret;
>  }
> diff --git a/include/block/block.h b/include/block/block.h
> index 610be9f..919c8f5 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -401,6 +401,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
>                                      char *dest, size_t sz);
>  int bdrv_is_snapshot(BlockDriverState *bs);
>  
> +int path_has_protocol(const char *path);

Exporting path_has_protocol could be a separate patch but I don't have a strong
objection as this patch is still short.

Reviewed-by: Fam Zheng <famz@redhat.com>

>  int path_is_absolute(const char *path);
>  void path_combine(char *dest, int dest_size,
>                    const char *base_path,
> -- 
> 1.9.3
> 

Re: [Qemu-devel] [PATCH v2 2/2] iotests: Add test for vmdk JSON file names

[Fam Zheng]

On Wed, 12/03 14:57, Max Reitz wrote:
> Add a test for vmdk files which use a file with a JSON file name, and
> which then try to open extents. That should fail and the error message
> should at least try to look helpful.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/059     | 6 ++++++
>  tests/qemu-iotests/059.out | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
> index 2ed1a7f..50ca5ce 100755
> --- a/tests/qemu-iotests/059
> +++ b/tests/qemu-iotests/059
> @@ -111,6 +111,12 @@ $QEMU_IO -f qcow2 -c "write -P 0xb 10240 512" "$TEST_IMG.qcow2" | _filter_qemu_i
>  $QEMU_IMG convert -f qcow2 -O vmdk -o subformat=streamOptimized "$TEST_IMG.qcow2" "$TEST_IMG" 2>&1
>  
>  echo
> +echo "=== Testing monolithicFlat with internally generated JSON file name ==="
> +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
> +
> +echo
>  echo "=== Testing version 3 ==="
>  _use_sample_img iotest-version3.vmdk.bz2
>  _img_info
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index 0dadba6..cbb0de4 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -2053,6 +2053,10 @@ wrote 512/512 bytes at offset 0
>  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
> +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 ===
>  image: TEST_DIR/iotest-version3.IMGFMT
>  file format: IMGFMT
> -- 
> 1.9.3
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/2] vmdk: Fix error for JSON descriptor file names

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

On Wed, Dec 03, 2014 at 02:57:21PM +0100, Max Reitz wrote:
> This series improves the error message emitted when a vmdk file has a
> JSON file name and the vmdk driver tries to read the extent files (which
> will not work), and adds an appropriate test case.
> 
> 
> v2:
> - Patch 1: Only error out if the extent file name is not absolute
>   [Kevin]
> - Patch 2: Because I changed the error message in patch 1, the test
>   output needs to be amended accordingly
> 
> 
> git-backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/2:[0018] [FC] 'vmdk: Fix error for JSON descriptor file names'
> 002/2:[0002] [FC] 'iotests: Add test for vmdk JSON file names'
> 
> 
> Max Reitz (2):
>   vmdk: Fix error for JSON descriptor file names
>   iotests: Add test for vmdk JSON file names
> 
>  block.c                    |  2 +-
>  block/vmdk.c               | 10 +++++++++-
>  include/block/block.h      |  1 +
>  tests/qemu-iotests/059     |  6 ++++++
>  tests/qemu-iotests/059.out |  4 ++++
>  5 files changed, 21 insertions(+), 2 deletions(-)
> 
> -- 
> 1.9.3
> 
> 

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]