diff mbox

[05/23] block: Make BlockBackend own its BlockDriverState

Message ID 1410336832-22160-6-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Sept. 10, 2014, 8:13 a.m. UTC
On BlockBackend destruction, unref its BlockDriverState.  Replaces the
callers' unrefs.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/block-backend.c |  9 ++-------
 blockdev.c            | 11 +++--------
 hw/block/xen_disk.c   |  6 +++---
 qemu-img.c            | 35 +----------------------------------
 qemu-io.c             |  5 -----
 5 files changed, 9 insertions(+), 57 deletions(-)

Comments

Kevin Wolf Sept. 10, 2014, 10:14 a.m. UTC | #1
Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben:
> On BlockBackend destruction, unref its BlockDriverState.  Replaces the
> callers' unrefs.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/block-backend.c |  9 ++-------
>  blockdev.c            | 11 +++--------
>  hw/block/xen_disk.c   |  6 +++---
>  qemu-img.c            | 35 +----------------------------------
>  qemu-io.c             |  5 -----
>  5 files changed, 9 insertions(+), 57 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 2a22660..ae51f7f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -58,10 +58,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
>   * @errp: return location for an error to be set on failure, or %NULL
>   *
>   * Create a new BlockBackend, with a reference count of one, and
> - * attach a new BlockDriverState to it, also with a reference count of
> - * one.  Caller owns *both* references.
> - * TODO Let caller own only the BlockBackend reference
> - * Fail if @name already exists.
> + * a new BlockDriverState attached.  Fail if @name already exists.
>   *
>   * Returns: the BlockBackend on success, %NULL on error
>   */
> @@ -88,6 +85,7 @@ BlockBackend *blk_new_with_bs(const char *name, Error **errp)
>  static void blk_delete(BlockBackend *blk)
>  {
>      assert(!blk->refcnt);
> +    bdrv_unref(blk->bs);
>      blk_detach_bs(blk);

I think the bdrv_unref() should really be part of blk_detach_bs().

The same way it would be more logical to have bdrv_ref() as part of
blk_attach_bs(). For blk_new_with_bs() this might mean bdrv_new,
blk_attach_bs, bdrv_unref, which looks a bit odd, but if blk_attach_bs()
is ever called from somewhere else, it probably makes more sense (if it
isn't, it should be static).

Kevin
Markus Armbruster Sept. 11, 2014, 6:38 p.m. UTC | #2
Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben:
>> On BlockBackend destruction, unref its BlockDriverState.  Replaces the
>> callers' unrefs.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/block-backend.c |  9 ++-------
>>  blockdev.c            | 11 +++--------
>>  hw/block/xen_disk.c   |  6 +++---
>>  qemu-img.c            | 35 +----------------------------------
>>  qemu-io.c             |  5 -----
>>  5 files changed, 9 insertions(+), 57 deletions(-)
>> 
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 2a22660..ae51f7f 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -58,10 +58,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
>>   * @errp: return location for an error to be set on failure, or %NULL
>>   *
>>   * Create a new BlockBackend, with a reference count of one, and
>> - * attach a new BlockDriverState to it, also with a reference count of
>> - * one.  Caller owns *both* references.
>> - * TODO Let caller own only the BlockBackend reference
>> - * Fail if @name already exists.
>> + * a new BlockDriverState attached.  Fail if @name already exists.
>>   *
>>   * Returns: the BlockBackend on success, %NULL on error
>>   */
>> @@ -88,6 +85,7 @@ BlockBackend *blk_new_with_bs(const char *name, Error **errp)
>>  static void blk_delete(BlockBackend *blk)
>>  {
>>      assert(!blk->refcnt);
>> +    bdrv_unref(blk->bs);
>>      blk_detach_bs(blk);
>
> I think the bdrv_unref() should really be part of blk_detach_bs().
>
> The same way it would be more logical to have bdrv_ref() as part of
> blk_attach_bs(). For blk_new_with_bs() this might mean bdrv_new,
> blk_attach_bs, bdrv_unref, which looks a bit odd, but if blk_attach_bs()
> is ever called from somewhere else, it probably makes more sense (if it
> isn't, it should be static).

My thinking was that you usually want to acquire a reference when you
detach, and you're usually ready to give yours up when you attach.

However, I now think I got a use-after-free hidden around there.  I'll
look into it tomorrow with a fresh mind.  Might lead to me accepting
your suggestion without further ado :)
diff mbox

Patch

diff --git a/block/block-backend.c b/block/block-backend.c
index 2a22660..ae51f7f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -58,10 +58,7 @@  BlockBackend *blk_new(const char *name, Error **errp)
  * @errp: return location for an error to be set on failure, or %NULL
  *
  * Create a new BlockBackend, with a reference count of one, and
- * attach a new BlockDriverState to it, also with a reference count of
- * one.  Caller owns *both* references.
- * TODO Let caller own only the BlockBackend reference
- * Fail if @name already exists.
+ * a new BlockDriverState attached.  Fail if @name already exists.
  *
  * Returns: the BlockBackend on success, %NULL on error
  */
@@ -88,6 +85,7 @@  BlockBackend *blk_new_with_bs(const char *name, Error **errp)
 static void blk_delete(BlockBackend *blk)
 {
     assert(!blk->refcnt);
+    bdrv_unref(blk->bs);
     blk_detach_bs(blk);
     QTAILQ_REMOVE(&blk_backends, blk, link);
     g_free(blk->name);
@@ -110,9 +108,6 @@  void blk_ref(BlockBackend *blk)
  *
  * Decrement @blk's reference count.  If this drops it to zero,
  * destroy @blk.
- *
- * Does *not* touch the attached BlockDriverState's reference count.
- * TODO Decrement it!
  */
 void blk_unref(BlockBackend *blk)
 {
diff --git a/blockdev.c b/blockdev.c
index 73e2da9..791f6d9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -108,7 +108,7 @@  void blockdev_auto_del(BlockDriverState *bs)
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
     if (dinfo && dinfo->auto_del) {
-        drive_del(dinfo);
+        blk_unref(blk);
     }
 }
 
@@ -219,10 +219,7 @@  static void bdrv_format_print(void *opaque, const char *name)
 
 void drive_del(DriveInfo *dinfo)
 {
-    BlockBackend *blk = dinfo->bdrv->blk;
-
-    bdrv_unref(dinfo->bdrv);
-    blk_unref(blk);
+    blk_unref(dinfo->bdrv->blk);
 }
 
 typedef struct {
@@ -524,7 +521,6 @@  static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     return blk;
 
 err:
-    bdrv_unref(dinfo->bdrv);
     blk_unref(blk);
 early_err:
     qemu_opts_del(opts);
@@ -1778,7 +1774,7 @@  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
         bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
                           BLOCKDEV_ON_ERROR_REPORT);
     } else {
-        drive_del(drive_get_by_blockdev(bs));
+        blk_unref(blk);
     }
 
     aio_context_release(aio_context);
@@ -2515,7 +2511,6 @@  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
     }
 
     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/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 51f4f3a..6d474b9 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -870,7 +870,6 @@  static int blk_connect(struct XenDevice *xendev)
             xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
                           error_get_pretty(local_err));
             error_free(local_err);
-            bdrv_unref(blkdev->bs);
             blk_unref(blk);
             blkdev->bs = NULL;
             return -1;
@@ -886,7 +885,9 @@  static int blk_connect(struct XenDevice *xendev)
         }
         /* blkdev->bs is not create by us, we get a reference
          * so we can bdrv_unref() unconditionally */
