diff mbox

[v5,04/23] block: Connect BlockBackend and DriveInfo

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

Commit Message

Markus Armbruster Oct. 2, 2014, 9:04 a.m. UTC 
Make the BlockBackend own the DriveInfo.  Change blockdev_init() to
return the BlockBackend instead of the DriveInfo.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   |  2 --
 block/block-backend.c     | 38 ++++++++++++++++++++++++++
 blockdev.c                | 69 ++++++++++++++++++++++++-----------------------
 include/sysemu/blockdev.h |  4 +++
 4 files changed, 77 insertions(+), 36 deletions(-)

Comments

Kevin Wolf Oct. 2, 2014, 10:55 a.m. UTC | #1 
Am 02.10.2014 um 11:04 hat Markus Armbruster geschrieben:
> Make the BlockBackend own the DriveInfo.  Change blockdev_init() to
> return the BlockBackend instead of the DriveInfo.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

> @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type)
>  
>  DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
>  {
> -    DriveInfo *dinfo;
> +    BlockBackend *blk;
>  
> -    QTAILQ_FOREACH(dinfo, &drives, next) {
> -        if (dinfo->bdrv == bs) {
> -            return dinfo;
> +    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> +        if (blk == bs->blk) {
> +            return blk_legacy_dinfo(blk);
>          }
>      }
>      return NULL;

In v3, I asked why you don't use bs->blk here. Apparently you understood
this as a suggestion to change the if condition from:

    if (blk_bs(blk) == bs)

to:

    if (blk == bs->blk)

Which isn't a wrong change, it just doesn't change a lot. What I really
meant is something like this, removing the loop:

    DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
    {
        return bs->blk ? blk_legacy_dinfo(bs->blk) : NULL;
    }

This would only behave differently if there were BlockBackends that can
be assigned to bs->blk, but aren't iterated by blk_next(). But such
BlockBackends don't exist, blk_next() includes all of them.

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

> Am 02.10.2014 um 11:04 hat Markus Armbruster geschrieben:
>> Make the BlockBackend own the DriveInfo.  Change blockdev_init() to
>> return the BlockBackend instead of the DriveInfo.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
>> @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type)
>>  
>>  DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
>>  {
>> -    DriveInfo *dinfo;
>> +    BlockBackend *blk;
>>  
>> -    QTAILQ_FOREACH(dinfo, &drives, next) {
>> -        if (dinfo->bdrv == bs) {
>> -            return dinfo;
>> +    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> +        if (blk == bs->blk) {
>> +            return blk_legacy_dinfo(blk);
>>          }
>>      }
>>      return NULL;
>
> In v3, I asked why you don't use bs->blk here. Apparently you understood
> this as a suggestion to change the if condition from:
>
>     if (blk_bs(blk) == bs)
>
> to:
>
>     if (blk == bs->blk)
>
> Which isn't a wrong change, it just doesn't change a lot. What I really
> meant is something like this, removing the loop:
>
>     DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
>     {
>         return bs->blk ? blk_legacy_dinfo(bs->blk) : NULL;
>     }
>
> This would only behave differently if there were BlockBackends that can
> be assigned to bs->blk, but aren't iterated by blk_next(). But such
> BlockBackends don't exist, blk_next() includes all of them.

Actually, there are: blk_hide_on_behalf_of_do_drive_del() removes from
blk_backends without also zapping bs->blk.

However, your version is *less* of a change, because it also doesn't
remove from drives.

In short: sold, thanks!
diff mbox

Patch

diff --git a/block.c b/block.c
index a25fae9..d6ab0c7 100644
--- a/block.c
+++ b/block.c
@@ -29,7 +29,6 @@ 
 #include "qemu/module.h"
 #include "qapi/qmp/qjson.h"
 #include "sysemu/sysemu.h"
-#include "sysemu/blockdev.h"    /* FIXME layering violation */
 #include "qemu/notify.h"
 #include "block/coroutine.h"
 #include "block/qapi.h"
@@ -2131,7 +2130,6 @@  static void bdrv_delete(BlockDriverState *bs)
     /* remove from list, if necessary */
     bdrv_make_anon(bs);
 
-    drive_info_del(drive_get_by_blockdev(bs));
     g_free(bs);
 }
 
diff --git a/block/block-backend.c b/block/block-backend.c
index a12215a..edaf73c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -12,11 +12,13 @@ 
 
 #include "sysemu/block-backend.h"
 #include "block/block_int.h"
+#include "sysemu/blockdev.h"
 
 struct BlockBackend {
     char *name;
     int refcnt;
     BlockDriverState *bs;
+    DriveInfo *legacy_dinfo;
     QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
 };
 
@@ -87,6 +89,7 @@  static void blk_delete(BlockBackend *blk)
         QTAILQ_REMOVE(&blk_backends, blk, link);
     }
     g_free(blk->name);
+    drive_info_del(blk->legacy_dinfo);
     g_free(blk);
 }
 
