From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtHQ7-0006Rr-42 for qemu-devel@nongnu.org; Tue, 25 Nov 2014 09:48:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtHQ1-0006ca-R2 for qemu-devel@nongnu.org; Tue, 25 Nov 2014 09:48:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43258) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtHQ1-0006cK-IZ for qemu-devel@nongnu.org; Tue, 25 Nov 2014 09:48:41 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAPEme00018069 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 25 Nov 2014 09:48:40 -0500 Message-ID: <547496C5.6060500@redhat.com> Date: Tue, 25 Nov 2014 15:48:37 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416925164-17848-1-git-send-email-kwolf@redhat.com> In-Reply-To: <1416925164-17848-1-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-devel@nongnu.org Cc: armbru@redhat.com, stefanha@redhat.com 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 > --- > 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 . > +# > + > +# 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