[Qemu-devel] [PATCH 00/12] block: Various Coverity-spotted fixes

[Qemu-devel] [PATCH 00/12] block: Various Coverity-spotted fixes

[Max Reitz]

This series fixes various issues spotted by Coverity. None of these is
critical; most are just "If you do something crazy, qemu-img crashes" or
"But what if there is no qcow2 driver?". Therefore, while these are bug
fixes, it is a bit late to try to push them into 2.2.0. I am therefore
tempted to vote to target 2.3 instead.

Also, none is security-relevant. The only crashes which are fixed here
are sure to have resulted from dereferencing a NULL pointer.


Max Reitz (12):
  block: qcow2 driver may not be found
  block/vvfat: qcow driver may not be found
  block/nfs: Add create_opts
  block: Check create_opts before image creation
  qemu-img: Check create_opts before image creation
  qemu-img: Check create_opts before image amendment
  iotests: Only kill NBD server if it runs
  iotests: Add test for unsupported image creation
  qcow2: Prevent numerical overflow
  qcow2: Flushing the caches in qcow2_close may fail
  qcow2: Respect bdrv_truncate() error
  block/raw-posix: Fix ret in raw_open_common()

 block.c                      | 18 +++++++++++
 block/nfs.c                  | 15 +++++++++
 block/qcow2-cluster.c        |  2 +-
 block/qcow2.c                | 22 ++++++++++---
 block/raw-posix.c            |  1 +
 block/vvfat.c                |  6 ++++
 qemu-img.c                   | 21 ++++++++++++
 tests/qemu-iotests/113       | 76 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/113.out   | 15 +++++++++
 tests/qemu-iotests/common.rc |  4 ++-
 tests/qemu-iotests/group     |  1 +
 11 files changed, 174 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/113
 create mode 100644 tests/qemu-iotests/113.out

-- 
1.9.3

[Qemu-devel] [PATCH 01/12] block: qcow2 driver may not be found

[Max Reitz]

Albeit absolutely impossible right now, bdrv_find_format("qcow2") may
fail. bdrv_append_temp_snapshot() should heed that case.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block.c b/block.c
index 866c8b4..b31fb67 100644
--- a/block.c
+++ b/block.c
@@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
     }
 
     bdrv_qcow2 = bdrv_find_format("qcow2");
+    if (!bdrv_qcow2) {
+        error_setg(errp, "Failed to locate qcow2 driver");
+        ret = -ENOENT;
+        goto out;
+    }
+
     opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0,
                             &error_abort);
     qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
-- 
1.9.3

[Qemu-devel] [PATCH 02/12] block/vvfat: qcow driver may not be found

[Max Reitz]

Although virtually impossible right now, bdrv_find_format("qcow") may
fail. The vvfat block driver should mind that case.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vvfat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index cefe3a4..e34a789 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2917,6 +2917,12 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
     }
 
     bdrv_qcow = bdrv_find_format("qcow");
+    if (!bdrv_qcow) {
+        error_setg(errp, "Failed to locate qcow driver");
+        ret = -ENOENT;
+        goto err;
+    }
+
     opts = qemu_opts_create(bdrv_qcow->create_opts, NULL, 0, &error_abort);
     qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s->sector_count * 512);
     qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:");
-- 
1.9.3

[Qemu-devel] [PATCH 03/12] block/nfs: Add create_opts

[Max Reitz]

The nfs protocol driver is capable of creating images, but did not
specify any creation options. Fix it.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nfs.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/nfs.c b/block/nfs.c
index c76e368..ca9e24e 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -409,6 +409,19 @@ out:
     return ret;
 }
 
