[Qemu-devel] [PATCH 0/2] block: Propagate error in bdrv_img_create()

[Qemu-devel] [PATCH 0/2] block: Propagate error in bdrv_img_create()

[Max Reitz]

Currently, when trying to create a backed image without specifying its
size, when the backing file does not exist or is not accessible, an
appropriate error message will be generated which is then (in
bdrv_img_create()) prefixed with the image file name and the strerror().
However, both are generally already part of the bdrv_open() error
message, so we should not double this information. An example:

$ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
qemu-img: /tmp/img.qcow2: Could not open '/tmp/enoent': Could not open
'/tmp/enoent': No such file or directory: No such file or directory

Just propagating the error is sufficient:

$ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
qemu-img /tmp/img.qcow2: Could not open '/tmp/enoent': No such file or
directory


Max Reitz (2):
  block: Propagate error in bdrv_img_create()
  iotests: Add test for non-existing backing file

 block.c                    |  5 -----
 tests/qemu-iotests/111     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/111.out |  3 +++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 57 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/111
 create mode 100644 tests/qemu-iotests/111.out

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] block: Propagate error in bdrv_img_create()

[Max Reitz]

If the specified backing file could not be opened, do not generate a new
error message which contains the message which has been generated by
bdrv_open(), but just propagate the latter.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/block.c b/block.c
index 88f6d9b..44fa908 100644
--- a/block.c
+++ b/block.c
@@ -5577,11 +5577,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
             ret = bdrv_open(&bs, backing_file, NULL, NULL, back_flags,
                             backing_drv, &local_err);
             if (ret < 0) {
-                error_setg_errno(errp, -ret, "Could not open '%s': %s",
-                                 backing_file,
-                                 error_get_pretty(local_err));
-                error_free(local_err);
-                local_err = NULL;
                 goto out;
             }
             size = bdrv_getlength(bs);
-- 
1.9.3

[Qemu-devel] [PATCH 2/2] iotests: Add test for non-existing backing file

[Max Reitz]

Test the error message when a COW file is about to be created which is
supposed to inherit the size of its backing file, while the backing file
given does not actually exist.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/111     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/111.out |  3 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 57 insertions(+)
 create mode 100755 tests/qemu-iotests/111
 create mode 100644 tests/qemu-iotests/111.out

diff --git a/tests/qemu-iotests/111 b/tests/qemu-iotests/111
new file mode 100755
index 0000000..6011c94
--- /dev/null
+++ b/tests/qemu-iotests/111
@@ -0,0 +1,53 @@
+#!/bin/bash
+#
+# Test case for non-existing backing file when creating a qcow2 image
+# and not specifying the size
+#
+# 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=mreitz@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
+
+_supported_fmt qed qcow qcow2 vmdk
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
+
+$QEMU_IMG create -f $IMGFMT -b "$TEST_IMG.inexistent" "$TEST_IMG" 2>&1 \
+    | _filter_testdir | _filter_imgfmt
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/111.out b/tests/qemu-iotests/111.out
new file mode 100644
index 0000000..683c01a
--- /dev/null
+++ b/tests/qemu-iotests/111.out
@@ -0,0 +1,3 @@
+QA output created by 111
+qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent': No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9bbd5d3..0a4ab3a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -109,3 +109,4 @@
 105 rw auto quick
 107 rw auto quick
 108 rw auto quick
+111 rw auto quick
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/2] block: Propagate error in bdrv_img_create()

[Peter Lieven]

On 27.10.2014 13:30, Max Reitz wrote:
> Currently, when trying to create a backed image without specifying its
> size, when the backing file does not exist or is not accessible, an
> appropriate error message will be generated which is then (in
> bdrv_img_create()) prefixed with the image file name and the strerror().
> However, both are generally already part of the bdrv_open() error
> message, so we should not double this information. An example:
>
> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> qemu-img: /tmp/img.qcow2: Could not open '/tmp/enoent': Could not open
> '/tmp/enoent': No such file or directory: No such file or directory
>
> Just propagating the error is sufficient:
>
> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> qemu-img /tmp/img.qcow2: Could not open '/tmp/enoent': No such file or
> directory
>
>
> Max Reitz (2):
>    block: Propagate error in bdrv_img_create()
>    iotests: Add test for non-existing backing file
>
>   block.c                    |  5 -----
>   tests/qemu-iotests/111     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/111.out |  3 +++
>   tests/qemu-iotests/group   |  1 +
>   4 files changed, 57 insertions(+), 5 deletions(-)
>   create mode 100755 tests/qemu-iotests/111
>   create mode 100644 tests/qemu-iotests/111.out
>

