[Qemu-devel] [PATCH 0/8] blockdev: (Nearly) free clean-up work

[Qemu-devel] [PATCH 0/8] blockdev: (Nearly) free clean-up work

[Max Reitz]

After a lot has been restructed in the block layer in the past, we can
now reap at least one of the fruits: Make bdrv_open() return a BDS!

I tried to squeeze patch 8 of this series into my bdrv_close_all()
series but noticed that won't work because bdrv_open_inherit() still has
one instance of bdrv_close() which is actually pretty reasonable.
Therefore, that was impossible there.

However, that instance exists only because bdrv_open_inherit() does not
always create a new BDS but sometimes takes an existing one and opens a
file "into" that BDS, basically. That is ugly and we've tried to get rid
of that for a long time now. Then I noticed that nearly all the callers
of bdrv_open()/bdrv_open_inherit() no longer need that feature, there
was only one show-stopper left: blk_new_open(). That functions creates a
BDS using blk_new_with_bs(), which in turn uses bdrv_new_root() instead
of a plain bdrv_new().

However, the sole difference between bdrv_new_root() and bdrv_new() is
gone as of my "Further BlockBackend work" series, so we can make
blk_new_open() reuse the BDS creation capabilities of bdrv_open() as
well. And that's it!

And with bdv_open_inherit() always creating the BDS, we no longer need
to call bdrv_close() there, which allows us to assert that any BDS given
to bdrv_close() will have a refcount of 0 (i.e. is about to be deleted).


As hinted before, this series depends on v2 of my previous series
"blockdev: Further BlockBackend work".


Max Reitz (8):
  qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE
  block: Drop useless bdrv_new() calls
  block: Let bdrv_open_inherit() return the snapshot
  block: Drop BB name from bad option error
  block: Drop blk_new_with_bs()
  block: Drop bdrv_new_root()
  block: Make bdrv_open() return a BDS
  block: Assert !bs->refcnt in bdrv_close()

 block.c                    | 139 ++++++++++++++++++---------------------------
 block/block-backend.c      |  31 +++-------
 block/parallels.c          |   9 +--
 block/qcow.c               |  15 ++---
 block/qcow2.c              |  53 ++++++-----------
 block/qed.c                |  18 ++----
 block/sheepdog.c           |  16 +++---
 block/vdi.c                |   7 +--
 block/vhdx.c               |   8 +--
 block/vmdk.c               |  31 +++++-----
 block/vpc.c                |   7 +--
 block/vvfat.c              |   9 ++-
 blockdev.c                 |  38 +++++--------
 include/block/block.h      |   6 +-
 include/qapi/qmp/qerror.h  |   3 -
 tests/qemu-iotests/036.out |  16 +++---
 tests/qemu-iotests/051.out |   8 +--
 17 files changed, 153 insertions(+), 261 deletions(-)

-- 
2.6.2

[Qemu-devel] [PATCH 1/8] qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE

[Max Reitz]

Just specifying a custom string is simpler in basically all places that
used it, and in addition, specifying the BB or node name is something we
generally do not do in other error messages when opening a BDS, so we
should not do it here.

This changes the output for iotest 036 (to the better, in my opinion),
so the reference output needs to be changed accordingly.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow.c               |  6 +-----
 block/qcow2.c              | 24 +++++-------------------
 block/qed.c                |  7 ++-----
 block/vmdk.c               |  7 ++-----
 include/qapi/qmp/qerror.h  |  3 ---
 tests/qemu-iotests/036.out | 16 ++++++++--------
 6 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 635085e..2601954 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -119,11 +119,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
     if (header.version != QCOW_VERSION) {
-        char version[64];
-        snprintf(version, sizeof(version), "QCOW version %" PRIu32,
-                 header.version);
-        error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                   bdrv_get_device_or_node_name(bs), "qcow", version);
+        error_setg(errp, "Unsupported qcow version %" PRIu32, header.version);
         ret = -ENOTSUP;
         goto fail;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 88f56c8..88e17aa 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -196,22 +196,8 @@ static void cleanup_unknown_header_ext(BlockDriverState *bs)
     }
 }
 
-static void GCC_FMT_ATTR(3, 4) report_unsupported(BlockDriverState *bs,
-    Error **errp, const char *fmt, ...)
-{
-    char msg[64];
-    va_list ap;
-
-    va_start(ap, fmt);
-    vsnprintf(msg, sizeof(msg), fmt, ap);
-    va_end(ap);
-
-    error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-               bdrv_get_device_or_node_name(bs), "qcow2", msg);
-}
-
-static void report_unsupported_feature(BlockDriverState *bs,
-    Error **errp, Qcow2Feature *table, uint64_t mask)
+static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
+                                       uint64_t mask)
 {
     char *features = g_strdup("");
     char *old;
@@ -236,7 +222,7 @@ static void report_unsupported_feature(BlockDriverState *bs,
         g_free(old);
     }
 
-    report_unsupported(bs, errp, "%s", features);
+    error_setg(errp, "Unsupported qcow2 feature(s): %s", features);
     g_free(features);
 }
 
@@ -853,7 +839,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
     if (header.version < 2 || header.version > 3) {
-        report_unsupported(bs, errp, "QCOW version %" PRIu32, header.version);
+        error_setg(errp, "Unsupported qcow2 version %" PRIu32, header.version);
         ret = -ENOTSUP;
         goto fail;
     }
@@ -933,7 +919,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         void *feature_table = NULL;
         qcow2_read_extensions(bs, header.header_length, ext_end,
                               &feature_table, NULL);