-        bdrv_ref(blkdev->bs);
+        /* Except we don't bdrv_unref() anymore, we blk_unref().
+         * Conditionally, because we can't easily blk_ref() here.
+         * TODO Clean this up! */
     }
     bdrv_attach_dev_nofail(blkdev->bs, blkdev);
     blkdev->file_size = bdrv_getlength(blkdev->bs);
@@ -986,7 +987,6 @@  static void blk_disconnect(struct XenDevice *xendev)
 
     if (blkdev->bs) {
         bdrv_detach_dev(blkdev->bs, blkdev);
-        bdrv_unref(blkdev->bs);
         if (!blkdev->dinfo) {
             blk_unref(blk_by_name(blkdev->dev));
         }
diff --git a/qemu-img.c b/qemu-img.c
index 9fba5a1..083a8eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -329,7 +329,6 @@  static BlockBackend *img_open(const char *id, const char *filename,
     }
     return blk;
 fail:
-    bdrv_unref(bs);
     blk_unref(blk);
     return NULL;
 }
@@ -712,9 +711,7 @@  static int img_check(int argc, char **argv)
 
 fail:
     qapi_free_ImageCheck(check);
-    bdrv_unref(bs);
     blk_unref(blk);
-
     return ret;
 }
 
@@ -786,7 +783,6 @@  static int img_commit(int argc, char **argv)
         break;
     }
 
