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

[Qemu-devel] [PATCH v2 for-2.7 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!


This series depends on the following series/patches:
- Revert "block: Forbid I/O throttling on nodes with multiple parents
  for 2.6"
  This is something I suppose Kevin will send when the 2.7 development
  window opens.
- "block: Move I/O throttling to BlockBackend" by Kevin
- "block: Remove BlockDriverState.blk" by Kevin


v2:
- Patch 1: bdrv_open_backing_file() has been changed already, so that
  part can be dropped from the patch
- Patch 2:
  - bdrv_append_temp_snapshot() now takes an additional argument
    (snapshot_options)
  - Drop a superfluous assignment to ret [Berto]
  - The function is now also static already, so we don't need to do that
- Patch 3: Added because this test has been added in the meantime
- Patch 4: Rebase conflicts
- Patch 6:
  - Rebase conflicts
  - A whole lot of places we no longer need to touch because they use
    blk_new_open() instead of bdrv_open()
- Patch 8: Added because patch 7 makes this possible.


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/8:[down] 'block: Drop useless bdrv_new() call'
       ^^^^ Actually [0006] [FC] with a changed commit title
002/8:[0015] [FC] 'block: Let bdrv_open_inherit() return the snapshot'
003/8:[down] 'tests: Drop BDS from test-throttle.c'
004/8:[0019] [FC] 'block: Drop blk_new_with_bs()'
005/8:[----] [-C] 'block: Drop bdrv_new_root()'
006/8:[0135] [FC] 'block: Make bdrv_open() return a BDS'
007/8:[----] [-C] 'block: Assert !bs->refcnt in bdrv_close()'
008/8:[down] 'block: Drop bdrv_parent_cb_...() from bdrv_close()'


Max Reitz (8):
  block: Drop useless bdrv_new() call
  block: Let bdrv_open_inherit() return the snapshot
  tests: Drop BDS from test-throttle.c
  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: Drop bdrv_parent_cb_...() from bdrv_close()

 block.c               | 139 ++++++++++++++++++++------------------------------
 block/block-backend.c |  30 +++--------
 block/vvfat.c         |   8 +--
 blockdev.c            |  38 +++++---------
 include/block/block.h |   5 +-
 tests/test-throttle.c |   6 +--
 6 files changed, 83 insertions(+), 143 deletions(-)

-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.7 1/8] block: Drop useless bdrv_new() call

[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.

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 1be887a..b6a452a 100644
--- a/block.c
+++ b/block.c
@@ -1472,8 +1472,7 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
     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);
     snapshot_options = NULL;
-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.7 2/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. 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 | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index b6a452a..d222227 100644
--- a/block.c
+++ b/block.c
@@ -1424,8 +1424,10 @@ done:
     return c;
 }
 
-static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
-                                     QDict *snapshot_options, Error **errp)
+static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
+                                                   int flags,
+                                                   QDict *snapshot_options,
+                                                   Error **errp)
 {
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char *tmp_filename = g_malloc0(PATH_MAX + 1);
@@ -1441,7 +1443,6 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
     /* Get the required size from the image */
     total_size = bdrv_getlength(bs);
     if (total_size < 0) {
-        ret = total_size;
         error_setg_errno(errp, -total_size, "Could not get image size");
         goto out;
     }
@@ -1481,12 +1482,16 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
         goto out;
     }
 
+    bdrv_ref(bs_snapshot);
     bdrv_append(bs_snapshot, bs);
 
+    g_free(tmp_filename);
+    return bs_snapshot;
+
 out:
     QDECREF(snapshot_options);
     g_free(tmp_filename);