-        report_unsupported_feature(bs, errp, feature_table,
+        report_unsupported_feature(errp, feature_table,
                                    s->incompatible_features &
                                    ~QCOW2_INCOMPAT_MASK);
         ret = -ENOTSUP;
diff --git a/block/qed.c b/block/qed.c
index 5ea05d4..c15f0be 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -398,11 +398,8 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
     }
     if (s->header.features & ~QED_FEATURE_MASK) {
         /* image uses unsupported feature bits */
-        char buf[64];
-        snprintf(buf, sizeof(buf), "%" PRIx64,
-            s->header.features & ~QED_FEATURE_MASK);
-        error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                   bdrv_get_device_or_node_name(bs), "QED", buf);
+        error_setg(errp, "Unsupported QED features: %" PRIx64,
+                   s->header.features & ~QED_FEATURE_MASK);
         return -ENOTSUP;
     }
     if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
diff --git a/block/vmdk.c b/block/vmdk.c
index 6f819e4..bcce9a7 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -645,11 +645,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     }
 
     if (le32_to_cpu(header.version) > 3) {
-        char buf[64];
-        snprintf(buf, sizeof(buf), "VMDK version %" PRId32,
-                 le32_to_cpu(header.version));
-        error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                   bdrv_get_device_or_node_name(bs), "vmdk", buf);
+        error_setg(errp, "Unsupported VMDK version %" PRIu32,
+                   le32_to_cpu(header.version));
         return -ENOTSUP;
     } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) {
         /* VMware KB 2064959 explains that version 3 added support for
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index f601499..d08652a 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -100,9 +100,6 @@
 #define QERR_UNDEFINED_ERROR \
     "An undefined error has occurred"
 
-#define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
-    "'%s' uses a %s feature which is not supported by this qemu version: %s"
-
 #define QERR_UNSUPPORTED \
     "this feature or command is not currently supported"
 
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index 5616e37..73856f5 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -22,18 +22,18 @@ autoclear_features        0x0
 refcount_order            4
 header_length             104
 
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Unknown incompatible feature: 8000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Test feature
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Test feature
 
 === Image with multiple incompatible feature bits ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Unknown incompatible feature: e000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Test feature, Unknown incompatible feature: 6000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Test feature, Unknown incompatible feature: c000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: test1, test2, Unknown incompatible feature: 8000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: test1, test2, test3
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: test2, Unknown incompatible feature: a000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: e000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Test feature, Unknown incompatible feature: 6000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Test feature, Unknown incompatible feature: c000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): test1, test2, Unknown incompatible feature: 8000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): test1, test2, test3
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): test2, Unknown incompatible feature: a000000000000000
 === Create image with unknown autoclear feature bit ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.6.2

Re: [Qemu-devel] [PATCH 1/8] qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE

[Eric Blake]

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

On 11/09/2015 08:44 PM, Max Reitz wrote:
> Just specifying a custom string is simpler in basically all places that
> used it, and in addition, specifying the BB or node name is something we
> generally do not do in other error messages when opening a BDS, so we
> should not do it here.
> 
> This changes the output for iotest 036 (to the better, in my opinion),

s/to/for/ to make the idiomatic expression sound correct to a native speaker

> so the reference output needs to be changed accordingly.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow.c               |  6 +-----
>  block/qcow2.c              | 24 +++++-------------------
>  block/qed.c                |  7 ++-----
>  block/vmdk.c               |  7 ++-----
>  include/qapi/qmp/qerror.h  |  3 ---
>  tests/qemu-iotests/036.out | 16 ++++++++--------
>  6 files changed, 18 insertions(+), 45 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/8] qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:16 AM CET, Max Reitz <mreitz@redhat.com> wrote:
> Just specifying a custom string is simpler in basically all places that
> used it, and in addition, specifying the BB or node name is something we
> generally do not do in other error messages when opening a BDS, so we
> should not do it here.
>
> This changes the output for iotest 036 (to the better, in my opinion),
> so the reference output needs to be changed accordingly.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 2/8] block: Drop useless bdrv_new() calls

[Max Reitz]

bdrv_append_temp_snapshot() uses bdrv_new() to create an empty BDS
before invoking bdrv_open() on that BDS. This is probably a relict from
when it used to do some modifications on that empty BDS, but now that is
unnecessary, so we can just set bs_snapshot to NULL and let bdrv_open()
do the rest.

The same applies to bdrv_open_backing_file(). There is even a bit more
cruft here: The assert() was intended for the BDS which is indirectly
passed as the first bdrv_open() argument, so we can now drop it, too.

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

diff --git a/block.c b/block.c
index 5b02990..aae1d69 100644
--- a/block.c
+++ b/block.c
@@ -1212,19 +1212,15 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         goto free_exit;
     }
 
-    backing_hd = bdrv_new();
-
     if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
         qdict_put(options, "driver", qstring_from_str(bs->backing_format));
     }
 
-    assert(bs->backing == NULL);
+    backing_hd = NULL;
     ret = bdrv_open_inherit(&backing_hd,
                             *backing_filename ? backing_filename : NULL,
                             NULL, options, 0, bs, &child_backing, &local_err);
     if (ret < 0) {
-        bdrv_unref(backing_hd);
-        backing_hd = NULL;
         bs->open_flags |= BDRV_O_NO_BACKING;
         error_setg(errp, "Could not open backing file: %s",
                    error_get_pretty(local_err));
@@ -1350,8 +1346,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
     qdict_put(snapshot_options, "driver",
               qstring_from_str("qcow2"));
 
-    bs_snapshot = bdrv_new();
-
+    bs_snapshot = NULL;
     ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
                     flags, &local_err);
     if (ret < 0) {
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH 2/8] block: Drop useless bdrv_new() calls

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:17 AM CET, Max Reitz wrote:
> bdrv_append_temp_snapshot() uses bdrv_new() to create an empty BDS
> before invoking bdrv_open() on that BDS. This is probably a relict from
> when it used to do some modifications on that empty BDS, but now that is
> unnecessary, so we can just set bs_snapshot to NULL and let bdrv_open()
> do the rest.
>
> The same applies to bdrv_open_backing_file(). There is even a bit more
> cruft here: The assert() was intended for the BDS which is indirectly
> passed as the first bdrv_open() argument, so we can now drop it, too.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 3/8] block: Let bdrv_open_inherit() return the snapshot

[Max Reitz]

If bdrv_open_inherit() creates a snapshot BDS and *pbs is NULL, that
snapshot BDS should be returned instead of the BDS under it.

To this end, bdrv_append_temp_snapshot() now returns the snapshot BDS
instead of just appending it on top of the snapshotted BDS, and that
function is made static since bdrv_open_inherit() is its only user
anyway. Also, it calls bdrv_ref() before bdrv_append() (which
bdrv_open_inherit() has to undo if not returning the overlay).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 27 +++++++++++++++++++++++----
 include/block/block.h |  1 -
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index aae1d69..c5ea5e7 100644
--- a/block.c
+++ b/block.c
@@ -1295,7 +1295,8 @@ done:
     return c;
 }
 
-int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
+static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
+                                                   int flags, Error **errp)
 {
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char *tmp_filename = g_malloc0(PATH_MAX + 1);
@@ -1354,11 +1355,15 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
         goto out;
     }
 
+    bdrv_ref(bs_snapshot);
     bdrv_append(bs_snapshot, bs);
 
+    g_free(tmp_filename);
+    return bs_snapshot;
+
 out:
     g_free(tmp_filename);
-    return ret;
+    return NULL;
 }
 
 /*
@@ -1557,15 +1562,29 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
     }
 
     QDECREF(options);
-    *pbs = bs;
 
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
     if (snapshot_flags) {
-        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
+        BlockDriverState *snapshot_bs;
+        snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
+            ret = -EINVAL;
             goto close_and_fail;
         }
+        if (!*pbs) {
+            /* The reference is now held by the overlay BDS */
+            bdrv_unref(bs);
+            bs = snapshot_bs;
+        } else {
+            /* It is still referenced in the same way that *pbs was referenced,
+             * however that may be */
+            bdrv_unref(snapshot_bs);
+        }
+    }
+
+    if (!*pbs) {
+        *pbs = bs;
     }
 
     return 0;