+static QemuOptsList nfs_create_opts = {
+    .name = "nfs-create-opts",
+    .head = QTAILQ_HEAD_INITIALIZER(nfs_create_opts.head),
+    .desc = {
+        {
+            .name = BLOCK_OPT_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Virtual disk size"
+        },
+        { /* end of list */ }
+    }
+};
+
 static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp)
 {
     int ret = 0;
@@ -470,6 +483,8 @@ static BlockDriver bdrv_nfs = {
 
     .instance_size                  = sizeof(NFSClient),
     .bdrv_needs_filename            = true,
+    .create_opts                    = &nfs_create_opts,
+
     .bdrv_has_zero_init             = nfs_has_zero_init,
     .bdrv_get_allocated_file_size   = nfs_get_allocated_file_size,
     .bdrv_truncate                  = nfs_file_truncate,
-- 
1.9.3

[Qemu-devel] [PATCH 04/12] block: Check create_opts before image creation

[Max Reitz]

If a driver supports image creation, it needs to set the .create_opts
field. We can use that to make sure .create_opts for both drivers
involved is not NULL in bdrv_img_create(), which is important so that
the create_opts pointer in that function is not NULL after the
qemu_opts_append() calls and when going into qemu_opts_create().

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block.c b/block.c
index b31fb67..1e4a72f 100644
--- a/block.c
+++ b/block.c
@@ -5560,6 +5560,18 @@ void bdrv_img_create(const char *filename, const char *fmt,
         return;
     }
 
+    if (!drv->create_opts) {
+        error_setg(errp, "Format driver '%s' does not support image creation",
+                   drv->format_name);
+        return;
+    }
+
+    if (!proto_drv->create_opts) {
+        error_setg(errp, "Protocol driver '%s' does not support image creation",
+                   proto_drv->format_name);
+        return;
+    }
+
     create_opts = qemu_opts_append(create_opts, drv->create_opts);
     create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
 
-- 
1.9.3

[Qemu-devel] [PATCH 05/12] qemu-img: Check create_opts before image creation

[Max Reitz]

If a driver supports image creation, it needs to set the .create_opts
field. We can use that to make sure .create_opts for both drivers
involved is not NULL for the target image in qemu-img convert, which is
important so that the create_opts pointer in img_convert() is not NULL
after the qemu_opts_append() calls and when going into
qemu_opts_create().

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index a42335c..8c4edf3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1531,6 +1531,20 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
+    if (!drv->create_opts) {
+        error_report("Format driver '%s' does not support image creation",
+                     drv->format_name);
+        ret = -1;
+        goto out;
+    }
+
+    if (!proto_drv->create_opts) {
+        error_report("Protocol driver '%s' does not support image creation",
+                     proto_drv->format_name);
+        ret = -1;
+        goto out;
+    }
+
     create_opts = qemu_opts_append(create_opts, drv->create_opts);
     create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
 
-- 
1.9.3

[Qemu-devel] [PATCH 06/12] qemu-img: Check create_opts before image amendment

[Max Reitz]

The image options which can be amended are described by the .create_opts
field for every driver. This field must therefore be non-NULL so that
anything can be amended in the first place. Check that this holds true
before going into qemu_opts_create() (because if .create_opts is NULL,
the create_opts pointer in img_amend() will be NULL after
qemu_opts_append()).

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 8c4edf3..7876258 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2986,6 +2986,13 @@ static int img_amend(int argc, char **argv)
         goto out;
     }
 
+    if (!bs->drv->create_opts) {
+        error_report("Format driver '%s' does not support any options to amend",
+                     fmt);
+        ret = -1;
+        goto out;
+    }
+
     create_opts = qemu_opts_append(create_opts, bs->drv->create_opts);
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
     if (options && qemu_opts_do_parse(opts, options, NULL)) {
-- 
1.9.3

[Qemu-devel] [PATCH 07/12] iotests: Only kill NBD server if it runs

[Max Reitz]

There may be NBD tests which do not create a sample image and simply
test whether wrong usage of the protocol is rejected as expected. In
this case, there will be no NBD server and trying to kill it during
clean-up will fail.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.rc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9c49deb..f2554ec 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -175,7 +175,9 @@ _cleanup_test_img()
     case "$IMGPROTO" in
 
         nbd)
-            kill $QEMU_NBD_PID
+            if [ -n "$QEMU_NBD_PID" ]; then
+                kill $QEMU_NBD_PID
+            fi
             rm -f "$TEST_IMG_FILE"
             ;;
         file)