-    bdrv_unref(bs);
     blk_unref(blk);
     if (ret) {
         return 1;
@@ -1194,12 +1190,10 @@  static int img_compare(int argc, char **argv)
     ret = 0;
 
 out:
-    bdrv_unref(bs2);
     blk_unref(blk2);
     qemu_vfree(buf1);
     qemu_vfree(buf2);
 out2:
-    bdrv_unref(bs1);
     blk_unref(blk1);
 out3:
     qemu_progress_end();
@@ -1756,18 +1750,8 @@  out:
     if (sn_opts) {
         qemu_opts_del(sn_opts);
     }
-    if (out_bs) {
-        bdrv_unref(out_bs);
-    }
     blk_unref(out_blk);
-    if (bs) {
-        for (bs_i = 0; bs_i < bs_n; bs_i++) {
-            if (bs[bs_i]) {
-                bdrv_unref(bs[bs_i]);
-            }
-        }
-        g_free(bs);
-    }
+    g_free(bs);
     if (blk) {
         for (bs_i = 0; bs_i < bs_n; bs_i++) {
             blk_unref(blk[bs_i]);
@@ -1905,7 +1889,6 @@  static ImageInfoList *collect_image_info_list(const char *filename,
         if (err) {
             error_report("%s", error_get_pretty(err));
             error_free(err);
-            bdrv_unref(bs);
             blk_unref(blk);
             goto err;
         }
@@ -1915,7 +1898,6 @@  static ImageInfoList *collect_image_info_list(const char *filename,
         *last = elem;
         last = &elem->next;
 
-        bdrv_unref(bs);
         blk_unref(blk);
 
         filename = fmt = NULL;
@@ -2204,7 +2186,6 @@  static int img_map(int argc, char **argv)
     dump_map_entry(output_format, &curr, NULL);
 
 out:
-    bdrv_unref(bs);
     blk_unref(blk);
     return ret < 0;
 }
@@ -2329,7 +2310,6 @@  static int img_snapshot(int argc, char **argv)
     }
 
     /* Cleanup */
-    bdrv_unref(bs);
     blk_unref(blk);
     if (ret) {
         return 1;
@@ -2649,17 +2629,10 @@  out:
     qemu_progress_end();
     /* Cleanup */
     if (!unsafe) {
-        if (bs_old_backing != NULL) {
-            bdrv_unref(bs_old_backing);
-        }
         blk_unref(blk_old_backing);
-        if (bs_new_backing != NULL) {
-            bdrv_unref(bs_new_backing);
-        }
         blk_unref(blk_new_backing);
     }
 
-    bdrv_unref(bs);
     blk_unref(blk);
     if (ret) {
         return 1;
@@ -2785,9 +2758,6 @@  static int img_resize(int argc, char **argv)
         break;
     }
 out:
-    if (bs) {
-        bdrv_unref(bs);
-    }
     blk_unref(blk);
     if (ret) {
         return 1;
@@ -2899,9 +2869,6 @@  static int img_amend(int argc, char **argv)
     }
 
 out:
-    if (bs) {
-        bdrv_unref(bs);
-    }
     blk_unref(blk);
     qemu_opts_del(opts);
     qemu_opts_free(create_opts);
diff --git a/qemu-io.c b/qemu-io.c
index ef1d3ea..45e8167 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -38,7 +38,6 @@  static ReadLineState *readline_state;
 
 static int close_f(BlockDriverState *bs, int argc, char **argv)
 {
-    bdrv_unref(bs);
     blk_unref(qemuio_blk);
     qemuio_bs = NULL;
     qemuio_blk = NULL;
@@ -74,7 +73,6 @@  static int openfile(char *name, int flags, int growable, QDict *opts)
                 name ? " device " : "", name ?: "",
                 error_get_pretty(local_err));
         error_free(local_err);
-        bdrv_unref(qemuio_bs);
         blk_unref(qemuio_blk);
         qemuio_bs = NULL;
         qemuio_blk = NULL;
@@ -483,9 +481,6 @@  int main(int argc, char **argv)
      */
     bdrv_drain_all();
 
-    if (qemuio_bs) {
-        bdrv_unref(qemuio_bs);
-    }
     blk_unref(qemuio_blk);
     g_free(readline_state);
     return 0;