diff --git a/include/block/block.h b/include/block/block.h
index 1abfc70..37e49a1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -210,7 +210,6 @@ BdrvChild *bdrv_open_child(const char *filename,
                            bool allow_none, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
 int bdrv_open(BlockDriverState **pbs, const char *filename,
               const char *reference, QDict *options, int flags, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH 3/8] block: Let bdrv_open_inherit() return the snapshot

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:18 AM CET, Max Reitz wrote:
> -int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
> +static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
> +                                                   int flags, Error **errp)
>  {
>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>      char *tmp_filename = g_malloc0(PATH_MAX + 1);
> @@ -1354,11 +1355,15 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>          goto out;
>      }
>  
> +    bdrv_ref(bs_snapshot);
>      bdrv_append(bs_snapshot, bs);
>  
> +    g_free(tmp_filename);
> +    return bs_snapshot;
> +
>  out:
>      g_free(tmp_filename);
> -    return ret;
> +    return NULL;
>  }

If I'm not wrong, now that you're not returning 'ret' anymore there's a
"ret = total_size" line earlier in this function that is useless now.

Other than that,

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 3/8] block: Let bdrv_open_inherit() return the snapshot

[Max Reitz]

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

On 01.12.2015 15:35, Alberto Garcia wrote:
> On Tue 10 Nov 2015 04:44:18 AM CET, Max Reitz wrote:
>> -int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>> +static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>> +                                                   int flags, Error **errp)
>>  {
>>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>>      char *tmp_filename = g_malloc0(PATH_MAX + 1);
>> @@ -1354,11 +1355,15 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>>          goto out;
>>      }
>>  
>> +    bdrv_ref(bs_snapshot);
>>      bdrv_append(bs_snapshot, bs);
>>  
>> +    g_free(tmp_filename);
>> +    return bs_snapshot;
>> +
>>  out:
>>      g_free(tmp_filename);
>> -    return ret;
>> +    return NULL;
>>  }
> 
> If I'm not wrong, now that you're not returning 'ret' anymore there's a
> "ret = total_size" line earlier in this function that is useless now.

Yes, indeed, thanks for finding that. Will fix.

Max

> Other than that,
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 



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

[Qemu-devel] [PATCH 4/8] block: Drop BB name from bad option error

[Max Reitz]

The information which BB is concerned does not seem useful enough to
justify its existence in most other place (which may be related to qemu
printing the -drive parameter in question anyway, and for blockdev-add
the attribution is naturally unambiguous). Furthermore, as of a future
patch, bdrv_get_device_name(bs) will always return the empty string
before bdrv_open_inherit() returns.

Therefore, just dropping that information seems to be the best course of
action.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                    | 6 +++---
 tests/qemu-iotests/051.out | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index c5ea5e7..395b7b0 100644
--- a/block.c
+++ b/block.c
@@ -1539,9 +1539,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
             error_setg(errp, "Block protocol '%s' doesn't support the option "
                        "'%s'", drv->format_name, entry->key);
         } else {
-            error_setg(errp, "Block format '%s' used by device '%s' doesn't "
-                       "support the option '%s'", drv->format_name,
-                       bdrv_get_device_name(bs), entry->key);
+            error_setg(errp,
+                       "Block format '%s' doesn't support the option '%s'",
+                       drv->format_name, entry->key);
         }
 
         ret = -EINVAL;
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 7765aa0..c6df65f 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -5,16 +5,16 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 === Unknown option ===
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=: Block format 'qcow2' used by device 'ide0-hd0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=: Block format 'qcow2' doesn't support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on: Block format 'qcow2' used by device 'ide0-hd0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on: Block format 'qcow2' doesn't support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234: Block format 'qcow2' used by device 'ide0-hd0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234: Block format 'qcow2' doesn't support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo: Block format 'qcow2' used by device 'ide0-hd0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo: Block format 'qcow2' doesn't support the option 'unknown_opt'
 
 
 === Unknown protocol option ===
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH 4/8] block: Drop BB name from bad option error

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:19 AM CET, Max Reitz wrote:
> The information which BB is concerned does not seem useful enough to
> justify its existence in most other place (which may be related to qemu
> printing the -drive parameter in question anyway, and for blockdev-add
> the attribution is naturally unambiguous). Furthermore, as of a future
> patch, bdrv_get_device_name(bs) will always return the empty string
> before bdrv_open_inherit() returns.
>
> Therefore, just dropping that information seems to be the best course of
> action.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 5/8] block: Drop blk_new_with_bs()