-- 
1.9.3

[Qemu-devel] [PATCH 08/12] iotests: Add test for unsupported image creation

[Max Reitz]

Add a test for creating and amending images (amendment uses the creation
options) with formats not supporting creation over protocols not
supporting creation.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/113     | 76 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/113.out | 15 +++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 92 insertions(+)
 create mode 100755 tests/qemu-iotests/113
 create mode 100644 tests/qemu-iotests/113.out

diff --git a/tests/qemu-iotests/113 b/tests/qemu-iotests/113
new file mode 100755
index 0000000..a2cd96b
--- /dev/null
+++ b/tests/qemu-iotests/113
@@ -0,0 +1,76 @@
+#!/bin/bash
+#
+# Test case for accessing creation options on image formats and
+# protocols not supporting image creation
+#
+# 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
+
+# We can only test one format here because we need its sample file
+_supported_fmt bochs
+_supported_proto nbd
+_supported_os Linux
+
+echo
+echo '=== Unsupported image creation in qemu-img create ==='
+echo
+
+$QEMU_IMG create -f $IMGFMT nbd://example.com 2>&1 64M | _filter_imgfmt
+
+echo
+echo '=== Unsupported image creation in qemu-img convert ==='
+echo
+
+# We could use any input image format here, but this is a bochs test, so just
+# use the bochs image
+_use_sample_img empty.bochs.bz2
+$QEMU_IMG convert -f $IMGFMT -O $IMGFMT "$TEST_IMG" nbd://example.com 2>&1 \
+    | _filter_imgfmt
+
+echo
+echo '=== Unsupported format in qemu-img amend ==='
+echo
+
+# The protocol does not matter here
+_use_sample_img empty.bochs.bz2
+$QEMU_IMG amend -f $IMGFMT -o foo=bar "$TEST_IMG" 2>&1 | _filter_imgfmt
+
+
+# success, all done
+echo
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/113.out b/tests/qemu-iotests/113.out
new file mode 100644
index 0000000..00bdfd6
--- /dev/null
+++ b/tests/qemu-iotests/113.out
@@ -0,0 +1,15 @@
+QA output created by 113
+
+=== Unsupported image creation in qemu-img create ===
+
+qemu-img: nbd://example.com: Format driver 'IMGFMT' does not support image creation
+
+=== Unsupported image creation in qemu-img convert ===
+
+qemu-img: Format driver 'IMGFMT' does not support image creation
+
+=== Unsupported format in qemu-img amend ===
+
+qemu-img: Format driver 'IMGFMT' does not support any options to amend
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7dfe469..fd2c64a 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
+113 rw auto quick
-- 
1.9.3

[Qemu-devel] [PATCH 09/12] qcow2: Prevent numerical overflow

[Max Reitz]

In qcow2_alloc_cluster_offset(), *num is limited to
INT_MAX >> BDRV_SECTOR_BITS by all callers. However, since remaining is
of type uint64_t, we might as well cast *num to that type before
performing the shift.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index df0b2c9..1fea514 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1263,7 +1263,7 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
 
 again:
     start = offset;
-    remaining = *num << BDRV_SECTOR_BITS;
+    remaining = (uint64_t)*num << BDRV_SECTOR_BITS;
     cluster_offset = 0;
     *host_offset = 0;
     cur_bytes = 0;
-- 
1.9.3

[Qemu-devel] [PATCH 10/12] qcow2: Flushing the caches in qcow2_close may fail

[Max Reitz]

