From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39867) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtHWM-0007YS-1L for qemu-devel@nongnu.org; Tue, 25 Nov 2014 09:55:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtHWF-0000p7-1f for qemu-devel@nongnu.org; Tue, 25 Nov 2014 09:55:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47773) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtHWE-0000kL-PE for qemu-devel@nongnu.org; Tue, 25 Nov 2014 09:55:06 -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 sAPEt5Zn026805 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 25 Nov 2014 09:55:05 -0500 Message-ID: <54749846.7010907@redhat.com> Date: Tue, 25 Nov 2014 15:55:02 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416925164-17848-1-git-send-email-kwolf@redhat.com> <547496C5.6060500@redhat.com> In-Reply-To: <547496C5.6060500@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: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 >> --- >> 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()). 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