[Max Reitz]

Its only caller is blk_new_open(), so we can just inline it there. Since
bdrv_new_root() is only a wrapper around bdrv_new(), we can just use
bdrv_new() instead.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index b0bf3b2..13f5fef 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -114,26 +114,6 @@ BlockBackend *blk_new(const char *name, Error **errp)
 }
 
 /*
- * Create a new BlockBackend with a new BlockDriverState attached.
- * Otherwise just like blk_new(), which see.
- */
-BlockBackend *blk_new_with_bs(const char *name, Error **errp)
-{
-    BlockBackend *blk;
-    BlockDriverState *bs;
-
-    blk = blk_new(name, errp);
-    if (!blk) {
-        return NULL;
-    }
-
-    bs = bdrv_new_root();
-    blk->bs = bs;
-    bs->blk = blk;
-    return blk;
-}
-
-/*
  * Calls blk_new_with_bs() and then calls bdrv_open() on the BlockDriverState.
  *
  * Just as with bdrv_open(), after having called this function the reference to
@@ -150,20 +130,25 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
                            Error **errp)
 {
     BlockBackend *blk;
+    BlockDriverState *bs;
     int ret;
 
-    blk = blk_new_with_bs(name, errp);
+    blk = blk_new(name, errp);
     if (!blk) {
         QDECREF(options);
         return NULL;
     }
 
-    ret = bdrv_open(&blk->bs, filename, reference, options, flags, errp);
+    bs = NULL;
+    ret = bdrv_open(&bs, filename, reference, options, flags, errp);
     if (ret < 0) {
         blk_unref(blk);
         return NULL;
     }
 
+    blk->bs = bs;
+    bs->blk = blk;
+
     return blk;
 }
 
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH 5/8] block: Drop blk_new_with_bs()

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:20 AM CET, Max Reitz wrote:
> Its only caller is blk_new_open(), so we can just inline it there. Since
> bdrv_new_root() is only a wrapper around bdrv_new(), we can just use
> bdrv_new() instead.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 6/8] block: Drop bdrv_new_root()

[Max Reitz]

It is unused now, so we may just as well drop it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 5 -----
 include/block/block.h | 1 -
 2 files changed, 6 deletions(-)

diff --git a/block.c b/block.c
index 395b7b0..0531992 100644
--- a/block.c
+++ b/block.c
@@ -243,11 +243,6 @@ void bdrv_register(BlockDriver *bdrv)
     QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
 }
 
-BlockDriverState *bdrv_new_root(void)
-{
-    return bdrv_new();
-}
-
 BlockDriverState *bdrv_new(void)
 {
     BlockDriverState *bs;
diff --git a/include/block/block.h b/include/block/block.h
index 37e49a1..6638790 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -193,7 +193,6 @@ BlockDriver *bdrv_find_format(const char *format_name);
 int bdrv_create(BlockDriver *drv, const char* filename,
                 QemuOpts *opts, Error **errp);
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
-BlockDriverState *bdrv_new_root(void);
 BlockDriverState *bdrv_new(void);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH 6/8] block: Drop bdrv_new_root()

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:21 AM CET, Max Reitz wrote:
> It is unused now, so we may just as well drop it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 7/8] block: Make bdrv_open() return a BDS

[Max Reitz]

There are no callers to bdrv_open() or bdrv_open_inherit() left that
pass a pointer to a non-NULL BDS pointer as the first argument of these
functions, so we can finally drop that parameter and just make them
return the new BDS.

Generally, the following pattern is applied:

    bs = NULL;
    ret = bdrv_open(&bs, ..., &local_err);
    if (ret < 0) {
        error_propagate(errp, local_err);
        ...
    }

by

    bs = bdrv_open(..., errp);
    if (!bs) {
        ret = -EINVAL;
        ...
    }

Of course, there are only a few instances where the pattern is really
pure.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 121 +++++++++++++++++---------------------------------
 block/block-backend.c |   6 +--
 block/parallels.c     |   9 ++--
 block/qcow.c          |   9 ++--
 block/qcow2.c         |  29 +++++-------
 block/qed.c           |  11 ++---
 block/sheepdog.c      |  16 +++----
 block/vdi.c           |   7 ++-
 block/vhdx.c          |   8 ++--
 block/vmdk.c          |  24 +++++-----
 block/vpc.c           |   7 ++-
 block/vvfat.c         |   9 ++--
 blockdev.c            |  38 ++++++----------
 include/block/block.h |   4 +-
 14 files changed, 113 insertions(+), 185 deletions(-)

diff --git a/block.c b/block.c
index 0531992..f258c54 100644
--- a/block.c
+++ b/block.c
@@ -82,10 +82,12 @@ static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states =
 static QLIST_HEAD(, BlockDriver) bdrv_drivers =
     QLIST_HEAD_INITIALIZER(bdrv_drivers);
 
-static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
-                             const char *reference, QDict *options, int flags,
-                             BlockDriverState *parent,
-                             const BdrvChildRole *child_role, Error **errp);
+static BlockDriverState *bdrv_open_inherit(const char *filename,
+                                           const char *reference,
+                                           QDict *options, int flags,
+                                           BlockDriverState *parent,
+                                           const BdrvChildRole *child_role,
+                                           Error **errp);
 
 static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
 static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
@@ -1211,15 +1213,15 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         qdict_put(options, "driver", qstring_from_str(bs->backing_format));
     }
 
-    backing_hd = NULL;
-    ret = bdrv_open_inherit(&backing_hd,
-                            *backing_filename ? backing_filename : NULL,
-                            NULL, options, 0, bs, &child_backing, &local_err);
-    if (ret < 0) {
+    backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL,
+                                   NULL, options, 0, bs, &child_backing,
+                                   &local_err);
+    if (!backing_hd) {
         bs->open_flags |= BDRV_O_NO_BACKING;
         error_setg(errp, "Could not open backing file: %s",
                    error_get_pretty(local_err));
         error_free(local_err);
+        ret = -EINVAL;
         goto free_exit;
     }
 
@@ -1256,7 +1258,6 @@ BdrvChild *bdrv_open_child(const char *filename,
     BdrvChild *c = NULL;
     BlockDriverState *bs;
     QDict *image_options;
-    int ret;
     char *bdref_key_dot;
     const char *reference;
 
@@ -1276,10 +1277,9 @@ BdrvChild *bdrv_open_child(const char *filename,
         goto done;
     }
 
-    bs = NULL;
-    ret = bdrv_open_inherit(&bs, filename, reference, image_options, 0,
-                            parent, child_role, errp);
-    if (ret < 0) {
+    bs = bdrv_open_inherit(filename, reference, image_options, 0,
+                           parent, child_role, errp);
+    if (!bs) {
         goto done;
     }
 
@@ -1342,11 +1342,9 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     qdict_put(snapshot_options, "driver",
               qstring_from_str("qcow2"));
 
-    bs_snapshot = NULL;
-    ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
-                    flags, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, flags, errp);
+    if (!bs_snapshot) {
+        ret = -EINVAL;
         goto out;
     }
 
@@ -1376,10 +1374,12 @@ out:
  * should be opened. If specified, neither options nor a filename may be given,
  * nor can an existing BDS be reused (that is, *pbs has to be NULL).
  */
