* [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format @ 2014-11-25 14:19 Kevin Wolf 2014-11-25 14:48 ` Max Reitz 2014-11-25 17:17 ` Stefan Hajnoczi 0 siblings, 2 replies; 4+ messages in thread From: Kevin Wolf @ 2014-11-25 14:19 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz If a qcow2 image specifies a backing file format that doesn't correspond to any format driver that qemu knows, we shouldn't fall back to probing, but simply error out. Not looking up the backing file driver in bdrv_open_backing_file(), but just filling in the "driver" option if it isn't there moves us closer to the goal of having everything in QDict options and gets us the error handling of bdrv_open(), which correctly refuses unknown drivers. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 7 +++--- tests/qemu-iotests/114 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/114.out | 13 ++++++++++ tests/qemu-iotests/group | 1 + 4 files changed, 79 insertions(+), 4 deletions(-) create mode 100755 tests/qemu-iotests/114 create mode 100644 tests/qemu-iotests/114.out diff --git a/block.c b/block.c index 674538e..f0ce60b 100644 --- a/block.c +++ b/block.c @@ -1180,7 +1180,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) { char *backing_filename = g_malloc0(PATH_MAX); int ret = 0; - BlockDriver *back_drv = NULL; BlockDriverState *backing_hd; Error *local_err = NULL; @@ -1213,14 +1212,14 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) backing_hd = bdrv_new(); - if (bs->backing_format[0] != '\0') { - back_drv = bdrv_find_format(bs->backing_format); + if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) { + qdict_put(options, "driver", qstring_from_str(bs->backing_format)); } assert(bs->backing_hd == NULL); ret = bdrv_open(&backing_hd, *backing_filename ? backing_filename : NULL, NULL, options, - bdrv_backing_flags(bs->open_flags), back_drv, &local_err); + bdrv_backing_flags(bs->open_flags), NULL, &local_err); if (ret < 0) { bdrv_unref(backing_hd); backing_hd = NULL; diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114 new file mode 100755 index 0000000..7827256 --- /dev/null +++ b/tests/qemu-iotests/114 @@ -0,0 +1,62 @@ +#!/bin/bash +# +# Test invalid backing file format in qcow2 images +# +# Copyright (C) 2014 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator +owner=kwolf@redhat.com + +seq="$(basename $0)" +echo "QA output created by $seq" + +here="$PWD" +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +# Any format supporting backing files +_supported_fmt qcow2 +_supported_proto generic +_supported_os Linux + + +TEST_IMG="$TEST_IMG.base" _make_test_img 64M +_make_test_img -b "$TEST_IMG.base" 64M + +# Set an invalid backing file format +$PYTHON qcow2.py "$TEST_IMG" add-header-ext 0xE2792ACA "foo" +_img_info + +# Try opening the image. Should fail (and not probe) in the first case, but +# overriding the backing file format should be possible. +$QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "open -o backing.driver=$IMGFMT $TEST_IMG" -c "read 0 4k" | _filter_qemu_io + +# success, all done +echo '*** done' +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out new file mode 100644 index 0000000..6c6b210 --- /dev/null +++ b/tests/qemu-iotests/114.out @@ -0,0 +1,13 @@ +QA output created by 114 +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='TEST_DIR/t.IMGFMT.base' +image: TEST_DIR/t.IMGFMT +file format: IMGFMT +virtual size: 64M (67108864 bytes) +cluster_size: 65536 +backing file: TEST_DIR/t.IMGFMT.base +backing file format: foo +qemu-io: can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown driver 'foo' +read 4096/4096 bytes at offset 0 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 7dfe469..9188049 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -112,3 +112,4 @@ 107 rw auto quick 108 rw auto quick 111 rw auto quick +114 rw auto quick -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format 2014-11-25 14:19 [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format Kevin Wolf @ 2014-11-25 14:48 ` Max Reitz 2014-11-25 14:55 ` Max Reitz 2014-11-25 17:17 ` Stefan Hajnoczi 1 sibling, 1 reply; 4+ messages in thread From: Max Reitz @ 2014-11-25 14:48 UTC (permalink / raw) To: Kevin Wolf, qemu-devel; +Cc: armbru, stefanha On 2014-11-25 at 15:19, Kevin Wolf wrote: > If a qcow2 image specifies a backing file format that doesn't correspond > to any format driver that qemu knows, we shouldn't fall back to probing, > but simply error out. > > Not looking up the backing file driver in bdrv_open_backing_file(), but > just filling in the "driver" option if it isn't there moves us closer to > the goal of having everything in QDict options and gets us the error > handling of bdrv_open(), which correctly refuses unknown drivers. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 7 +++--- > tests/qemu-iotests/114 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/114.out | 13 ++++++++++ > tests/qemu-iotests/group | 1 + > 4 files changed, 79 insertions(+), 4 deletions(-) > create mode 100755 tests/qemu-iotests/114 > create mode 100644 tests/qemu-iotests/114.out > > diff --git a/block.c b/block.c > index 674538e..f0ce60b 100644 > --- a/block.c > +++ b/block.c > @@ -1180,7 +1180,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) > { > char *backing_filename = g_malloc0(PATH_MAX); > int ret = 0; > - BlockDriver *back_drv = NULL; > BlockDriverState *backing_hd; > Error *local_err = NULL; > > @@ -1213,14 +1212,14 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) > > backing_hd = bdrv_new(); > > - if (bs->backing_format[0] != '\0') { > - back_drv = bdrv_find_format(bs->backing_format); > + if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) { > + qdict_put(options, "driver", qstring_from_str(bs->backing_format)); > } > > assert(bs->backing_hd == NULL); > ret = bdrv_open(&backing_hd, > *backing_filename ? backing_filename : NULL, NULL, options, > - bdrv_backing_flags(bs->open_flags), back_drv, &local_err); > + bdrv_backing_flags(bs->open_flags), NULL, &local_err); > if (ret < 0) { > bdrv_unref(backing_hd); > backing_hd = NULL; > diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114 > new file mode 100755 > index 0000000..7827256 > --- /dev/null > +++ b/tests/qemu-iotests/114 > @@ -0,0 +1,62 @@ > +#!/bin/bash > +# > +# Test invalid backing file format in qcow2 images > +# > +# Copyright (C) 2014 Red Hat, Inc. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > +# > + > +# creator > +owner=kwolf@redhat.com > + > +seq="$(basename $0)" > +echo "QA output created by $seq" > + > +here="$PWD" > +tmp=/tmp/$$ > +status=1 # failure is the default! > + > +_cleanup() > +{ > + _cleanup_test_img > +} > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common.rc > +. ./common.filter > + > +# Any format supporting backing files > +_supported_fmt qcow2 Well... :-P > +_supported_proto generic > +_supported_os Linux > + > + > +TEST_IMG="$TEST_IMG.base" _make_test_img 64M > +_make_test_img -b "$TEST_IMG.base" 64M > + > +# Set an invalid backing file format > +$PYTHON qcow2.py "$TEST_IMG" add-header-ext 0xE2792ACA "foo" > +_img_info > + > +# Try opening the image. Should fail (and not probe) in the first case, but > +# overriding the backing file format should be possible. > +$QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | _filter_testdir > +$QEMU_IO -c "open -o backing.driver=$IMGFMT $TEST_IMG" -c "read 0 4k" | _filter_qemu_io > + > +# success, all done > +echo '*** done' > +rm -f $seq.full > +status=0 > diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out > new file mode 100644 > index 0000000..6c6b210 > --- /dev/null > +++ b/tests/qemu-iotests/114.out > @@ -0,0 +1,13 @@ > +QA output created by 114 > +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='TEST_DIR/t.IMGFMT.base' > +image: TEST_DIR/t.IMGFMT > +file format: IMGFMT > +virtual size: 64M (67108864 bytes) > +cluster_size: 65536 > +backing file: TEST_DIR/t.IMGFMT.base > +backing file format: foo > +qemu-io: can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown driver 'foo' > +read 4096/4096 bytes at offset 0 > +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +*** done > diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group > index 7dfe469..9188049 100644 > --- a/tests/qemu-iotests/group > +++ b/tests/qemu-iotests/group > @@ -112,3 +112,4 @@ > 107 rw auto quick > 108 rw auto quick > 111 rw auto quick > +114 rw auto quick The test fails for me, for two reasons. First, qcow2.py does not pad newly added header fields, which makes qcow2 not see the end-of-extensions field, which will then make qcow2 interpret the backing file string as an extension field. Oops. Fixed by having while fd.tell() % 8: fd.write("\0") or something like that (rather something like that, with fd.write("\0" * padding_required), if that works in Python) in update_extensions() in qcow2.py (after fd.write(ex.data)). Second, qcow2 uses g_malloc0() for unknown header extensions. Oops: (process:20246): GLib-ERROR **: gmem.c:140: failed to allocate 1752132989 bytes Actually, there is no reason to read the data at all... Mind if we remove that bdrv_pread() there? The data buffer is not even freed, I have no idea why the code looks like that... (below "default:" in qcow2_read_extensions()). Max ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format 2014-11-25 14:48 ` Max Reitz @ 2014-11-25 14:55 ` Max Reitz 0 siblings, 0 replies; 4+ messages in thread From: Max Reitz @ 2014-11-25 14:55 UTC (permalink / raw) To: Kevin Wolf, qemu-devel; +Cc: armbru, stefanha On 2014-11-25 at 15:48, Max Reitz wrote: > On 2014-11-25 at 15:19, Kevin Wolf wrote: >> If a qcow2 image specifies a backing file format that doesn't correspond >> to any format driver that qemu knows, we shouldn't fall back to probing, >> but simply error out. >> >> Not looking up the backing file driver in bdrv_open_backing_file(), but >> just filling in the "driver" option if it isn't there moves us closer to >> the goal of having everything in QDict options and gets us the error >> handling of bdrv_open(), which correctly refuses unknown drivers. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block.c | 7 +++--- >> tests/qemu-iotests/114 | 62 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/114.out | 13 ++++++++++ >> tests/qemu-iotests/group | 1 + >> 4 files changed, 79 insertions(+), 4 deletions(-) >> create mode 100755 tests/qemu-iotests/114 >> create mode 100644 tests/qemu-iotests/114.out >> >> diff --git a/block.c b/block.c >> index 674538e..f0ce60b 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1180,7 +1180,6 @@ int bdrv_open_backing_file(BlockDriverState >> *bs, QDict *options, Error **errp) >> { >> char *backing_filename = g_malloc0(PATH_MAX); >> int ret = 0; >> - BlockDriver *back_drv = NULL; >> BlockDriverState *backing_hd; >> Error *local_err = NULL; >> @@ -1213,14 +1212,14 @@ int bdrv_open_backing_file(BlockDriverState >> *bs, QDict *options, Error **errp) >> backing_hd = bdrv_new(); >> - if (bs->backing_format[0] != '\0') { >> - back_drv = bdrv_find_format(bs->backing_format); >> + if (bs->backing_format[0] != '\0' && !qdict_haskey(options, >> "driver")) { >> + qdict_put(options, "driver", >> qstring_from_str(bs->backing_format)); >> } >> assert(bs->backing_hd == NULL); >> ret = bdrv_open(&backing_hd, >> *backing_filename ? backing_filename : NULL, >> NULL, options, >> - bdrv_backing_flags(bs->open_flags), back_drv, >> &local_err); >> + bdrv_backing_flags(bs->open_flags), NULL, >> &local_err); >> if (ret < 0) { >> bdrv_unref(backing_hd); >> backing_hd = NULL; >> diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114 >> new file mode 100755 >> index 0000000..7827256 >> --- /dev/null >> +++ b/tests/qemu-iotests/114 >> @@ -0,0 +1,62 @@ >> +#!/bin/bash >> +# >> +# Test invalid backing file format in qcow2 images >> +# >> +# Copyright (C) 2014 Red Hat, Inc. >> +# >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 2 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see <http://www.gnu.org/licenses/>. >> +# >> + >> +# creator >> +owner=kwolf@redhat.com >> + >> +seq="$(basename $0)" >> +echo "QA output created by $seq" >> + >> +here="$PWD" >> +tmp=/tmp/$$ >> +status=1 # failure is the default! >> + >> +_cleanup() >> +{ >> + _cleanup_test_img >> +} >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +# get standard environment, filters and checks >> +. ./common.rc >> +. ./common.filter >> + >> +# Any format supporting backing files >> +_supported_fmt qcow2 > > Well... :-P > >> +_supported_proto generic >> +_supported_os Linux >> + >> + >> +TEST_IMG="$TEST_IMG.base" _make_test_img 64M >> +_make_test_img -b "$TEST_IMG.base" 64M >> + >> +# Set an invalid backing file format >> +$PYTHON qcow2.py "$TEST_IMG" add-header-ext 0xE2792ACA "foo" >> +_img_info >> + >> +# Try opening the image. Should fail (and not probe) in the first >> case, but >> +# overriding the backing file format should be possible. >> +$QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | >> _filter_testdir >> +$QEMU_IO -c "open -o backing.driver=$IMGFMT $TEST_IMG" -c "read 0 >> 4k" | _filter_qemu_io >> + >> +# success, all done >> +echo '*** done' >> +rm -f $seq.full >> +status=0 >> diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out >> new file mode 100644 >> index 0000000..6c6b210 >> --- /dev/null >> +++ b/tests/qemu-iotests/114.out >> @@ -0,0 +1,13 @@ >> +QA output created by 114 >> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 >> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 >> backing_file='TEST_DIR/t.IMGFMT.base' >> +image: TEST_DIR/t.IMGFMT >> +file format: IMGFMT >> +virtual size: 64M (67108864 bytes) >> +cluster_size: 65536 >> +backing file: TEST_DIR/t.IMGFMT.base >> +backing file format: foo >> +qemu-io: can't open device TEST_DIR/t.qcow2: Could not open backing >> file: Unknown driver 'foo' >> +read 4096/4096 bytes at offset 0 >> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> +*** done >> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group >> index 7dfe469..9188049 100644 >> --- a/tests/qemu-iotests/group >> +++ b/tests/qemu-iotests/group >> @@ -112,3 +112,4 @@ >> 107 rw auto quick >> 108 rw auto quick >> 111 rw auto quick >> +114 rw auto quick > > The test fails for me, for two reasons. > > First, qcow2.py does not pad newly added header fields, which makes > qcow2 not see the end-of-extensions field, which will then make qcow2 > interpret the backing file string as an extension field. Oops. > > Fixed by having > > while fd.tell() % 8: > fd.write("\0") > > or something like that (rather something like that, with fd.write("\0" > * padding_required), if that works in Python) in update_extensions() > in qcow2.py (after fd.write(ex.data)). > > Second, qcow2 uses g_malloc0() for unknown header extensions. Oops: > > (process:20246): GLib-ERROR **: gmem.c:140: failed to allocate > 1752132989 bytes > > Actually, there is no reason to read the data at all... Mind if we > remove that bdrv_pread() there? The data buffer is not even freed, I > have no idea why the code looks like that... (below "default:" in > qcow2_read_extensions()). Oh, that's why it isn't freed. It's entered into a list of unknown header extensions to keep it when rewriting the header. Well, fine, but we should at least be using g_try_malloc0(). Max ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format 2014-11-25 14:19 [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format Kevin Wolf 2014-11-25 14:48 ` Max Reitz @ 2014-11-25 17:17 ` Stefan Hajnoczi 1 sibling, 0 replies; 4+ messages in thread From: Stefan Hajnoczi @ 2014-11-25 17:17 UTC (permalink / raw) To: Kevin Wolf; +Cc: armbru, qemu-devel, mreitz [-- Attachment #1: Type: text/plain, Size: 1011 bytes --] On Tue, Nov 25, 2014 at 03:19:24PM +0100, Kevin Wolf wrote: > If a qcow2 image specifies a backing file format that doesn't correspond > to any format driver that qemu knows, we shouldn't fall back to probing, > but simply error out. > > Not looking up the backing file driver in bdrv_open_backing_file(), but > just filling in the "driver" option if it isn't there moves us closer to > the goal of having everything in QDict options and gets us the error > handling of bdrv_open(), which correctly refuses unknown drivers. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 7 +++--- > tests/qemu-iotests/114 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/114.out | 13 ++++++++++ > tests/qemu-iotests/group | 1 + > 4 files changed, 79 insertions(+), 4 deletions(-) > create mode 100755 tests/qemu-iotests/114 > create mode 100644 tests/qemu-iotests/114.out Looks good, modulo the test failure that Max noticed. [-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-11-25 17:17 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-25 14:19 [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format Kevin Wolf 2014-11-25 14:48 ` Max Reitz 2014-11-25 14:55 ` Max Reitz 2014-11-25 17:17 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).