block: Keep DriveInfo alive until BlockDriverState dies

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. The problem
was introduced when we replaced DriveInfo reference counting by BDS
reference counting in commit a94a3fa..fa510eb.

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 <[email protected]>
Reviewed-by: Max Reitz <[email protected]>
Reviewed-by: Benoît Canet <[email protected]>
Signed-off-by: Kevin Wolf <[email protected]>

diff --git a/block.c b/block.c
index a857913fc28a3c694106df76e95159cebd9c9b89..0c4540c060eac37a3928d6e79d20d7ad36b1717f 100644 (file)
--- 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 5ec4635437278a97b9bf39ab0f5be5f219a75240..450f95cbe758c336304dc6a4a21b9294f07a444a 100644 (file)
--- 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 23a5d10c68eb0221b110f4c0626e804e452c6a94..abec3810493d32f77aaaced959ab30c553f03906 100644 (file)
--- 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 5e347d04bd3c3e1ffc89273f466d7298e3982ede..c0b1f6a320f0c8f5f668952f4a8d4f4aa1b45162 100644 (file)
--- 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 (file)
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);
+}