qcow2_cache_flush() may fail; if one of the caches failed to be flushed
successfully to disk in qcow2_close() the image should not be marked
clean, and we should emit a warning.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d120494..2bd2a53 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1428,10 +1428,23 @@ static void qcow2_close(BlockDriverState *bs)
     s->l1_table = NULL;
 
     if (!(bs->open_flags & BDRV_O_INCOMING)) {
-        qcow2_cache_flush(bs, s->l2_table_cache);
-        qcow2_cache_flush(bs, s->refcount_block_cache);
+        int ret1, ret2;
 
-        qcow2_mark_clean(bs);
+        ret1 = qcow2_cache_flush(bs, s->l2_table_cache);
+        ret2 = qcow2_cache_flush(bs, s->refcount_block_cache);
+
+        if (ret1) {
+            error_report("Failed to flush the L2 table cache: %s",
+                         strerror(-ret1));
+        }
+        if (ret2) {
+            error_report("Failed to flush the refcount block cache: %s",
+                         strerror(-ret2));
+        }
+
+        if (!ret1 && !ret2) {
+            qcow2_mark_clean(bs);
+        }
     }
 
     qcow2_cache_destroy(bs, s->l2_table_cache);
-- 
1.9.3

[Qemu-devel] [PATCH 11/12] qcow2: Respect bdrv_truncate() error

[Max Reitz]

bdrv_truncate() may fail and qcow2_write_compressed() should return the
error code in that case.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2bd2a53..691f363 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2163,8 +2163,7 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
         /* align end of file to a sector boundary to ease reading with
            sector based I/Os */
         cluster_offset = bdrv_getlength(bs->file);
-        bdrv_truncate(bs->file, cluster_offset);
-        return 0;
+        return bdrv_truncate(bs->file, cluster_offset);
     }
 
     if (nb_sectors != s->cluster_sectors) {
-- 
1.9.3

[Qemu-devel] [PATCH 12/12] block/raw-posix: Fix ret in raw_open_common()

[Max Reitz]

The return value must be negative on error; there is one place in
raw_open_common() where errp is set, but ret remains 0. Fix it.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/raw-posix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index b1af77e..96491fc 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -446,6 +446,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
 
     if (fstat(s->fd, &st) < 0) {
+        ret = -errno;
         error_setg_errno(errp, errno, "Could not stat file");
         goto fail;
     }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 10/12] qcow2: Flushing the caches in qcow2_close may fail

[Max Reitz]

On 2014-11-25 at 15:08, Max Reitz wrote:
> qcow2_cache_flush() may fail; if one of the caches failed to be flushed
> successfully to disk in qcow2_close() the image should not be marked
> clean, and we should emit a warning.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/qcow2.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d120494..2bd2a53 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1428,10 +1428,23 @@ static void qcow2_close(BlockDriverState *bs)
>       s->l1_table = NULL;
>   
>       if (!(bs->open_flags & BDRV_O_INCOMING)) {
> -        qcow2_cache_flush(bs, s->l2_table_cache);
> -        qcow2_cache_flush(bs, s->refcount_block_cache);
> +        int ret1, ret2;
>   
> -        qcow2_mark_clean(bs);
> +        ret1 = qcow2_cache_flush(bs, s->l2_table_cache);
> +        ret2 = qcow2_cache_flush(bs, s->refcount_block_cache);
> +
> +        if (ret1) {
> +            error_report("Failed to flush the L2 table cache: %s",
> +                         strerror(-ret1));
> +        }
> +        if (ret2) {
> +            error_report("Failed to flush the refcount block cache: %s",
> +                         strerror(-ret2));
> +        }
> +
> +        if (!ret1 && !ret2) {
> +            qcow2_mark_clean(bs);
> +        }
>       }
>   
>       qcow2_cache_destroy(bs, s->l2_table_cache);

I just noticed this breaks 026, 071 and 089, due to lots of additional 
"Failed to flush the .* cache" lines. I can either go the easy way and 
remove the error_report() calls from this cache; or I can amend the test 
outputs. I'm not sure what I prefer, because I'm not sure whether having 
the output is actually useful. Any thoughts from you?

Max

Re: [Qemu-devel] [PATCH 03/12] block/nfs: Add create_opts

[Kevin Wolf]

Am 25.11.2014 um 15:07 hat Max Reitz geschrieben:
> The nfs protocol driver is capable of creating images, but did not
> specify any creation options. Fix it.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>

As you'll respin anyway: Can you mention how this would fail? (I suppose
some segfault when trying to access the options?)