-static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
-                             const char *reference, QDict *options, int flags,
-                             BlockDriverState *parent,
-                             const BdrvChildRole *child_role, Error **errp)
+static BlockDriverState *bdrv_open_inherit(const char *filename,
+                                           const char *reference,
+                                           QDict *options, int flags,
+                                           BlockDriverState *parent,
+                                           const BdrvChildRole *child_role,
+                                           Error **errp)
 {
     int ret;
     BdrvChild *file = NULL;
@@ -1390,7 +1390,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
     Error *local_err = NULL;
     int snapshot_flags = 0;
 
-    assert(pbs);
     assert(!child_role || !flags);
     assert(!child_role == !parent);
 
@@ -1398,32 +1397,21 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         bool options_non_empty = options ? qdict_size(options) : false;
         QDECREF(options);
 
-        if (*pbs) {
-            error_setg(errp, "Cannot reuse an existing BDS when referencing "
-                       "another block device");
-            return -EINVAL;
-        }
-
         if (filename || options_non_empty) {
             error_setg(errp, "Cannot reference an existing block device with "
                        "additional options or a new filename");
-            return -EINVAL;
+            return NULL;
         }
 
         bs = bdrv_lookup_bs(reference, reference, errp);
         if (!bs) {
-            return -ENODEV;
+            return NULL;
         }
         bdrv_ref(bs);
-        *pbs = bs;
-        return 0;
+        return bs;
     }
 
-    if (*pbs) {
-        bs = *pbs;
-    } else {
-        bs = bdrv_new();
-    }
+    bs = bdrv_new();
 
     /* NULL means an empty set of options */
     if (options == NULL) {
@@ -1447,7 +1435,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         qdict_del(options, "driver");
         if (!drv) {
             error_setg(errp, "Unknown driver: '%s'", drvname);
-            ret = -EINVAL;
             goto fail;
         }
     }
@@ -1479,7 +1466,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         file = bdrv_open_child(filename, options, "file", bs,
                                &child_file, true, &local_err);
         if (local_err) {
-            ret = -EINVAL;
             goto fail;
         }
     }
@@ -1493,7 +1479,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         }
     } else if (!drv) {
         error_setg(errp, "Must specify either driver or file");
-        ret = -EINVAL;
         goto fail;
     }
 
@@ -1539,7 +1524,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
                        drv->format_name, entry->key);
         }
 
-        ret = -EINVAL;
         goto close_and_fail;
     }
 
@@ -1552,7 +1536,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
                && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
         error_setg(errp,
                    "Guest must be stopped for opening of encrypted image");
-        ret = -EBUSY;
         goto close_and_fail;
     }
 
@@ -1564,25 +1547,14 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         BlockDriverState *snapshot_bs;
         snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
-            ret = -EINVAL;
             goto close_and_fail;
         }
-        if (!*pbs) {
-            /* The reference is now held by the overlay BDS */
-            bdrv_unref(bs);
-            bs = snapshot_bs;
-        } else {
-            /* It is still referenced in the same way that *pbs was referenced,
-             * however that may be */
-            bdrv_unref(snapshot_bs);
-        }
-    }
-
-    if (!*pbs) {
-        *pbs = bs;
+        /* The reference is now held by the overlay BDS */
+        bdrv_unref(bs);
+        bs = snapshot_bs;
     }
 