@@ -167,6 +170,41 @@  BlockDriverState *blk_bs(BlockBackend *blk)
 }
 
 /*
+ * Return @blk's DriveInfo if any, else null.
+ */
+DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
+{
+    return blk->legacy_dinfo;
+}
+
+/*
+ * Set @blk's DriveInfo to @dinfo, and return it.
+ * @blk must not have a DriveInfo set already.
+ * No other BlockBackend may have the same DriveInfo set.
+ */
+DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
+{
+    assert(!blk->legacy_dinfo);
+    return blk->legacy_dinfo = dinfo;
+}
+
+/*
+ * Return the BlockBackend with DriveInfo @dinfo.
+ * It must exist.
+ */
+BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
+{
+    BlockBackend *blk;
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        if (blk->legacy_dinfo == dinfo) {
+            return blk;
+        }
+    }
+    abort();
+}
+
+/*
  * Hide @blk.
  * @blk must not have been hidden already.
  * Make attached BlockDriverState, if any, anonymous.
diff --git a/blockdev.c b/blockdev.c
index 494714b..3a6bbe9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -47,8 +47,6 @@ 
 #include "trace.h"
 #include "sysemu/arch_init.h"
 
-static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
-
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
     [IF_IDE] = "ide",
@@ -89,7 +87,8 @@  static const int if_max_devs[IF_COUNT] = {
  */
 void blockdev_mark_auto_del(BlockDriverState *bs)
 {
-    DriveInfo *dinfo = drive_get_by_blockdev(bs);
+    BlockBackend *blk = bs->blk;
+    DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
     if (dinfo && !dinfo->enable_auto_del) {
         return;
@@ -105,7 +104,8 @@  void blockdev_mark_auto_del(BlockDriverState *bs)
 
 void blockdev_auto_del(BlockDriverState *bs)
 {
-    DriveInfo *dinfo = drive_get_by_blockdev(bs);
+    BlockBackend *blk = bs->blk;
+    DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
     if (dinfo && dinfo->auto_del) {
         drive_del(dinfo);
@@ -153,15 +153,15 @@  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
 {
+    BlockBackend *blk;
     DriveInfo *dinfo;
 
-    /* seek interface, bus and unit */
-
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        if (dinfo->type == type &&
-	    dinfo->bus == bus &&
-	    dinfo->unit == unit)
+    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+        dinfo = blk_legacy_dinfo(blk);
+        if (dinfo && dinfo->type == type
+            && dinfo->bus == bus && dinfo->unit == unit) {
             return dinfo;
+        }
     }
 
     return NULL;
@@ -177,13 +177,15 @@  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index)
 int drive_get_max_bus(BlockInterfaceType type)
 {
     int max_bus;
+    BlockBackend *blk;
     DriveInfo *dinfo;
 
     max_bus = -1;
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        if(dinfo->type == type &&
-           dinfo->bus > max_bus)
+    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+        dinfo = blk_legacy_dinfo(blk);
+        if (dinfo && dinfo->type == type && dinfo->bus > max_bus) {
             max_bus = dinfo->bus;
+        }
     }
     return max_bus;
 }
@@ -200,11 +202,11 @@  DriveInfo *drive_get_next(BlockInterfaceType type)
 
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
 {
-    DriveInfo *dinfo;
+    BlockBackend *blk;
 
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        if (dinfo->bdrv == bs) {
-            return dinfo;
+    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+        if (blk == bs->blk) {
+            return blk_legacy_dinfo(blk);
         }
     }
     return NULL;
@@ -217,10 +219,8 @@  static void bdrv_format_print(void *opaque, const char *name)
 
 void drive_del(DriveInfo *dinfo)
 {
-    BlockBackend *blk = blk_by_name(dinfo->id);
-
     bdrv_unref(dinfo->bdrv);
-    blk_unref(blk);
+    blk_unref(blk_by_legacy_dinfo(dinfo));
 }
 
 void drive_info_del(DriveInfo *dinfo)
@@ -230,7 +230,6 @@  void drive_info_del(DriveInfo *dinfo)
     }
     qemu_opts_del(dinfo->opts);
     g_free(dinfo->id);
-    QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo->serial);
     g_free(dinfo);
 }