Kevin

Re: [Qemu-devel] [PATCH 10/12] qcow2: Flushing the caches in qcow2_close may fail

[Kevin Wolf]

Am 25.11.2014 um 15:22 hat Max Reitz geschrieben:
> On 2014-11-25 at 15:08, Max Reitz wrote:
> >qcow2_cache_flush() may fail; if one of the caches failed to be flushed
> >successfully to disk in qcow2_close() the image should not be marked
> >clean, and we should emit a warning.
> >
> >Cc: qemu-stable@nongnu.org
> >Signed-off-by: Max Reitz <mreitz@redhat.com>
> >---
> >  block/qcow2.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index d120494..2bd2a53 100644
> >--- a/block/qcow2.c
> >+++ b/block/qcow2.c
> >@@ -1428,10 +1428,23 @@ static void qcow2_close(BlockDriverState *bs)
> >      s->l1_table = NULL;
> >      if (!(bs->open_flags & BDRV_O_INCOMING)) {
> >-        qcow2_cache_flush(bs, s->l2_table_cache);
> >-        qcow2_cache_flush(bs, s->refcount_block_cache);
> >+        int ret1, ret2;
> >-        qcow2_mark_clean(bs);
> >+        ret1 = qcow2_cache_flush(bs, s->l2_table_cache);
> >+        ret2 = qcow2_cache_flush(bs, s->refcount_block_cache);
> >+
> >+        if (ret1) {
> >+            error_report("Failed to flush the L2 table cache: %s",
> >+                         strerror(-ret1));
> >+        }
> >+        if (ret2) {
> >+            error_report("Failed to flush the refcount block cache: %s",
> >+                         strerror(-ret2));
> >+        }
> >+
> >+        if (!ret1 && !ret2) {
> >+            qcow2_mark_clean(bs);
> >+        }
> >      }
> >      qcow2_cache_destroy(bs, s->l2_table_cache);
> 
> I just noticed this breaks 026, 071 and 089, due to lots of
> additional "Failed to flush the .* cache" lines. I can either go the
> easy way and remove the error_report() calls from this cache; or I
> can amend the test outputs. I'm not sure what I prefer, because I'm
> not sure whether having the output is actually useful. Any thoughts
> from you?

I think I have a slight preference for keeping the error messages and
updating the reference output.

Kevin

Re: [Qemu-devel] [PATCH 03/12] block/nfs: Add create_opts

[Max Reitz]

On 2014-11-25 at 15:41, Kevin Wolf wrote:
> Am 25.11.2014 um 15:07 hat Max Reitz geschrieben:
>> The nfs protocol driver is capable of creating images, but did not
>> specify any creation options. Fix it.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> As you'll respin anyway: Can you mention how this would fail? (I suppose
> some segfault when trying to access the options?)

I'd have to try, but I guess it actually doesn't fail in most cases 
because the size option is part of the format drivers options anyway. I 
can imagine something being broken when doing preallocation in qcow2, 
though, which makes the qcow2 reset the size option.

Max

Re: [Qemu-devel] [PATCH 01/12] block: qcow2 driver may not be found

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Albeit absolutely impossible right now, bdrv_find_format("qcow2") may
> fail. bdrv_append_temp_snapshot() should heed that case.

Impossible because we always compile in bdrv_qcow2.

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/block.c b/block.c
> index 866c8b4..b31fb67 100644
> --- a/block.c
> +++ b/block.c
> @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>      }
>  
>      bdrv_qcow2 = bdrv_find_format("qcow2");
> +    if (!bdrv_qcow2) {
> +        error_setg(errp, "Failed to locate qcow2 driver");
> +        ret = -ENOENT;
> +        goto out;
> +    }
> +
>      opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0,
>                              &error_abort);
>      qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);

This dynamic qcow2 driver lookup business is silly.  Compiling without
qcow2 would be a massive loss of functionality, and I wouldn't bet a
nickel on the error paths it would pring to live.

Even sillier lookups: "file" in bdrv_find_protocol(), and "raw" in
find_image_format().