Both patches:

Reviewed-by: Peter Lieven <pl@kamp.de>

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for non-existing backing file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On 10/27/2014 06:30 AM, Max Reitz wrote:
> Test the error message when a COW file is about to be created which is
> supposed to inherit the size of its backing file, while the backing file
> given does not actually exist.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> +
> +$QEMU_IMG create -f $IMGFMT -b "$TEST_IMG.inexistent" "$TEST_IMG" 2>&1 \
> +    | _filter_testdir | _filter_imgfmt

inexistent it a nonexistent word :)  But it correctly tests the problem.

> +++ b/tests/qemu-iotests/111.out
> @@ -0,0 +1,3 @@
> +QA output created by 111
> +qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent': No such file or directory

I'm okay whether you leave the test as-is, or s/in/non/ in two places.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for non-existing backing file

[Max Reitz]

On 2014-10-27 at 18:14, Eric Blake wrote:
> On 10/27/2014 06:30 AM, Max Reitz wrote:
>> Test the error message when a COW file is about to be created which is
>> supposed to inherit the size of its backing file, while the backing file
>> given does not actually exist.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> +
>> +$QEMU_IMG create -f $IMGFMT -b "$TEST_IMG.inexistent" "$TEST_IMG" 2>&1 \
>> +    | _filter_testdir | _filter_imgfmt
> inexistent it a nonexistent word :)  But it correctly tests the problem.

I was tempted to use .nai, which would be "does not exist" in Japanese, 
but then I went for http://en.wiktionary.org/wiki/inexistent :-P

I'd be fine with whatever.

Max

>> +++ b/tests/qemu-iotests/111.out
>> @@ -0,0 +1,3 @@
>> +QA output created by 111
>> +qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent': No such file or directory
> I'm okay whether you leave the test as-is, or s/in/non/ in two places.

Re: [Qemu-devel] [PATCH 0/2] block: Propagate error in bdrv_img_create()

[Kevin Wolf]

Am 27.10.2014 um 13:30 hat Max Reitz geschrieben:
> Currently, when trying to create a backed image without specifying its
> size, when the backing file does not exist or is not accessible, an
> appropriate error message will be generated which is then (in
> bdrv_img_create()) prefixed with the image file name and the strerror().
> However, both are generally already part of the bdrv_open() error
> message, so we should not double this information. An example:
> 
> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> qemu-img: /tmp/img.qcow2: Could not open '/tmp/enoent': Could not open
> '/tmp/enoent': No such file or directory: No such file or directory
> 
> Just propagating the error is sufficient:
> 
> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> qemu-img /tmp/img.qcow2: Could not open '/tmp/enoent': No such file or
> directory

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] block: Propagate error in bdrv_img_create()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Mon, Oct 27, 2014 at 01:30:07PM +0100, Max Reitz wrote:
> Currently, when trying to create a backed image without specifying its
> size, when the backing file does not exist or is not accessible, an
> appropriate error message will be generated which is then (in
> bdrv_img_create()) prefixed with the image file name and the strerror().
> However, both are generally already part of the bdrv_open() error
> message, so we should not double this information. An example:
> 
> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> qemu-img: /tmp/img.qcow2: Could not open '/tmp/enoent': Could not open
> '/tmp/enoent': No such file or directory: No such file or directory
> 
> Just propagating the error is sufficient:
> 
> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> qemu-img /tmp/img.qcow2: Could not open '/tmp/enoent': No such file or
> directory
> 
> 
> Max Reitz (2):
>   block: Propagate error in bdrv_img_create()
>   iotests: Add test for non-existing backing file
> 
>  block.c                    |  5 -----
>  tests/qemu-iotests/111     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/111.out |  3 +++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 57 insertions(+), 5 deletions(-)
>  create mode 100755 tests/qemu-iotests/111
>  create mode 100644 tests/qemu-iotests/111.out