-    return ret;
+    return NULL;
 }
 
 /*
@@ -1705,17 +1710,31 @@ 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, snapshot_options,
-                                        &local_err);
+        BlockDriverState *snapshot_bs;
+        snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags,
+                                                snapshot_options, &local_err);
         snapshot_options = NULL;
         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;
-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.7 3/8] tests: Drop BDS from test-throttle.c

[Max Reitz]

Now that throttling has been moved to the BlockBackend level, we do not
need to create a BDS along with the BB in the I/O throttling test.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/test-throttle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 5ec966c..d7fb0a6 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -578,9 +578,9 @@ static void test_groups(void)
     BlockBackend *blk1, *blk2, *blk3;
     BlockBackendPublic *blkp1, *blkp2, *blkp3;
 
-    blk1 = blk_new_with_bs(&error_abort);
-    blk2 = blk_new_with_bs(&error_abort);
-    blk3 = blk_new_with_bs(&error_abort);
+    blk1 = blk_new(&error_abort);
+    blk2 = blk_new(&error_abort);
+    blk3 = blk_new(&error_abort);
 
     blkp1 = blk_get_public(blk1);
     blkp2 = blk_get_public(blk2);
-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.7 4/8] block: Drop blk_new_with_bs()

[Max Reitz]

Its only caller is blk_new_open(), so we can just inline it there.

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

diff --git a/block/block-backend.c b/block/block-backend.c
index 66b8bad..4e8e8ab 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -175,26 +175,6 @@ BlockBackend *blk_new(Error **errp)
 }
 
 /*
- * Create a new BlockBackend with a new BlockDriverState attached.
- * Otherwise just like blk_new(), which see.
- */
-BlockBackend *blk_new_with_bs(Error **errp)
-{
-    BlockBackend *blk;
-    BlockDriverState *bs;
-
-    blk = blk_new(errp);
-    if (!blk) {
-        return NULL;
-    }
-
-    bs = bdrv_new_root();
-    blk->root = bdrv_root_attach_child(bs, "root", &child_root);
-    blk->root->opaque = 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
@@ -210,21 +190,25 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
                            QDict *options, int flags, Error **errp)
 {
     BlockBackend *blk;
+    BlockDriverState *bs;
     int ret;
 
-    blk = blk_new_with_bs(errp);
+    blk = blk_new(errp);
     if (!blk) {
         QDECREF(options);
         return NULL;
     }
 
-    ret = bdrv_open(&blk->root->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_set_enable_write_cache(blk, true);
+    blk->root = bdrv_root_attach_child(bs, "root", &child_root);
+    blk->root->opaque = blk;
 
     return blk;
 }
-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.7 5/8] block: Drop bdrv_new_root()

[Max Reitz]

By now it has become just a wrapper around bdrv_new() and it has only a
single user. Use bdrv_new() there instead and drop this function.

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

diff --git a/block.c b/block.c
index d222227..b6d1796 100644
--- a/block.c
+++ b/block.c
@@ -222,11 +222,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 762984e..31fcd07 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -198,7 +198,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_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_replace_in_backing_chain(BlockDriverState *old,
-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.7 6/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               | 123 +++++++++++++++++---------------------------------
 block/block-backend.c |   6 +--
 block/vvfat.c         |   8 ++--
 blockdev.c            |  38 ++++++----------
 include/block/block.h |   4 +-
 5 files changed, 63 insertions(+), 116 deletions(-)

diff --git a/block.c b/block.c
index b6d1796..355c4be 100644
--- a/block.c
+++ b/block.c
@@ -64,10 +64,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);
 
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
@@ -1338,14 +1340,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
         qdict_put(options, "driver", qstring_from_str(bs->backing_format));
     }
 
-    backing_hd = NULL;
-    ret = bdrv_open_inherit(&backing_hd,
-                            *backing_filename ? backing_filename : NULL,
-                            reference, options, 0, bs, &child_backing,
-                            errp);
-    if (ret < 0) {
+    backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL,
+                                   reference, options, 0, bs, &child_backing,
+                                   errp);
+    if (!backing_hd) {
         bs->open_flags |= BDRV_O_NO_BACKING;
         error_prepend(errp, "Could not open backing file: ");
+        ret = -EINVAL;
         goto free_exit;
     }
 
@@ -1385,7 +1386,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;
 
@@ -1405,10 +1405,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;
     }
 
@@ -1429,7 +1428,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     int64_t total_size;
     QemuOpts *opts = NULL;
     BlockDriverState *bs_snapshot;