Statically linking to them would be simpler and more honest.

Aside: similar silliness exists around QemuOpts.

Patch looks correct.

Re: [Qemu-devel] [PATCH 01/12] block: qcow2 driver may not be found

[Max Reitz]

On 2014-11-26 at 08:23, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> Albeit absolutely impossible right now, bdrv_find_format("qcow2") may
>> fail. bdrv_append_temp_snapshot() should heed that case.
> Impossible because we always compile in bdrv_qcow2.

Right now we do, right.

>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 866c8b4..b31fb67 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>>       }
>>   
>>       bdrv_qcow2 = bdrv_find_format("qcow2");
>> +    if (!bdrv_qcow2) {
>> +        error_setg(errp, "Failed to locate qcow2 driver");
>> +        ret = -ENOENT;
>> +        goto out;
>> +    }
>> +
>>       opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0,
>>                               &error_abort);
>>       qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
> This dynamic qcow2 driver lookup business is silly.  Compiling without
> qcow2 would be a massive loss of functionality, and I wouldn't bet a
> nickel on the error paths it would pring to live.

True.

> Even sillier lookups: "file" in bdrv_find_protocol(), and "raw" in
> find_image_format().
>
> Statically linking to them would be simpler and more honest.
>
> Aside: similar silliness exists around QemuOpts.
>
> Patch looks correct.

Well, at least it will silence Coverity...

Max

Re: [Qemu-devel] [PATCH 01/12] block: qcow2 driver may not be found

[Eric Blake]

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

On 11/26/2014 02:13 AM, Max Reitz wrote:
> On 2014-11-26 at 08:23, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>>
>>> Albeit absolutely impossible right now, bdrv_find_format("qcow2") may
>>> fail. bdrv_append_temp_snapshot() should heed that case.
>> Impossible because we always compile in bdrv_qcow2.
> 

>>> +++ b/block.c
>>> @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState
>>> *bs, int flags, Error **errp)
>>>       }
>>>         bdrv_qcow2 = bdrv_find_format("qcow2");
>>> +    if (!bdrv_qcow2) {

Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity?

-- 
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 01/12] block: qcow2 driver may not be found

[Max Reitz]