-    return 0;
+    return bs;
 
 fail:
     if (file != NULL) {
@@ -1591,35 +1563,25 @@ fail:
     QDECREF(bs->options);
     QDECREF(options);
     bs->options = NULL;
-    if (!*pbs) {
-        /* If *pbs is NULL, a new BDS has been created in this function and
-           needs to be freed now. Otherwise, it does not need to be closed,
-           since it has not really been opened yet. */
-        bdrv_unref(bs);
-    }
+    bdrv_unref(bs);
     if (local_err) {
         error_propagate(errp, local_err);
     }
-    return ret;
+    return NULL;
 
 close_and_fail:
-    /* See fail path, but now the BDS has to be always closed */
-    if (*pbs) {
-        bdrv_close(bs);
-    } else {
-        bdrv_unref(bs);
-    }
+    bdrv_unref(bs);
     QDECREF(options);
     if (local_err) {
         error_propagate(errp, local_err);
     }
-    return ret;
+    return NULL;
 }
 
-int bdrv_open(BlockDriverState **pbs, const char *filename,
-              const char *reference, QDict *options, int flags, Error **errp)
+BlockDriverState *bdrv_open(const char *filename, const char *reference,
+                            QDict *options, int flags, Error **errp)
 {
-    return bdrv_open_inherit(pbs, filename, reference, options, flags, NULL,
+    return bdrv_open_inherit(filename, reference, options, flags, NULL,
                              NULL, errp);
 }
 
@@ -3636,11 +3598,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
                           qstring_from_str(backing_fmt));
             }
 
-            bs = NULL;
-            ret = bdrv_open(&bs, full_backing, NULL, backing_options,
-                            back_flags, &local_err);
+            bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
+                           &local_err);
             g_free(full_backing);
-            if (ret < 0) {
+            if (!bs) {
                 goto out;
             }
             size = bdrv_getlength(bs);
diff --git a/block/block-backend.c b/block/block-backend.c
index 13f5fef..9b674e2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -131,7 +131,6 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
 {
     BlockBackend *blk;
     BlockDriverState *bs;
-    int ret;
 
     blk = blk_new(name, errp);
     if (!blk) {
@@ -139,9 +138,8 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
         return NULL;
     }
 
-    bs = NULL;
-    ret = bdrv_open(&bs, filename, reference, options, flags, errp);
-    if (ret < 0) {
+    bs = bdrv_open(filename, reference, options, flags, errp);
+    if (!bs) {
         blk_unref(blk);
         return NULL;
     }
diff --git a/block/parallels.c b/block/parallels.c
index 4f79293..2e358a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -475,12 +475,9 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
         return ret;
     }
 
-    file = NULL;
-    ret = bdrv_open(&file, filename, NULL, NULL,
-                    BDRV_O_RDWR | BDRV_O_PROTOCOL, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        return ret;
+    file = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
+    if (!file) {
+        return -EINVAL;
     }
     ret = bdrv_truncate(file, 0);
     if (ret < 0) {
diff --git a/block/qcow.c b/block/qcow.c
index 2601954..013590a 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -790,11 +790,10 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
         goto cleanup;
     }
 
-    qcow_bs = NULL;
-    ret = bdrv_open(&qcow_bs, filename, NULL, NULL,
-                    BDRV_O_RDWR | BDRV_O_PROTOCOL, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    qcow_bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                        errp);
+    if (!qcow_bs) {
+        ret = -EINVAL;
         goto cleanup;
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 88e17aa..7dc5fe0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2095,12 +2095,9 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         return ret;
     }
 
-    bs = NULL;
-    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                    &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        return ret;
+    bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        return -EINVAL;
     }
 
     /* Write the header */
@@ -2149,7 +2146,6 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     }
 
     bdrv_unref(bs);
-    bs = NULL;
 
     /*
      * And now open the image and make it consistent first (i.e. increase the
@@ -2158,11 +2154,10 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow2"));
-    ret = bdrv_open(&bs, filename, NULL, options,
-                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
-                    &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    bs = bdrv_open(filename, NULL, options,
+                   BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, errp);
+    if (!bs) {
+        ret = -EINVAL;
         goto out;
     }
 
@@ -2207,16 +2202,14 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     }
 
     bdrv_unref(bs);
-    bs = NULL;
 
     /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow2"));
-    ret = bdrv_open(&bs, filename, NULL, options,
-                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
-                    &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    bs = bdrv_open(filename, NULL, options,
+                   BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, errp);
+    if (!bs) {
+        ret = -EINVAL;
         goto out;
     }
 
diff --git a/block/qed.c b/block/qed.c
index c15f0be..60af894 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -572,13 +572,10 @@ static int qed_create(const char *filename, uint32_t cluster_size,
         return ret;
     }
 
-    bs = NULL;
-    ret = bdrv_open(&bs, filename, NULL, NULL,
-                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
-                    &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        return ret;
+    bs = bdrv_open(filename, NULL, NULL,
+                   BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        return -EINVAL;
     }
 
     /* File must start empty and grow, check truncate is supported */
diff --git a/block/sheepdog.c b/block/sheepdog.c
index d80e4ed..24116dc 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1630,18 +1630,18 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot,
 
 static int sd_prealloc(const char *filename, Error **errp)
 {
-    BlockDriverState *bs = NULL;
+    BlockDriverState *bs;
     BDRVSheepdogState *base = NULL;
     unsigned long buf_size;
     uint32_t idx, max_idx;
     uint32_t object_size;
     int64_t vdi_size;
     void *buf = NULL;
-    int ret;
+    int ret = 0;
 
-    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                    errp);
-    if (ret < 0) {
+    bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        ret = -EINVAL;
         goto out_with_err_set;
     }
 
@@ -1831,9 +1831,9 @@ static int sd_create(const char *filename, QemuOpts *opts,
             goto out;
         }
 
-        bs = NULL;
-        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, errp);
-        if (ret < 0) {
+        bs = bdrv_open(backing_file, NULL, NULL, BDRV_O_PROTOCOL, errp);
+        if (!bs) {
+            ret = -EINVAL;
             goto out;
         }
 
diff --git a/block/vdi.c b/block/vdi.c
index 17f435f..12c3788 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -764,10 +764,9 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
         error_propagate(errp, local_err);
         goto exit;
     }