Eric: Leaving "inexistent".  It is in several dictionaries besides
Wiktionary :).

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/2] block: Propagate error in bdrv_img_create()

[Max Reitz]

On 2014-10-29 at 11:42, Stefan Hajnoczi wrote:
> On Mon, Oct 27, 2014 at 01:30:07PM +0100, Max Reitz wrote:
>> Currently, when trying to create a backed image without specifying its
>> size, when the backing file does not exist or is not accessible, an
>> appropriate error message will be generated which is then (in
>> bdrv_img_create()) prefixed with the image file name and the strerror().
>> However, both are generally already part of the bdrv_open() error
>> message, so we should not double this information. An example:
>>
>> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
>> qemu-img: /tmp/img.qcow2: Could not open '/tmp/enoent': Could not open
>> '/tmp/enoent': No such file or directory: No such file or directory
>>
>> Just propagating the error is sufficient:
>>
>> $ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
>> qemu-img /tmp/img.qcow2: Could not open '/tmp/enoent': No such file or
>> directory
>>
>>
>> Max Reitz (2):
>>    block: Propagate error in bdrv_img_create()
>>    iotests: Add test for non-existing backing file
>>
>>   block.c                    |  5 -----
>>   tests/qemu-iotests/111     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/111.out |  3 +++
>>   tests/qemu-iotests/group   |  1 +
>>   4 files changed, 57 insertions(+), 5 deletions(-)
>>   create mode 100755 tests/qemu-iotests/111
>>   create mode 100644 tests/qemu-iotests/111.out
> Eric: Leaving "inexistent".  It is in several dictionaries besides
> Wiktionary :).
>
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block

Did you really? It appears neither there nor did it in your pull request...

Max

Re: [Qemu-devel] [PATCH 0/2] block: Propagate error in bdrv_img_create()

[Kevin Wolf]

Am 03.11.2014 um 13:33 hat Max Reitz geschrieben:
> On 2014-10-29 at 11:42, Stefan Hajnoczi wrote:
> >On Mon, Oct 27, 2014 at 01:30:07PM +0100, Max Reitz wrote:
> >>Currently, when trying to create a backed image without specifying its
> >>size, when the backing file does not exist or is not accessible, an
> >>appropriate error message will be generated which is then (in
> >>bdrv_img_create()) prefixed with the image file name and the strerror().
> >>However, both are generally already part of the bdrv_open() error
> >>message, so we should not double this information. An example:
> >>
> >>$ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> >>qemu-img: /tmp/img.qcow2: Could not open '/tmp/enoent': Could not open
> >>'/tmp/enoent': No such file or directory: No such file or directory
> >>
> >>Just propagating the error is sufficient:
> >>
> >>$ qemu-img create -f qcow2 -b /tmp/enoent /tmp/img.qcow2
> >>qemu-img /tmp/img.qcow2: Could not open '/tmp/enoent': No such file or
> >>directory
> >>
> >>
> >>Max Reitz (2):
> >>   block: Propagate error in bdrv_img_create()
> >>   iotests: Add test for non-existing backing file
> >>
> >>  block.c                    |  5 -----
> >>  tests/qemu-iotests/111     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/qemu-iotests/111.out |  3 +++
> >>  tests/qemu-iotests/group   |  1 +
> >>  4 files changed, 57 insertions(+), 5 deletions(-)
> >>  create mode 100755 tests/qemu-iotests/111
> >>  create mode 100644 tests/qemu-iotests/111.out
> >Eric: Leaving "inexistent".  It is in several dictionaries besides
> >Wiktionary :).
> >
> >Thanks, applied to my block tree:
> >https://github.com/stefanha/qemu/commits/block
> 
> Did you really? It appears neither there nor did it in your pull request...

Looks like it's missing indeed. Applied to my tree then.

Kevin