On 2014-11-26 at 16:19, Eric Blake wrote:
> On 11/26/2014 02:13 AM, Max Reitz wrote:
>> On 2014-11-26 at 08:23, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> Albeit absolutely impossible right now, bdrv_find_format("qcow2") may
>>>> fail. bdrv_append_temp_snapshot() should heed that case.
>>> Impossible because we always compile in bdrv_qcow2.
>>>> +++ b/block.c
>>>> @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState
>>>> *bs, int flags, Error **errp)
>>>>        }
>>>>          bdrv_qcow2 = bdrv_find_format("qcow2");
>>>> +    if (!bdrv_qcow2) {
> Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity?

Sounds like a good compromise. Will do.

Max

Re: [Qemu-devel] [PATCH 01/12] block: qcow2 driver may not be found

[Kevin Wolf]

Am 26.11.2014 um 16:20 hat Max Reitz geschrieben:
> On 2014-11-26 at 16:19, Eric Blake wrote:
> >On 11/26/2014 02:13 AM, Max Reitz wrote:
> >>On 2014-11-26 at 08:23, Markus Armbruster wrote:
> >>>Max Reitz <mreitz@redhat.com> writes:
> >>>
> >>>>Albeit absolutely impossible right now, bdrv_find_format("qcow2") may
> >>>>fail. bdrv_append_temp_snapshot() should heed that case.
> >>>Impossible because we always compile in bdrv_qcow2.
> >>>>+++ b/block.c
> >>>>@@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState
> >>>>*bs, int flags, Error **errp)
> >>>>       }
> >>>>         bdrv_qcow2 = bdrv_find_format("qcow2");
> >>>>+    if (!bdrv_qcow2) {
> >Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity?
> 
> Sounds like a good compromise. Will do.

I think it's better to have either proper error handling for the case
that someone compiles it out, like implemented by this patch, or to
reference the symbol so that compiling it out already breaks the build.
The assert() would potentially be a crash of a running VM, which is not
as nice.

Kevin

Re: [Qemu-devel] [PATCH 01/12] block: qcow2 driver may not be found

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.11.2014 um 16:20 hat Max Reitz geschrieben:
>> On 2014-11-26 at 16:19, Eric Blake wrote:
>> >On 11/26/2014 02:13 AM, Max Reitz wrote:
>> >>On 2014-11-26 at 08:23, Markus Armbruster wrote:
>> >>>Max Reitz <mreitz@redhat.com> writes:
>> >>>
>> >>>>Albeit absolutely impossible right now, bdrv_find_format("qcow2") may
>> >>>>fail. bdrv_append_temp_snapshot() should heed that case.
>> >>>Impossible because we always compile in bdrv_qcow2.
>> >>>>+++ b/block.c
>> >>>>@@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState
>> >>>>*bs, int flags, Error **errp)
>> >>>>       }
>> >>>>         bdrv_qcow2 = bdrv_find_format("qcow2");
>> >>>>+    if (!bdrv_qcow2) {
>> >Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity?
>> 
>> Sounds like a good compromise. Will do.
>
> I think it's better to have either proper error handling for the case
> that someone compiles it out, like implemented by this patch, or to
> reference the symbol so that compiling it out already breaks the build.
> The assert() would potentially be a crash of a running VM, which is not
> as nice.

Ways to bind to a well-known block driver, and how the binding fails
when the driver isn't around, in decreasing order of preference:

1. Static binding

   Fails at compile time.  Me like.

2. Dynamic binding, assert the required code is there

   Fails at run time in a catastrophic, but locally obvious way.

3. Dynamic binding without error checking

   Fails at run time in a catastrophic way, when the pointer gets
   dereferenced.  Less predictable than 2.

4. Dynamic binding, error out if the required code is there

   Fails at run time in a way that could be recoverable, but probably
   isn't, not least because nobody ever tests it.  Moreover, code
   clutter.

Use of dynamic binding to reference a well-known block driver feels like
"Look ma, dynamic binding in C!" to me.

Re: [Qemu-devel] [PATCH 03/12] block/nfs: Add create_opts

[Max Reitz]

On 2014-11-25 at 15:49, Max Reitz wrote:
> On 2014-11-25 at 15:41, Kevin Wolf wrote:
>> Am 25.11.2014 um 15:07 hat Max Reitz geschrieben:
>>> The nfs protocol driver is capable of creating images, but did not
>>> specify any creation options. Fix it.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> As you'll respin anyway: Can you mention how this would fail? (I suppose
>> some segfault when trying to access the options?)
>
> I'd have to try, but I guess it actually doesn't fail in most cases 
> because the size option is part of the format drivers options anyway. 
> I can imagine something being broken when doing preallocation in 
> qcow2, though, which makes the qcow2 reset the size option.

Indeed it doesn't fail because the size option is part of the format 
driver. The interesting thing is that all format block drivers take the 
QemuOpts object which is basically the original options QDict absorbed 
into their *and the protocol's* creation options QemuOptsList (with the 
size option set etc. pp.), then take out the options they understand and 
pass along the rest to the protocol driver.

This leads to the funny constellation that if a format driver does not 
understand some option, it's passed on (you can use preallocation=full 
with any format block driver, as long as the protocol is raw-posix), but 
if it does, it's not (if you use qcow2, the protocol block driver will 
only be given the preallocation option if qcow2 was so kind and set it 
itself).

It seems too intentional to me to call it a bug...

However, as for this patch, I could indeed produce a failure case:

$ qemu-img create -f nfs nfs://127.0.0.1/foo.qcow2 64M
[1]    14582 segmentation fault (core dumped)

Maybe we want qemu-img to check whether the format given through -f or 
-O is indeed a format... But for now, all the better for me as it gives 
me something which I'm fixing. I won't make this an iotest, though, 
because -f $protocol is not something that should actually work.

Max