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

[Qemu-devel] [PATCH 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.

This series depends on
"[PATCH] iotests: Specify qcow2 format for qemu-io in 059".


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

 block/vmdk.c               | 9 ++++++++-
 tests/qemu-iotests/059     | 6 ++++++
 tests/qemu-iotests/059.out | 4 ++++
 3 files changed, 18 insertions(+), 1 deletion(-)

-- 
1.9.3

[Qemu-devel] [PATCH 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 bs->file->exact_filename is empty).

Note that s->create_type does not need to be freed on error because it
will be freed by the caller (which ultimately is vmdk_open()).

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

diff --git a/block/vmdk.c b/block/vmdk.c
index 2cbfd3e..fe549c2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -894,7 +894,14 @@ 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);
+
+    if (!bs->file->exact_filename[0]) {
+        error_setg(errp, "Cannot use extents with VMDK descriptor file '%s'",
+                   bs->file->filename);
+        ret = -EINVAL;
+        goto exit;
+    }
+    ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp);
 exit:
     return ret;
 }
-- 
1.9.3

[Qemu-devel] [PATCH 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..97509ab 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 extents 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 1/2] vmdk: Fix error for JSON descriptor file names

[Fam Zheng]

On Wed, 12/03 10:23, 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 bs->file->exact_filename is empty).
> 
> Note that s->create_type does not need to be freed on error because it
> will be freed by the caller (which ultimately is vmdk_open()).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2cbfd3e..fe549c2 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -894,7 +894,14 @@ 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);
> +
> +    if (!bs->file->exact_filename[0]) {
> +        error_setg(errp, "Cannot use extents with VMDK descriptor file '%s'",
> +                   bs->file->filename);
> +        ret = -EINVAL;
> +        goto exit;
> +    }
> +    ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp);
>  exit:
>      return ret;
>  }
> -- 
> 1.9.3
> 

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

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

[Fam Zheng]

On Wed, 12/03 10:23, 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..97509ab 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 extents 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 1/2] vmdk: Fix error for JSON descriptor file names

[Kevin Wolf]

Am 03.12.2014 um 10:23 hat Max Reitz geschrieben:
> 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 bs->file->exact_filename is empty).
> 
> Note that s->create_type does not need to be freed on error because it
> will be freed by the caller (which ultimately is vmdk_open()).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2cbfd3e..fe549c2 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -894,7 +894,14 @@ 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);
> +
> +    if (!bs->file->exact_filename[0]) {
> +        error_setg(errp, "Cannot use extents with VMDK descriptor file '%s'",
> +                   bs->file->filename);
> +        ret = -EINVAL;
> +        goto exit;
> +    }

Isn't this overly restrictive? If the extent paths are all absolute,
there is no reason not to open them. Or does the VMDK spec say that they
are always relative?

> +    ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp);
>  exit:
>      return ret;
>  }

Kevin

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

[Max Reitz]

On 2014-12-03 at 13:44, Kevin Wolf wrote:
> Am 03.12.2014 um 10:23 hat Max Reitz geschrieben:
>> 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 bs->file->exact_filename is empty).
>>
>> Note that s->create_type does not need to be freed on error because it
>> will be freed by the caller (which ultimately is vmdk_open()).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/vmdk.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 2cbfd3e..fe549c2 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -894,7 +894,14 @@ 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);
>> +
>> +    if (!bs->file->exact_filename[0]) {
>> +        error_setg(errp, "Cannot use extents with VMDK descriptor file '%s'",
>> +                   bs->file->filename);
>> +        ret = -EINVAL;
>> +        goto exit;
>> +    }
> Isn't this overly restrictive? If the extent paths are all absolute,
> there is no reason not to open them. Or does the VMDK spec say that they
> are always relative?

Yes, you're right. Will respin.

Max

>> +    ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp);
>>   exit:
>>       return ret;
>>   }
> Kevin