diff mbox

[v5,03/23] block: Connect BlockBackend to BlockDriverState

Message ID 1412240698-21695-4-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Oct. 2, 2014, 9:04 a.m. UTC
Convenience function blk_new_with_bs() creates a BlockBackend with its
BlockDriverState.  Callers have to unref both.  The commit after next
will relieve them of the need to unref the BlockDriverState.

Complication: due to the silly way drive_del works, we need a way to
hide a BlockBackend, just like bdrv_make_anon().  To emphasize its
"special" status, give the function a suitably off-putting name:
blk_hide_on_behalf_of_do_drive_del().  Unfortunately, hiding turns the
BlockBackend's name into the empty string.  Can't avoid that without
breaking the blk->bs->device_name equals blk->name invariant.

The patch adds a memory leak: drive_del while a device model is
connected leaks the BlockBackend.  Avoiding the leak here is rather
hairy, but it'll become straightforward shortly, so I mark it FIXME in
the code now, and plug it when it's easy.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c                        |  12 ++--
 block/block-backend.c          |  71 ++++++++++++++++++++++-
 blockdev.c                     |  18 +++---
 hw/block/xen_disk.c            |   8 +--
 include/block/block_int.h      |   2 +
 include/sysemu/block-backend.h |   5 ++
 qemu-img.c                     | 125 +++++++++++++++++++----------------------
 qemu-io.c                      |   4 +-
 qemu-nbd.c                     |   4 +-
 9 files changed, 156 insertions(+), 93 deletions(-)

Comments

Kevin Wolf Oct. 2, 2014, 10:41 a.m. UTC | #1
Am 02.10.2014 um 11:04 hat Markus Armbruster geschrieben:
> Convenience function blk_new_with_bs() creates a BlockBackend with its
> BlockDriverState.  Callers have to unref both.  The commit after next
> will relieve them of the need to unref the BlockDriverState.
> 
> Complication: due to the silly way drive_del works, we need a way to
> hide a BlockBackend, just like bdrv_make_anon().  To emphasize its
> "special" status, give the function a suitably off-putting name:
> blk_hide_on_behalf_of_do_drive_del().  Unfortunately, hiding turns the
> BlockBackend's name into the empty string.  Can't avoid that without
> breaking the blk->bs->device_name equals blk->name invariant.
> 
> The patch adds a memory leak: drive_del while a device model is
> connected leaks the BlockBackend.  Avoiding the leak here is rather
> hairy, but it'll become straightforward shortly, so I mark it FIXME in
> the code now, and plug it when it's easy.

Does this leak actually still exist now that you have a blk_unref() in
drive_del() (which is called during autodel) rather than do_drive_del()?

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Kevin
Markus Armbruster Oct. 2, 2014, 11:37 a.m. UTC | #2
Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.10.2014 um 11:04 hat Markus Armbruster geschrieben:
>> Convenience function blk_new_with_bs() creates a BlockBackend with its
>> BlockDriverState.  Callers have to unref both.  The commit after next
>> will relieve them of the need to unref the BlockDriverState.
>> 
>> Complication: due to the silly way drive_del works, we need a way to
>> hide a BlockBackend, just like bdrv_make_anon().  To emphasize its
>> "special" status, give the function a suitably off-putting name:
>> blk_hide_on_behalf_of_do_drive_del().  Unfortunately, hiding turns the
>> BlockBackend's name into the empty string.  Can't avoid that without
>> breaking the blk->bs->device_name equals blk->name invariant.
>> 
>> The patch adds a memory leak: drive_del while a device model is
>> connected leaks the BlockBackend.  Avoiding the leak here is rather
>> hairy, but it'll become straightforward shortly, so I mark it FIXME in
>> the code now, and plug it when it's easy.
>
> Does this leak actually still exist now that you have a blk_unref() in
> drive_del() (which is called during autodel) rather than do_drive_del()?

Yes.  The following hunk adds it:

    @@ -1813,11 +1811,11 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
          * then we can just get rid of the block driver state right here.
          */
         if (bdrv_get_attached_dev(bs)) {
    -        bdrv_make_anon(bs);
    -
    +        blk_hide_on_behalf_of_do_drive_del(blk);
             /* Further I/O must not pause the guest */
             bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
                               BLOCKDEV_ON_ERROR_REPORT);
    +        /* FIXME bs->blk leaked when bs dies */
         } else {
             drive_del(dinfo);
         }

