Re: [Qemu-devel] [PATCH] block: Don't probe for unknown backing file format

[Max Reitz <mreitz@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 <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