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

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