block: Connect BlockBackend to BlockDriverState

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.

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

9 files changed, 156 insertions, 94 deletions

diff --git a/block.c b/block.c
index 42659ecc71..e11a3ec275 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 e89caa9611..a12215a546 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 508188ea9f..12fabcc8d9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -279,7 +279,7 @@ static void bdrv_format_print(void *opaque, const char *name)
 
 void drive_del(DriveInfo *dinfo)
 {
-    BlockBackend *blk = blk_by_name(dinfo->id);
+    BlockBackend *blk = dinfo->bdrv->blk;
 
     bdrv_unref(dinfo->bdrv);
     blk_unref(blk);
@@ -528,14 +528,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;
@@ -600,7 +597,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);
@@ -1840,16 +1836,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) {
@@ -1879,8 +1877,7 @@ 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);
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 265cf13aa9..00220837e0 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 8d86a6c1eb..14e0b7c053 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 198062a00e..b97a794e45 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 bdb95a4a3f..5548637444 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 a8dc973ab7..8380734b08 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 634ba11260..f741973687 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);