-    Error *local_err = NULL;
     int ret;
 
     /* if snapshot, we create a temporary backing file and open it
@@ -1468,12 +1466,10 @@ 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);
+    bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, flags, errp);
     snapshot_options = NULL;
-    if (ret < 0) {
-        error_propagate(errp, local_err);
+    if (!bs_snapshot) {
+        ret = -EINVAL;
         goto out;
     }
 
@@ -1504,10 +1500,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;
@@ -1519,7 +1517,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
     QDict *snapshot_options = NULL;
     int snapshot_flags = 0;
 
-    assert(pbs);
     assert(!child_role || !flags);
     assert(!child_role == !parent);
 
@@ -1527,32 +1524,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) {
@@ -1589,7 +1575,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         drv = bdrv_find_format(drvname);
         if (!drv) {
             error_setg(errp, "Unknown driver: '%s'", drvname);
-            ret = -EINVAL;
             goto fail;
         }
     }
@@ -1619,7 +1604,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;
         }
     }
@@ -1646,7 +1630,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         qdict_put(options, "driver", qstring_from_str(drv->format_name));
     } else if (!drv) {
         error_setg(errp, "Must specify either driver or file");
-        ret = -EINVAL;
         goto fail;
     }
 
@@ -1689,7 +1672,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
                        drv->format_name, entry->key);
         }
 
-        ret = -EINVAL;
         goto close_and_fail;
     }
 
@@ -1700,7 +1682,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;
     }
 
@@ -1714,25 +1695,14 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
                                                 snapshot_options, &local_err);
         snapshot_options = NULL;
         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) {
@@ -1743,36 +1713,26 @@ 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(snapshot_options);
     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);
 }
 
@@ -3527,11 +3487,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 4e8e8ab..1b6f391 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -191,7 +191,6 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
 {
     BlockBackend *blk;
     BlockDriverState *bs;
-    int ret;
 
     blk = blk_new(errp);
     if (!blk) {
@@ -199,9 +198,8 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
         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/vvfat.c b/block/vvfat.c
index 6b85314..cf7e266 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2953,12 +2953,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_NO_FLUSH, errp);
-    if (ret < 0) {
+    s->qcow = bdrv_open(s->qcow_filename, NULL, options,
+                        BDRV_O_RDWR | BDRV_O_NO_FLUSH, errp);
+    if (!s->qcow) {
+        ret = -EINVAL;
         goto err;
     }
 
diff --git a/blockdev.c b/blockdev.c
index a2bd31a..7048d91 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -657,7 +657,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);
@@ -688,9 +687,8 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
         bdrv_flags |= BDRV_O_INACTIVE;
     }
 
-    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;
     }
 
@@ -1643,7 +1641,7 @@ typedef struct ExternalSnapshotState {
 static void external_snapshot_prepare(BlkActionState *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 */
@@ -1768,11 +1766,10 @@ static void external_snapshot_prepare(BlkActionState *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;
     }
 
@@ -2509,7 +2506,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;
 
@@ -2553,9 +2550,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;
     }
 