where blk_hide_on_behalf_of_do_drive_del() is from a few hunks further
up:

    +/*
    + * Hide @blk.
    + * @blk must not have been hidden already.
    + * Make attached BlockDriverState, if any, anonymous.
    + * Once hidden, @blk is invisible to all functions that don't receive
    + * it as argument.  For example, blk_by_name() won't return it.
    + * Strictly for use by do_drive_del().
    + * TODO get rid of it!
    + */
    +void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk)
    +{
    +    QTAILQ_REMOVE(&blk_backends, blk, link);
    +    blk->name[0] = 0;
    +    if (blk->bs) {
    +        bdrv_make_anon(blk->bs);
    +    }
    +}

The net effect is just

    +    QTAILQ_REMOVE(&blk_backends, blk, link);
    +    blk->name[0] = 0;

Admittedly not obvious: this messes with drive_del()!  Let's follow the
call chain.

    void blockdev_auto_del(BlockDriverState *bs)
    {
        DriveInfo *dinfo = drive_get_by_blockdev(bs);

        if (dinfo && dinfo->auto_del) {
            drive_del(dinfo);
        }
    }

drive_get_by_blockdev() still returns the DriveInfo, as we haven't
touched drives (we will in the next patch).  However:

    void drive_del(DriveInfo *dinfo)
    {
        BlockBackend *blk = blk_by_name(dinfo->id);

        bdrv_unref(dinfo->bdrv);
        blk_unref(blk);
    }

blk_by_name() returns null rather than the BB, because
blk_hide_on_behalf_of_do_drive_del() already removed the BB from
blk_backends.

PATCH 06 plugs the leak: blockdev_auto_del() passes the BB straight to
blk_unref().

But there's still a bug!  Your review of v3 convinced me that the leak
was already plugged in PATCH 04.  Maybe it was in v3 (I didn't check
again), but in v5, there's temporary breakage instead.  I'll try to
extend the test suite to cover it in v6.
diff mbox

Patch

diff --git a/block.c b/block.c
index 3905c32..a25fae9 100644
--- a/block.c
+++ b/block.c
@@ -2025,6 +2025,8 @@  static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
             bs_src->device_name);
     bs_dest->device_list = bs_src->device_list;
+    bs_dest->blk = bs_src->blk;
+
     memcpy(bs_dest->op_blockers, bs_src->op_blockers,
            sizeof(bs_dest->op_blockers));
 }
@@ -2037,7 +2039,7 @@  static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
  * This will modify the BlockDriverState fields, and swap contents
  * between bs_new and bs_old. Both bs_new and bs_old are modified.
  *
- * bs_new is required to be anonymous.
+ * bs_new must be nameless and not attached to a BlockBackend.
  *
  * This function does not create any image files.
  */
@@ -2056,8 +2058,9 @@  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
         QTAILQ_REMOVE(&graph_bdrv_states, bs_old, node_list);
     }
 
-    /* bs_new must be anonymous and shouldn't have anything fancy enabled */
+    /* bs_new must be nameless and shouldn't have anything fancy enabled */
     assert(bs_new->device_name[0] == '\0');
+    assert(!bs_new->blk);
     assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
     assert(bs_new->job == NULL);
     assert(bs_new->dev == NULL);
@@ -2073,8 +2076,9 @@  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     bdrv_move_feature_fields(bs_old, bs_new);
     bdrv_move_feature_fields(bs_new, &tmp);
 
-    /* bs_new shouldn't be in bdrv_states even after the swap!  */
+    /* bs_new must remain nameless and unattached */
     assert(bs_new->device_name[0] == '\0');
+    assert(!bs_new->blk);
 
     /* Check a few fields that should remain attached to the device */
     assert(bs_new->dev == NULL);
@@ -2101,7 +2105,7 @@  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
  * This will modify the BlockDriverState fields, and swap contents
  * between bs_new and bs_top. Both bs_new and bs_top are modified.
  *
- * bs_new is required to be anonymous.
+ * bs_new must be nameless and not attached to a BlockBackend.
  *
  * This function does not create any image files.
  */
diff --git a/block/block-backend.c b/block/block-backend.c
index e89caa9..a12215a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -16,10 +16,11 @@ 
 struct BlockBackend {
     char *name;
     int refcnt;
+    BlockDriverState *bs;
     QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
 };
 