-    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                    &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        ret = -EINVAL;
         goto exit;
     }
 
diff --git a/block/vhdx.c b/block/vhdx.c
index 2fe9a5e..8ec4b86 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1842,11 +1842,9 @@ static int vhdx_create(const char *filename, QemuOpts *opts, Error **errp)
         goto exit;
     }
 
-    bs = NULL;
-    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                    &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        ret = -EINVAL;
         goto exit;
     }
 
diff --git a/block/vmdk.c b/block/vmdk.c
index bcce9a7..3c16e6d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1634,11 +1634,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
         goto exit;
     }
 
-    assert(bs == NULL);
-    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                    &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        ret = -EINVAL;
         goto exit;
     }
 
@@ -1898,7 +1896,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
         goto exit;
     }
     if (backing_file) {
-        BlockDriverState *bs = NULL;
+        BlockDriverState *bs;
         char *full_backing = g_new0(char, PATH_MAX);
         bdrv_get_full_backing_filename_from_filename(filename, backing_file,
                                                      full_backing, PATH_MAX,
@@ -1909,9 +1907,10 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
             ret = -ENOENT;
             goto exit;
         }
-        ret = bdrv_open(&bs, full_backing, NULL, NULL, BDRV_O_NO_BACKING, errp);
+        bs = bdrv_open(full_backing, NULL, NULL, BDRV_O_NO_BACKING, errp);
         g_free(full_backing);
-        if (ret != 0) {
+        if (!bs) {
+            ret = -EINVAL;
             goto exit;
         }
         if (strcmp(bs->drv->format_name, "vmdk")) {
@@ -1978,11 +1977,10 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
             goto exit;
         }
     }
-    assert(new_bs == NULL);
-    ret = bdrv_open(&new_bs, filename, NULL, NULL,
-                    BDRV_O_RDWR | BDRV_O_PROTOCOL, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    new_bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                       errp);
+    if (!new_bs) {
+        ret = -EINVAL;
         goto exit;
     }
     ret = bdrv_pwrite(new_bs, desc_offset, desc, desc_len);
diff --git a/block/vpc.c b/block/vpc.c
index 299d373..2d2a867 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -795,10 +795,9 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
         error_propagate(errp, local_err);
         goto out;
     }
-    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                    &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        ret = -EINVAL;
         goto out;
     }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index b184eca..a656e75 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2952,13 +2952,12 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
         goto err;
     }
 
-    s->qcow = NULL;
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow"));
-    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, options,
-                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
-                    errp);
-    if (ret < 0) {
+    s->qcow = bdrv_open(s->qcow_filename, NULL, options,
+                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, errp);
+    if (!s->qcow) {
+        ret = -EINVAL;
         goto err;
     }
 
diff --git a/blockdev.c b/blockdev.c
index f9c376f..d900282 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -629,7 +629,6 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
     QemuOpts *opts;
     Error *local_error = NULL;
     BlockdevDetectZeroesOptions detect_zeroes;
-    int ret;
     int bdrv_flags = 0;
 
     opts = qemu_opts_create(&qemu_root_bds_opts, NULL, 1, errp);
@@ -650,9 +649,8 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
         goto fail;
     }
 