@@ -3168,7 +3164,6 @@ static void do_drive_backup(const char *device, const char *target,
     Error *local_err = NULL;
     int flags;
     int64_t size;
-    int ret;
 
     if (!has_speed) {
         speed = 0;
@@ -3252,10 +3247,8 @@ static void do_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;
     }
 
@@ -3480,7 +3473,6 @@ void qmp_drive_mirror(const char *device, const char *target,
     QDict *options = NULL;
     int flags;
     int64_t size;
-    int ret;
 
     blk = blk_by_name(device);
     if (!blk) {
@@ -3589,11 +3581,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 31fcd07..ce033ec 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -213,8 +213,8 @@ BdrvChild *bdrv_open_child(const char *filename,
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, 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.8.0

[Qemu-devel] [PATCH v2 for-2.7 7/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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 355c4be..963abcc 100644
--- a/block.c
+++ b/block.c
@@ -74,8 +74,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 /* 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)
 {
@@ -2106,6 +2104,7 @@ static void bdrv_close(BlockDriverState *bs)
     BdrvAioNotifier *ban, *ban_next;
 
     assert(!bs->job);
+    assert(!bs->refcnt);
 
     bdrv_drained_begin(bs); /* complete I/O */
     bdrv_flush(bs);
-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.7 8/8] block: Drop bdrv_parent_cb_...() from bdrv_close()

[Max Reitz]

bdrv_close() now asserts that the BDS's refcount is 0, therefore it
cannot have any parents and the bdrv_parent_cb_change_media() call is a
no-op.

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

diff --git a/block.c b/block.c
index 963abcc..7d7fd89 100644
--- a/block.c
+++ b/block.c
@@ -2113,8 +2113,6 @@ static void bdrv_close(BlockDriverState *bs)
     bdrv_release_named_dirty_bitmaps(bs);
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
-    bdrv_parent_cb_change_media(bs, false);
-
     if (bs->drv) {
         BdrvChild *child, *next;
 
-- 
2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() return the snapshot

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> 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. 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>

This is a tricky patch, but after all it looks correct to me. I think we
could improve a bit on the documentation, though:

1. The commit message suggests that by returning the wrong BDS we may
   have an observable bug. It would be good to add details on why this
   used to be harmless (IIUC, all users of BDRV_O_SNAPSHOT go through
   blk_new_open(), and there first setting *pbs (which is blk->root->bs)
   and then doing bdrv_append() does the right thing)

2. The refcounting stuff isn't obvious either:

> @@ -1481,12 +1482,16 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
>          goto out;
>      }
>  
> +    bdrv_ref(bs_snapshot);
>      bdrv_append(bs_snapshot, bs);

This is because bdrv_append() drops the reference, but we want to return
a strong reference.

>      /* 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, snapshot_options,
> -                                        &local_err);
> +        BlockDriverState *snapshot_bs;
> +        snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags,
> +                                                snapshot_options, &local_err);
>          snapshot_options = NULL;
>          if (local_err) {
> +            ret = -EINVAL;
>              goto close_and_fail;
>          }
> +        if (!*pbs) {
> +            /* The reference is now held by the overlay BDS */
> +            bdrv_unref(bs);

We still hold a strong reference to the newly created bs that we wanted
to return, but now we're returning a different BDS, so we must drop the
reference. (The overlay BDS doesn't hold "the" same reference as the
comment suggests, but an additional one.)

> +            bs = snapshot_bs;
> +        } else {
> +            /* It is still referenced in the same way that *pbs was referenced,
> +             * however that may be */
> +            bdrv_unref(snapshot_bs);

In this case we don't in fact return the reference for bs_snapshot, so
drop it.

So I think what I would like here is comments that explain where the
ownership of the individual strong references goes, not who else may or
may not hold additional references to a BDS.

Kevin

Re: [Qemu-devel] [PATCH v2 for-2.7 1/8] block: Drop useless bdrv_new() call

[Alberto Garcia]

On Wed 06 Apr 2016 07:57:07 PM CEST, 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.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

Re: [Qemu-devel] [PATCH v2 for-2.7 1/8] block: Drop useless bdrv_new() call

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> 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.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 for-2.7 3/8] tests: Drop BDS from test-throttle.c

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> Now that throttling has been moved to the BlockBackend level, we do not
> need to create a BDS along with the BB in the I/O throttling test.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 for-2.7 4/8] block: Drop blk_new_with_bs()

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> Its only caller is blk_new_open(), so we can just inline it there.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Maybe mention that this isn't pure code motion, but that you switch to a
bdrv_open() call with a NULL BDS. Either way:

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

Re: [Qemu-devel] [PATCH v2 for-2.7 5/8] block: Drop bdrv_new_root()

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> By now it has become just a wrapper around bdrv_new() and it has only a
> single user. Use bdrv_new() there instead and drop this function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>

The commit message isn't up-to-date any more. The last user is already
removed in patch 4. With this fixed:

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

Re: [Qemu-devel] [PATCH v2 for-2.7 6/8] block: Make bdrv_open() return a BDS

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> 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>

> @@ -1527,32 +1524,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) {

While the following hunks remove the other instances, there's one
ret = -EINVAL left between here and the next hunk:

    /* json: syntax counts as explicit options, as if in the QDict */
    parse_json_protocol(options, &filename, &local_err);
    if (local_err) {
        ret = -EINVAL;
        goto fail;
    }

> @@ -1589,7 +1575,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>          drv = bdrv_find_format(drvname);
>          if (!drv) {
>              error_setg(errp, "Unknown driver: '%s'", drvname);
> -            ret = -EINVAL;
>              goto fail;
>          }
>      }

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

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

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> 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>

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

Re: [Qemu-devel] [PATCH v2 for-2.7 8/8] block: Drop bdrv_parent_cb_...() from bdrv_close()

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> bdrv_close() now asserts that the BDS's refcount is 0, therefore it
> cannot have any parents and the bdrv_parent_cb_change_media() call is a
> no-op.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() return the snapshot

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3437 bytes --]

On 07.04.2016 13:29, Kevin Wolf wrote:
> Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
>> 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. 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>
> 
> This is a tricky patch, but after all it looks correct to me. I think we
> could improve a bit on the documentation, though:
> 
> 1. The commit message suggests that by returning the wrong BDS we may
>    have an observable bug. It would be good to add details on why this
>    used to be harmless (IIUC, all users of BDRV_O_SNAPSHOT go through
>    blk_new_open(), and there first setting *pbs (which is blk->root->bs)
>    and then doing bdrv_append() does the right thing)

To be honest, I'd rather not wrap my head around why it worked, but I'll
try to.

> 2. The refcounting stuff isn't obvious either:
> 
>> @@ -1481,12 +1482,16 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
>>          goto out;
>>      }
>>  
>> +    bdrv_ref(bs_snapshot);
>>      bdrv_append(bs_snapshot, bs);
> 
> This is because bdrv_append() drops the reference, but we want to return
> a strong reference.

Well, it's mostly because now we do return a (strong) reference. Before,
bs_snapshot simply was not returned at all.

I'll add a comment, though.

>>      /* 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, snapshot_options,
>> -                                        &local_err);
>> +        BlockDriverState *snapshot_bs;
>> +        snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags,
>> +                                                snapshot_options, &local_err);
>>          snapshot_options = NULL;
>>          if (local_err) {
>> +            ret = -EINVAL;
>>              goto close_and_fail;
>>          }
>> +        if (!*pbs) {
>> +            /* The reference is now held by the overlay BDS */
>> +            bdrv_unref(bs);
> 
> We still hold a strong reference to the newly created bs that we wanted
> to return, but now we're returning a different BDS, so we must drop the
> reference. (The overlay BDS doesn't hold "the" same reference as the
> comment suggests, but an additional one.)
> 
>> +            bs = snapshot_bs;
>> +        } else {
>> +            /* It is still referenced in the same way that *pbs was referenced,
>> +             * however that may be */
>> +            bdrv_unref(snapshot_bs);
> 
> In this case we don't in fact return the reference for bs_snapshot, so
> drop it.
> 
> So I think what I would like here is comments that explain where the
> ownership of the individual strong references goes, not who else may or
> may not hold additional references to a BDS.

Well, the ownership goes away. ;-)

I've fattened the comments so they explain exactly why the refcount is
decremented, respectively, and why this will most likely not result in
the deletion of the BDS.

Max


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

Re: [Qemu-devel] [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() return the snapshot

[Kevin Wolf]

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

Am 08.04.2016 um 17:49 hat Max Reitz geschrieben:
> On 07.04.2016 13:29, Kevin Wolf wrote:
> > Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> >> 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. 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>
> > 
> > This is a tricky patch, but after all it looks correct to me. I think we
> > could improve a bit on the documentation, though:
> > 
> > 1. The commit message suggests that by returning the wrong BDS we may
> >    have an observable bug. It would be good to add details on why this
> >    used to be harmless (IIUC, all users of BDRV_O_SNAPSHOT go through
> >    blk_new_open(), and there first setting *pbs (which is blk->root->bs)
> >    and then doing bdrv_append() does the right thing)
> 
> To be honest, I'd rather not wrap my head around why it worked, but I'll
> try to.

I hope you just need to verify my theory stated above. ;-)

> > 2. The refcounting stuff isn't obvious either:
> > 
> >> @@ -1481,12 +1482,16 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
> >>          goto out;
> >>      }
> >>  
> >> +    bdrv_ref(bs_snapshot);
> >>      bdrv_append(bs_snapshot, bs);
> > 
> > This is because bdrv_append() drops the reference, but we want to return
> > a strong reference.
> 
> Well, it's mostly because now we do return a (strong) reference. Before,
> bs_snapshot simply was not returned at all.
> 
> I'll add a comment, though.

Yes, that's the additional reference you need. I just kept all existing
references in mind:

We already get a strong reference from bdrv_new(). bdrv_append() takes
ownership of a strong reference, though, and this is why we need a
second one to return.

> >>      /* 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, snapshot_options,
> >> -                                        &local_err);
> >> +        BlockDriverState *snapshot_bs;
> >> +        snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags,
> >> +                                                snapshot_options, &local_err);
> >>          snapshot_options = NULL;
> >>          if (local_err) {
> >> +            ret = -EINVAL;
> >>              goto close_and_fail;
> >>          }
> >> +        if (!*pbs) {
> >> +            /* The reference is now held by the overlay BDS */
> >> +            bdrv_unref(bs);
> > 
> > We still hold a strong reference to the newly created bs that we wanted
> > to return, but now we're returning a different BDS, so we must drop the
> > reference. (The overlay BDS doesn't hold "the" same reference as the
> > comment suggests, but an additional one.)
> > 
> >> +            bs = snapshot_bs;
> >> +        } else {
> >> +            /* It is still referenced in the same way that *pbs was referenced,
> >> +             * however that may be */
> >> +            bdrv_unref(snapshot_bs);
> > 
> > In this case we don't in fact return the reference for bs_snapshot, so
> > drop it.
> > 
> > So I think what I would like here is comments that explain where the
> > ownership of the individual strong references goes, not who else may or
> > may not hold additional references to a BDS.
> 
> Well, the ownership goes away. ;-)

Still good to know that this function owns the reference before it drops
it. ;-)

> I've fattened the comments so they explain exactly why the refcount is
> decremented, respectively, and why this will most likely not result in
> the deletion of the BDS.

Sounds good.

Kevin

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