@@ -302,8 +301,8 @@  static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
 typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
 
 /* Takes the ownership of bs_opts */
-static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
-                                Error **errp)
+static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
+                                   Error **errp)
 {
     const char *buf;
     int ro = 0;
@@ -486,7 +485,7 @@  static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     dinfo = g_malloc0(sizeof(*dinfo));
     dinfo->id = g_strdup(qemu_opts_id(opts));
     dinfo->bdrv = bs;
-    QTAILQ_INSERT_TAIL(&drives, dinfo, next);
+    blk_set_legacy_dinfo(blk, dinfo);
 
     if (!file || !*file) {
         if (has_driver_specific_opts) {
@@ -494,7 +493,7 @@  static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
         } else {
             QDECREF(bs_opts);
             qemu_opts_del(opts);
-            return dinfo;
+            return blk;
         }
     }
     if (snapshot) {
@@ -531,7 +530,7 @@  static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     QDECREF(bs_opts);
     qemu_opts_del(opts);
 
-    return dinfo;
+    return blk;
 
 err:
     bdrv_unref(bs);
@@ -644,6 +643,7 @@  QemuOptsList qemu_legacy_drive_opts = {
 DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 {
     const char *value;
+    BlockBackend *blk;
     DriveInfo *dinfo = NULL;
     QDict *bs_opts;
     QemuOpts *legacy_opts;
@@ -941,9 +941,9 @@  DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     }
 
     /* Actual block device init: Functionality shared with blockdev-add */
-    dinfo = blockdev_init(filename, bs_opts, &local_err);
+    blk = blockdev_init(filename, bs_opts, &local_err);
     bs_opts = NULL;
-    if (dinfo == NULL) {
+    if (!blk) {
         if (local_err) {
             error_report("%s", error_get_pretty(local_err));
             error_free(local_err);
@@ -954,6 +954,7 @@  DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     }
 
     /* Set legacy DriveInfo fields */
+    dinfo = blk_legacy_dinfo(blk);
     dinfo->enable_auto_del = true;
     dinfo->opts = all_opts;
 
@@ -1783,7 +1784,7 @@  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
     bs = blk_bs(blk);
 
-    dinfo = drive_get_by_blockdev(bs);
+    dinfo = blk_legacy_dinfo(blk);
     if (dinfo && !dinfo->enable_auto_del) {
         error_report("Deleting device added with blockdev-add"
                      " is not supported");
@@ -1815,7 +1816,6 @@  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
         /* 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);
     }
@@ -2509,7 +2509,7 @@  void qmp_change_backing_file(const char *device,
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
     QmpOutputVisitor *ov = qmp_output_visitor_new();
-    DriveInfo *dinfo;
+    BlockBackend *blk;
     QObject *obj;
     QDict *qdict;
     Error *local_err = NULL;
@@ -2547,14 +2547,15 @@  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     qdict_flatten(qdict);
 
-    dinfo = blockdev_init(NULL, qdict, &local_err);
+    blk = blockdev_init(NULL, qdict, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto fail;
     }
 
-    if (bdrv_key_required(dinfo->bdrv)) {
-        drive_del(dinfo);
+    if (bdrv_key_required(blk_bs(blk))) {
+        bdrv_unref(blk_bs(blk));
+        blk_unref(blk);
         error_setg(errp, "blockdev-add doesn't support encrypted devices");
         goto fail;
     }
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index abec381..1dc5906 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -45,6 +45,10 @@  struct DriveInfo {
     QTAILQ_ENTRY(DriveInfo) next;
 };
 
+DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
+DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo);
+BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
+
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);