-/* All the BlockBackends */
+/* All the BlockBackends (except for hidden ones) */
 static QTAILQ_HEAD(, BlockBackend) blk_backends =
     QTAILQ_HEAD_INITIALIZER(blk_backends);
 
@@ -47,10 +48,44 @@  BlockBackend *blk_new(const char *name, Error **errp)
     return blk;
 }
 
+/*
+ * Create a new BlockBackend with a new BlockDriverState attached.
+ * Both have a reference count of one.  Caller owns *both* references.
+ * TODO Let caller own only the BlockBackend reference
+ * 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(name, errp);
+    if (!bs) {
+        blk_unref(blk);
+        return NULL;
+    }
+
+    blk->bs = bs;
+    bs->blk = blk;
+    return blk;
+}
+
 static void blk_delete(BlockBackend *blk)
 {
     assert(!blk->refcnt);
-    QTAILQ_REMOVE(&blk_backends, blk, link);
+    if (blk->bs) {
+        blk->bs->blk = NULL;
+        blk->bs = NULL;
+    }
+    /* Avoid double-remove after blk_hide_on_behalf_of_do_drive_del() */
+    if (blk->name[0]) {
+        QTAILQ_REMOVE(&blk_backends, blk, link);
+    }
     g_free(blk->name);
     g_free(blk);
 }
@@ -68,6 +103,8 @@  void blk_ref(BlockBackend *blk)
  * Decrement @blk's reference count.
  * If this drops it to zero, destroy @blk.
  * For convenience, do nothing if @blk is null.
+ * Does *not* touch the attached BlockDriverState's reference count.
+ * TODO Decrement it!
  */
 void blk_unref(BlockBackend *blk)
 {
@@ -95,7 +132,9 @@  BlockBackend *blk_next(BlockBackend *blk)
 }
 
 /*
- * Return @blk's name, a non-null, non-empty string.
+ * Return @blk's name, a non-null string.
+ * Wart: the name is empty iff @blk has been hidden with
+ * blk_hide_on_behalf_of_do_drive_del().
  */
 const char *blk_name(BlockBackend *blk)
 {
@@ -118,3 +157,29 @@  BlockBackend *blk_by_name(const char *name)
     }
     return NULL;
 }
+
+/*
+ * Return the BlockDriverState attached to @blk if any, else null.
+ */
+BlockDriverState *blk_bs(BlockBackend *blk)
+{
+    return blk->bs;
+}
+
+/*
+ * Hide @blk.
+ * @blk must not have been hidden already.
+ * Make attached BlockDriverState, if any, anonymous.
+ * Once hidden, @blk is invisible to all functions that don't receive
+ * it as argument.  For example, blk_by_name() won't return it.
+ * Strictly for use by do_drive_del().
+ * TODO get rid of it!
+ */
+void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk)
+{
+    QTAILQ_REMOVE(&blk_backends, blk, link);
+    blk->name[0] = 0;
+    if (blk->bs) {
+        bdrv_make_anon(blk->bs);
+    }
+}
diff --git a/blockdev.c b/blockdev.c
index 6b9cbf2..494714b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -466,14 +466,11 @@  static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     }
 
     /* init */
-    blk = blk_new(qemu_opts_id(opts), errp);
+    blk = blk_new_with_bs(qemu_opts_id(opts), errp);
     if (!blk) {
         goto early_err;
     }
-    bs = bdrv_new_root(qemu_opts_id(opts), errp);
-    if (!bs) {
-        goto bdrv_new_err;
-    }
+    bs = blk_bs(blk);
     bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
     bs->read_only = ro;
     bs->detect_zeroes = detect_zeroes;
@@ -538,7 +535,6 @@  static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
 
 err:
     bdrv_unref(bs);
-bdrv_new_err:
     blk_unref(blk);
 early_err:
     qemu_opts_del(opts);
@@ -1774,16 +1770,18 @@  void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
+    BlockBackend *blk;
     BlockDriverState *bs;
     DriveInfo *dinfo;
     AioContext *aio_context;
     Error *local_err = NULL;
 