-    bs = NULL;
-    ret = bdrv_open(&bs, NULL, NULL, bs_opts, bdrv_flags, errp);
-    if (ret < 0) {
+    bs = bdrv_open(NULL, NULL, bs_opts, bdrv_flags, errp);
+    if (!bs) {
         goto fail_no_bs_opts;
     }
 
@@ -1561,7 +1559,7 @@ typedef struct ExternalSnapshotState {
 static void external_snapshot_prepare(BlkTransactionState *common,
                                       Error **errp)
 {
-    int flags = 0, ret;
+    int flags = 0;
     QDict *options = NULL;
     Error *local_err = NULL;
     /* Device and node name of the image to generate the snapshot from */
@@ -1676,11 +1674,10 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         flags |= BDRV_O_NO_BACKING;
     }
 
-    assert(state->new_bs == NULL);
-    ret = bdrv_open(&state->new_bs, new_image_file, snapshot_ref, options,
-                    flags, errp);
+    state->new_bs = bdrv_open(new_image_file, snapshot_ref, options, flags,
+                              errp);
     /* We will manually add the backing_hd field to the bs later */
-    if (ret != 0) {
+    if (!state->new_bs) {
         return;
     }
 
@@ -2211,7 +2208,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
 {
     BlockBackend *blk;
     BlockDriverState *medium_bs = NULL;
-    int bdrv_flags, ret;
+    int bdrv_flags;
     QDict *options = NULL;
     Error *err = NULL;
 
@@ -2253,9 +2250,8 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         qdict_put(options, "driver", qstring_from_str(format));
     }
 
-    assert(!medium_bs);
-    ret = bdrv_open(&medium_bs, filename, NULL, options, bdrv_flags, errp);
-    if (ret < 0) {
+    medium_bs = bdrv_open(filename, NULL, options, bdrv_flags, errp);
+    if (!medium_bs) {
         goto fail;
     }
 
@@ -2829,7 +2825,6 @@ void qmp_drive_backup(const char *device, const char *target,
     Error *local_err = NULL;
     int flags;
     int64_t size;
-    int ret;
 
     if (!has_speed) {
         speed = 0;
@@ -2913,10 +2908,8 @@ void qmp_drive_backup(const char *device, const char *target,
         qdict_put(options, "driver", qstring_from_str(format));
     }
 
-    target_bs = NULL;
-    ret = bdrv_open(&target_bs, target, NULL, options, flags, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    target_bs = bdrv_open(target, NULL, options, flags, errp);
+    if (!target_bs) {
         goto out;
     }
 
@@ -3035,7 +3028,6 @@ void qmp_drive_mirror(const char *device, const char *target,
     QDict *options;
     int flags;
     int64_t size;
-    int ret;
 
     if (!has_speed) {
         speed = 0;
@@ -3178,11 +3170,9 @@ void qmp_drive_mirror(const char *device, const char *target,
     /* Mirroring takes care of copy-on-write using the source's backing
      * file.
      */
-    target_bs = NULL;
-    ret = bdrv_open(&target_bs, target, NULL, options,
-                    flags | BDRV_O_NO_BACKING, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    target_bs = bdrv_open(target, NULL, options, flags | BDRV_O_NO_BACKING,
+                          errp);
+    if (!target_bs) {
         goto out;
     }
 
diff --git a/include/block/block.h b/include/block/block.h
index 6638790..41001a2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -209,8 +209,8 @@ BdrvChild *bdrv_open_child(const char *filename,
                            bool allow_none, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-int bdrv_open(BlockDriverState **pbs, const char *filename,
-              const char *reference, QDict *options, int flags, Error **errp);
+BlockDriverState *bdrv_open(const char *filename, const char *reference,
+                            QDict *options, int flags, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs,
                                     QDict *options, int flags);
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH 7/8] block: Make bdrv_open() return a BDS

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:22 AM CET, Max Reitz wrote:
> @@ -1398,32 +1397,21 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>          bool options_non_empty = options ? qdict_size(options) : false;
>          QDECREF(options);
>  
> -        if (*pbs) {
> -            error_setg(errp, "Cannot reuse an existing BDS when referencing "
> -                       "another block device");
> -            return -EINVAL;
> -        }
> -
>          if (filename || options_non_empty) {
>              error_setg(errp, "Cannot reference an existing block device with "
>                         "additional options or a new filename");
> -            return -EINVAL;
> +            return NULL;
>          }
>  
>          bs = bdrv_lookup_bs(reference, reference, errp);
>          if (!bs) {
> -            return -ENODEV;
> +            return NULL;
>          }
>          bdrv_ref(bs);
> -        *pbs = bs;
> -        return 0;
> +        return bs;
>      }

This last part is probably a bit simpler with just one return statement:

           bs = bdrv_lookup_bs(reference, reference, errp);
           if (bs) {
               bdrv_ref(bs);
           }
           return bs;

but I'm fine either way.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 7/8] block: Make bdrv_open() return a BDS

[Max Reitz]

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

On 01.12.2015 15:44, Alberto Garcia wrote:
> On Tue 10 Nov 2015 04:44:22 AM CET, Max Reitz wrote:
>> @@ -1398,32 +1397,21 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>>          bool options_non_empty = options ? qdict_size(options) : false;
>>          QDECREF(options);
>>  
>> -        if (*pbs) {
>> -            error_setg(errp, "Cannot reuse an existing BDS when referencing "
>> -                       "another block device");
>> -            return -EINVAL;
>> -        }
>> -
>>          if (filename || options_non_empty) {
>>              error_setg(errp, "Cannot reference an existing block device with "
>>                         "additional options or a new filename");
>> -            return -EINVAL;
>> +            return NULL;
>>          }
>>  
>>          bs = bdrv_lookup_bs(reference, reference, errp);
>>          if (!bs) {
>> -            return -ENODEV;
>> +            return NULL;
>>          }
>>          bdrv_ref(bs);
>> -        *pbs = bs;
>> -        return 0;
>> +        return bs;
>>      }
> 
> This last part is probably a bit simpler with just one return statement:
> 
>            bs = bdrv_lookup_bs(reference, reference, errp);
>            if (bs) {
>                bdrv_ref(bs);
>            }
>            return bs;
> 
> but I'm fine either way.

Yes, thanks for the suggestion; but I think I like it to be explicit
("ret = do_something(); if (ret indicates failure) { fail; } go_on;").
Maybe I'm just not flexible enough to discard my precious patterns. :-)

> Reviewed-by: Alberto Garcia <berto@igalia.com>

Thanks!

Max


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

[Qemu-devel] [PATCH 8/8] block: Assert !bs->refcnt in bdrv_close()

[Max Reitz]

The only caller of bdrv_close() left is bdrv_delete(). We may as well
assert that, in a way (there are some things in bdrv_close() that make
more sense under that assumption, such as the call to
bdrv_release_all_dirty_bitmaps() which in turn assumes that no frozen
bitmaps are attached to the BDS).

In addition, being called only in bdrv_delete() means that we can drop
bdrv_close()'s forward declaration at the top of block.c.

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

diff --git a/block.c b/block.c
index f258c54..0abf80f 100644
--- a/block.c
+++ b/block.c
@@ -95,8 +95,6 @@ static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
-static void bdrv_close(BlockDriverState *bs);
-
 #ifdef _WIN32
 static int is_windows_drive_prefix(const char *filename)
 {
@@ -1872,6 +1870,7 @@ static void bdrv_close(BlockDriverState *bs)
     BdrvAioNotifier *ban, *ban_next;
 
     assert(!bs->job);
+    assert(!bs->refcnt);
 
     /* Disable I/O limits and drain all pending throttled requests */
     if (bs->throttle_state) {
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] block: Assert !bs->refcnt in bdrv_close()

[Alberto Garcia]

On Tue 10 Nov 2015 04:44:23 AM CET, Max Reitz wrote:
> The only caller of bdrv_close() left is bdrv_delete(). We may as well
> assert that, in a way (there are some things in bdrv_close() that make
> more sense under that assumption, such as the call to
> bdrv_release_all_dirty_bitmaps() which in turn assumes that no frozen
> bitmaps are attached to the BDS).
>
> In addition, being called only in bdrv_delete() means that we can drop
> bdrv_close()'s forward declaration at the top of block.c.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto