* [Qemu-devel] [PATCH 0/2] vmdk: Fix error for JSON descriptor file names @ 2014-12-03 9:23 Max Reitz 2014-12-03 9:23 ` [Qemu-devel] [PATCH 1/2] " Max Reitz 2014-12-03 9:23 ` [Qemu-devel] [PATCH 2/2] iotests: Add test for vmdk JSON " Max Reitz 0 siblings, 2 replies; 7+ messages in thread From: Max Reitz @ 2014-12-03 9:23 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/2] vmdk: Fix error for JSON descriptor file names 2014-12-03 9:23 [Qemu-devel] [PATCH 0/2] vmdk: Fix error for JSON descriptor file names Max Reitz @ 2014-12-03 9:23 ` Max Reitz 2014-12-03 9:40 ` Fam Zheng 2014-12-03 12:44 ` Kevin Wolf 2014-12-03 9:23 ` [Qemu-devel] [PATCH 2/2] iotests: Add test for vmdk JSON " Max Reitz 1 sibling, 2 replies; 7+ messages in thread From: Max Reitz @ 2014-12-03 9:23 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vmdk: Fix error for JSON descriptor file names 2014-12-03 9:23 ` [Qemu-devel] [PATCH 1/2] " Max Reitz @ 2014-12-03 9:40 ` Fam Zheng 2014-12-03 12:44 ` Kevin Wolf 1 sibling, 0 replies; 7+ messages in thread From: Fam Zheng @ 2014-12-03 9:40 UTC (permalink / raw) To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vmdk: Fix error for JSON descriptor file names 2014-12-03 9:23 ` [Qemu-devel] [PATCH 1/2] " Max Reitz 2014-12-03 9:40 ` Fam Zheng @ 2014-12-03 12:44 ` Kevin Wolf 2014-12-03 13:08 ` Max Reitz 1 sibling, 1 reply; 7+ messages in thread From: Kevin Wolf @ 2014-12-03 12:44 UTC (permalink / raw) To: Max Reitz; +Cc: Fam Zheng, qemu-devel, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vmdk: Fix error for JSON descriptor file names 2014-12-03 12:44 ` Kevin Wolf @ 2014-12-03 13:08 ` Max Reitz 0 siblings, 0 replies; 7+ messages in thread From: Max Reitz @ 2014-12-03 13:08 UTC (permalink / raw) To: Kevin Wolf; +Cc: Fam Zheng, qemu-devel, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/2] iotests: Add test for vmdk JSON file names 2014-12-03 9:23 [Qemu-devel] [PATCH 0/2] vmdk: Fix error for JSON descriptor file names Max Reitz 2014-12-03 9:23 ` [Qemu-devel] [PATCH 1/2] " Max Reitz @ 2014-12-03 9:23 ` Max Reitz 2014-12-03 9:41 ` Fam Zheng 1 sibling, 1 reply; 7+ messages in thread From: Max Reitz @ 2014-12-03 9:23 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for vmdk JSON file names 2014-12-03 9:23 ` [Qemu-devel] [PATCH 2/2] iotests: Add test for vmdk JSON " Max Reitz @ 2014-12-03 9:41 ` Fam Zheng 0 siblings, 0 replies; 7+ messages in thread From: Fam Zheng @ 2014-12-03 9:41 UTC (permalink / raw) To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-12-03 13:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-03 9:23 [Qemu-devel] [PATCH 0/2] vmdk: Fix error for JSON descriptor file names Max Reitz 2014-12-03 9:23 ` [Qemu-devel] [PATCH 1/2] " Max Reitz 2014-12-03 9:40 ` Fam Zheng 2014-12-03 12:44 ` Kevin Wolf 2014-12-03 13:08 ` Max Reitz 2014-12-03 9:23 ` [Qemu-devel] [PATCH 2/2] iotests: Add test for vmdk JSON " Max Reitz 2014-12-03 9:41 ` Fam Zheng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.