-    bs = bdrv_find(id);
-    if (!bs) {
+    blk = blk_by_name(id);
+    if (!blk) {
         error_report("Device '%s' not found", id);
         return -1;
     }
+    bs = blk_bs(blk);
 
     dinfo = drive_get_by_blockdev(bs);
     if (dinfo && !dinfo->enable_auto_del) {
@@ -1813,11 +1811,11 @@  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
      * then we can just get rid of the block driver state right here.
      */
     if (bdrv_get_attached_dev(bs)) {
-        bdrv_make_anon(bs);
-
+        blk_hide_on_behalf_of_do_drive_del(blk);
         /* Further I/O must not pause the guest */
         bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
                           BLOCKDEV_ON_ERROR_REPORT);
+        /* FIXME bs->blk leaked when bs dies */
     } else {
         drive_del(dinfo);
     }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 265cf13..0022083 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -860,15 +860,11 @@  static int blk_connect(struct XenDevice *xendev)
 
         /* setup via xenbus -> create new block driver instance */
         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
-        blk = blk_new(blkdev->dev, NULL);
+        blk = blk_new_with_bs(blkdev->dev, NULL);
         if (!blk) {
             return -1;
         }
-        blkdev->bs = bdrv_new_root(blkdev->dev, NULL);
-        if (!blkdev->bs) {
-            blk_unref(blk);
-            return -1;
-        }
+        blkdev->bs = blk_bs(blk);
 
         drv = bdrv_find_whitelisted_format(blkdev->fileproto, readonly);
         if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d86a6c..14e0b7c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -324,6 +324,8 @@  struct BlockDriverState {
     BlockDriver *drv; /* NULL means no media */
     void *opaque;
 
+    BlockBackend *blk;          /* owning backend, if any */
+
     void *dev;                  /* attached device model, if any */
     /* TODO change to DeviceState when all users are qdevified */
     const BlockDevOps *dev_ops;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 198062a..b97a794 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -17,10 +17,15 @@ 
 #include "qapi/error.h"
 
 BlockBackend *blk_new(const char *name, Error **errp);
+BlockBackend *blk_new_with_bs(const char *name, Error **errp);
 void blk_ref(BlockBackend *blk);
 void blk_unref(BlockBackend *blk);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 
+BlockDriverState *blk_bs(BlockBackend *blk);
+
+void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk);
+
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index bdb95a4..5548637 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -284,20 +284,19 @@  static int print_block_option_help(const char *filename, const char *fmt)
     return 0;
 }
 
-static BlockDriverState *bdrv_new_open(const char *id,
-                                       const char *filename,
-                                       const char *fmt,
-                                       int flags,
-                                       bool require_io,
-                                       bool quiet)
+static BlockBackend *img_open(const char *id, const char *filename,
+                              const char *fmt, int flags,
+                              bool require_io, bool quiet)
 {
+    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriver *drv;
     char password[256];
     Error *local_err = NULL;
     int ret;
 
-    bs = bdrv_new_root(id, &error_abort);
+    blk = blk_new_with_bs(id, &error_abort);
+    bs = blk_bs(blk);
 
     if (fmt) {
         drv = bdrv_find_format(fmt);
@@ -328,9 +327,10 @@  static BlockDriverState *bdrv_new_open(const char *id,
             goto fail;
         }
     }
-    return bs;
+    return blk;
 fail:
     bdrv_unref(bs);
+    blk_unref(blk);
     return NULL;
 }
 
@@ -580,7 +580,7 @@  static int img_check(int argc, char **argv)
     BlockDriverState *bs;
     int fix = 0;
     int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
-    ImageCheck *check = NULL;
+    ImageCheck *check;
     bool quiet = false;
 
     fmt = NULL;
@@ -651,12 +651,11 @@  static int img_check(int argc, char **argv)
         return 1;
     }
 
-    blk = blk_new("image", &error_abort);
-    bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
-    if (!bs) {
-        ret = 1;
-        goto fail;
+    blk = img_open("image", filename, fmt, flags, true, quiet);
+    if (!blk) {
+        return 1;
     }
+    bs = blk_bs(blk);
 
     check = g_new0(ImageCheck, 1);
     ret = collect_image_check(bs, check, filename, fmt, fix);
@@ -762,12 +761,12 @@  static int img_commit(int argc, char **argv)
         return 1;
     }
 
