[Qemu-devel] [PATCH 2/4] block: Keep DriveInfo alive until BlockDriverState dies

[Markus Armbruster]

If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not
the BDS.  This can happen in three places:

* Device model destruction during unplug: blockdev_auto_del()

* Xen IDE unplug: pci_piix3_xen_ide_unplug()

* drive_del command when no device model is attached: do_drive_del()

The other callers of drive_del are on error paths where refcnt == 1.

If the user somehow manages to plug in a device model using a BDS that
has gone through drive_del(), the legacy configuration passed in
DriveInfo doesn't reach the device model, and automatic deletion on
unplug doesn't work.  Worse, some device models such as scsi-disk
crash when DriveInfo doesn't exist.

This is theoretical; I didn't research an actual reproducer.

Fix by keeping DriveInfo alive until its BDS dies.

This affects qemu_drive_opts: now you can't reuse the same ID for new
drive options until the BDS dies.  Before, you could, but since the
code always attempts to create a BDS with the same ID next, the
enclosing operation "create a new drive" failed anyway.  Different
error path, same result.

Unfortunately, the fix involves use of blockdev.c stuff from block.c,
which is a layering violation.  Fortunately, my forthcoming
BlockBackend work will get rid of it again.

Signed-off-by: Markus Armbruster <address@hidden>
---
 block.c                   |  2 ++
 blockdev.c                | 13 ++++++++-----
 include/sysemu/blockdev.h |  1 +
 stubs/Makefile.objs       |  1 +
 stubs/blockdev.c          | 12 ++++++++++++
 5 files changed, 24 insertions(+), 5 deletions(-)
 create mode 100644 stubs/blockdev.c

diff --git a/block.c b/block.c
index d06dd51..6faf36f 100644
--- a/block.c
+++ b/block.c
@@ -29,6 +29,7 @@
 #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"
@@ -2110,6 +2111,7 @@ 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/blockdev.c b/blockdev.c
index 5ec4635..450f95c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -216,11 +216,17 @@ static void bdrv_format_print(void *opaque, const char 
*name)
 
 void drive_del(DriveInfo *dinfo)
 {
+    bdrv_unref(dinfo->bdrv);
+}
+
+void drive_info_del(DriveInfo *dinfo)
+{
+    if (!dinfo) {
+        return;
+    }
     if (dinfo->opts) {
         qemu_opts_del(dinfo->opts);
     }
-
-    bdrv_unref(dinfo->bdrv);
     g_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo->serial);
@@ -525,9 +531,6 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
 
 err:
     bdrv_unref(bs);
-    QTAILQ_REMOVE(&drives, dinfo, next);
-    g_free(dinfo->id);
-    g_free(dinfo);
 early_err:
     qemu_opts_del(opts);
 err_no_opts:
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 23a5d10..abec381 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -56,6 +56,7 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const 
char *file,
                     const char *optstr);
 DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
 void drive_del(DriveInfo *dinfo);
+void drive_info_del(DriveInfo *dinfo);
 
 /* device-hotplug */
 
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 5e347d0..c0b1f6a 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,6 @@
 stub-obj-y += arch-query-cpu-def.o
 stub-obj-y += bdrv-commit-all.o
+stub-obj-y += blockdev.o
 stub-obj-y += chr-baum-init.o
 stub-obj-y += chr-msmouse.o
 stub-obj-y += chr-testdev.o
diff --git a/stubs/blockdev.c b/stubs/blockdev.c
new file mode 100644
index 0000000..5d0a79c
--- /dev/null
+++ b/stubs/blockdev.c
@@ -0,0 +1,12 @@
+#include <assert.h>
+#include "sysemu/blockdev.h"
+
+DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
+{
+    return NULL;
+}
+
+void drive_info_del(DriveInfo *dinfo)
+{
+    assert(!dinfo);
+}
-- 
1.9.3