-    blk = blk_new("image", &error_abort);
-    bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
-    if (!bs) {
-        ret = -1;
-        goto out;
+    blk = img_open("image", filename, fmt, flags, true, quiet);
+    if (!blk) {
+        return 1;
     }
+    bs = blk_bs(blk);
+
     ret = bdrv_commit(bs);
     switch(ret) {
     case 0:
@@ -787,7 +786,6 @@  static int img_commit(int argc, char **argv)
         break;
     }
 
-out:
     bdrv_unref(bs);
     blk_unref(blk);
     if (ret) {
@@ -1022,21 +1020,21 @@  static int img_compare(int argc, char **argv)
         goto out3;
     }
 
-    blk1 = blk_new("image_1", &error_abort);
-    bs1 = bdrv_new_open("image_1", filename1, fmt1, flags, true, quiet);
-    if (!bs1) {
+    blk1 = img_open("image_1", filename1, fmt1, flags, true, quiet);
+    if (!blk1) {
         error_report("Can't open file %s", filename1);
         ret = 2;
-        goto out2;
+        goto out3;
     }
+    bs1 = blk_bs(blk1);
 
-    blk2 = blk_new("image_2", &error_abort);
-    bs2 = bdrv_new_open("image_2", filename2, fmt2, flags, true, quiet);
-    if (!bs2) {
+    blk2 = img_open("image_2", filename2, fmt2, flags, true, quiet);
+    if (!blk2) {
         error_report("Can't open file %s", filename2);
         ret = 2;
-        goto out1;
+        goto out2;
     }
+    bs2 = blk_bs(blk2);
 
     buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
     buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
@@ -1198,7 +1196,6 @@  static int img_compare(int argc, char **argv)
 out:
     qemu_vfree(buf1);
     qemu_vfree(buf2);
-out1:
     bdrv_unref(bs2);
     blk_unref(blk2);
 out2:
@@ -1379,15 +1376,15 @@  static int img_convert(int argc, char **argv)
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
         char *id = bs_n > 1 ? g_strdup_printf("source_%d", bs_i)
                             : g_strdup("source");
-        blk[bs_i] = blk_new(id, &error_abort);
-        bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, src_flags,
-                                 true, quiet);
+        blk[bs_i] = img_open(id, argv[optind + bs_i], fmt, src_flags,
+                             true, quiet);
         g_free(id);
-        if (!bs[bs_i]) {
+        if (!blk[bs_i]) {
             error_report("Could not open '%s'", argv[optind + bs_i]);
             ret = -1;
             goto out;
         }
+        bs[bs_i] = blk_bs(blk[bs_i]);
         bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
         if (bs_sectors[bs_i] < 0) {
             error_report("Could not get size of %s: %s",
@@ -1505,12 +1502,12 @@  static int img_convert(int argc, char **argv)
         goto out;
     }
 
-    out_blk = blk_new("target", &error_abort);
-    out_bs = bdrv_new_open("target", out_filename, out_fmt, flags, true, quiet);
-    if (!out_bs) {
+    out_blk = img_open("target", out_filename, out_fmt, flags, true, quiet);
+    if (!out_blk) {
         ret = -1;
         goto out;
     }
+    out_bs = blk_bs(out_blk);
 
     bs_i = 0;
     bs_offset = 0;
@@ -1895,13 +1892,12 @@  static ImageInfoList *collect_image_info_list(const char *filename,
         }
         g_hash_table_insert(filenames, (gpointer)filename, NULL);
 
-        blk = blk_new("image", &error_abort);
-        bs = bdrv_new_open("image", filename, fmt,
-                           BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false);
-        if (!bs) {
-            blk_unref(blk);
+        blk = img_open("image", filename, fmt,
+                       BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false);
+        if (!blk) {
             goto err;
         }
+        bs = blk_bs(blk);
 
         bdrv_query_image_info(bs, &info, &err);
         if (err) {
@@ -2161,12 +2157,11 @@  static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = blk_new("image", &error_abort);
-    bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS, true, false);
-    if (!bs) {
-        ret = -1;
-        goto out;
+    blk = img_open("image", filename, fmt, BDRV_O_FLAGS, true, false);
+    if (!blk) {
+        return 1;
     }
+    bs = blk_bs(blk);
 
     if (output_format == OFORMAT_HUMAN) {
         printf("%-16s%-16s%-16s%s\n", "Offset", "Length", "Mapped to", "File");
@@ -2285,12 +2280,11 @@  static int img_snapshot(int argc, char **argv)
     filename = argv[optind++];
 
     /* Open the image */
-    blk = blk_new("image", &error_abort);
-    bs = bdrv_new_open("image", filename, NULL, bdrv_oflags, true, quiet);
-    if (!bs) {
-        ret = -1;
-        goto out;
+    blk = img_open("image", filename, NULL, bdrv_oflags, true, quiet);
+    if (!blk) {
+        return 1;
     }
+    bs = blk_bs(blk);
 
     /* Perform the requested action */
     switch(action) {
@@ -2333,7 +2327,6 @@  static int img_snapshot(int argc, char **argv)
     }
 
     /* Cleanup */
-out:
     bdrv_unref(bs);
     blk_unref(blk);
     if (ret) {
@@ -2433,12 +2426,12 @@  static int img_rebase(int argc, char **argv)
      * Ignore the old backing file for unsafe rebase in case we want to correct
      * the reference to a renamed or moved backing file.
      */
-    blk = blk_new("image", &error_abort);
-    bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
-    if (!bs) {
+    blk = img_open("image", filename, fmt, flags, true, quiet);
+    if (!blk) {
         ret = -1;
         goto out;
     }
+    bs = blk_bs(blk);
 
     /* Find the right drivers for the backing files */
     old_backing_drv = NULL;
@@ -2466,8 +2459,8 @@  static int img_rebase(int argc, char **argv)
     if (!unsafe) {
         char backing_name[1024];
 
-        blk_old_backing = blk_new("old_backing", &error_abort);
-        bs_old_backing = bdrv_new_root("old_backing", &error_abort);
+        blk_old_backing = blk_new_with_bs("old_backing", &error_abort);
+        bs_old_backing = blk_bs(blk_old_backing);
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
         ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, src_flags,
                         old_backing_drv, &local_err);
@@ -2478,8 +2471,8 @@  static int img_rebase(int argc, char **argv)
             goto out;
         }
         if (out_baseimg[0]) {
-            blk_new_backing = blk_new("new_backing", &error_abort);
-            bs_new_backing = bdrv_new_root("new_backing", &error_abort);
+            blk_new_backing = blk_new_with_bs("new_backing", &error_abort);
+            bs_new_backing = blk_bs(blk_new_backing);
             ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL, src_flags,
                             new_backing_drv, &local_err);
             if (ret) {
@@ -2755,13 +2748,13 @@  static int img_resize(int argc, char **argv)
     n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
     qemu_opts_del(param);
 
-    blk = blk_new("image", &error_abort);
-    bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR,
-                       true, quiet);
-    if (!bs) {
+    blk = img_open("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR,
+                   true, quiet);
+    if (!blk) {
         ret = -1;
         goto out;
     }
+    bs = blk_bs(blk);
 
     if (relative) {
         total_size = bdrv_getlength(bs) + n * relative;
@@ -2873,13 +2866,13 @@  static int img_amend(int argc, char **argv)
         goto out;
     }
 
-    blk = blk_new("image", &error_abort);
-    bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
-    if (!bs) {
+    blk = img_open("image", filename, fmt, flags, true, quiet);
+    if (!blk) {
         error_report("Could not open image '%s'", filename);
         ret = -1;
         goto out;
     }
+    bs = blk_bs(blk);
 
     fmt = bs->drv->format_name;
 
diff --git a/qemu-io.c b/qemu-io.c
index a8dc973..8380734 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -62,8 +62,8 @@  static int openfile(char *name, int flags, int growable, QDict *opts)
         return 1;
     }
 
-    qemuio_blk = blk_new("hda", &error_abort);
-    qemuio_bs = bdrv_new_root("hda", &error_abort);
+    qemuio_blk = blk_new_with_bs("hda", &error_abort);
+    qemuio_bs = blk_bs(qemuio_blk);
 
     if (growable) {
         flags |= BDRV_O_PROTOCOL;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 634ba11..f741973 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -692,8 +692,8 @@  int main(int argc, char **argv)
         drv = NULL;
     }
 
-    blk = blk_new("hda", &error_abort);
-    bs = bdrv_new_root("hda", &error_abort);
+    blk = blk_new_with_bs("hda", &error_abort);
+    bs = blk_bs(blk);
 
     srcpath = argv[optind];
     ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);