[Qemu-devel] [PATCH 5/5] blockdev: Fix drive_add for drives without media

[Markus Armbruster <armbru@redhat.com>]

Watch this:

    (qemu) drive_add 0 if=none
    (qemu) info block
    none0: type=hd removable=0 [not inserted]
    (qemu) drive_del none0
    Segmentation fault (core dumped)

add_init_drive() is confused about drive_init()'s failure modes, and
cleans up when it shouldn't.  This leaves the DriveInfo with member
opts dangling.  drive_del attempts to free it, and dies.

drive_init() behaves as follows:

* If it created a drive with media, it returns its DriveInfo.

* If it created a drive without media, it clears *fatal_error and
  returns NULL.

* If it couldn't create a drive, it sets *fatal_error and returns
  NULL.

Of its three callers:

* drive_init_func() is correct.

* usb_msd_init() assumes drive_init() failed when it returns NULL.
  This is correct only because it always passes option "file", and
  "drive without media" can't happen then.

* add_init_drive() assumes drive_init() failed when it returns NULL.
  This is incorrect.

Clean up drive_init() to return NULL on failure and only on failure.
Drop its parameter fatal_error.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c          |    8 ++------
 blockdev.h          |    2 +-
 hw/device-hotplug.c |    3 +--
 hw/usb-msd.c        |    3 +--
 vl.c                |    6 ++----
 5 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 51a2da3..8a74bf3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -135,7 +135,7 @@ static int parse_block_error_action(const char *buf, int is_read)
     }
 }
 
-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
+DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 {
     const char *buf;
     const char *file = NULL;
@@ -157,8 +157,6 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     int snapshot = 0;
     int ret;
 
-    *fatal_error = 1;
-
     translation = BIOS_ATA_TRANSLATION_AUTO;
 
     if (default_to_scsi) {
@@ -463,8 +461,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         abort();
     }
     if (!file || !*file) {
-        *fatal_error = 0;
-        return NULL;
+        return dinfo;
     }
     if (snapshot) {
         /* always use cache=unsafe with snapshot */
@@ -493,7 +490,6 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if (bdrv_key_required(dinfo->bdrv))
         autostart = 0;
-    *fatal_error = 0;
     return dinfo;
 }
 
diff --git a/blockdev.h b/blockdev.h
index 4536b5c..38ee5d2 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -40,7 +40,7 @@ void drive_uninit(DriveInfo *dinfo);
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
 QemuOpts *drive_add(const char *file, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
-DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, int *fatal_error);
+DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
 
 /* device-hotplug */
 
diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 9704e2f..c310cb8 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -29,7 +29,6 @@
 
 DriveInfo *add_init_drive(const char *optstr)
 {
-    int fatal_error;
     DriveInfo *dinfo;
     QemuOpts *opts;
 
@@ -37,7 +36,7 @@ DriveInfo *add_init_drive(const char *optstr)
     if (!opts)
         return NULL;
 
-    dinfo = drive_init(opts, current_machine->use_scsi, &fatal_error);
+    dinfo = drive_init(opts, current_machine->use_scsi);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 0a95d8d..00f64b0 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -570,7 +570,6 @@ static USBDevice *usb_msd_init(const char *filename)
     QemuOpts *opts;
     DriveInfo *dinfo;
     USBDevice *dev;
-    int fatal_error;
     const char *p1;
     char fmt[32];
 
@@ -600,7 +599,7 @@ static USBDevice *usb_msd_init(const char *filename)
     qemu_opt_set(opts, "if", "none");
 
     /* create host drive */
-    dinfo = drive_init(opts, 0, &fatal_error);
+    dinfo = drive_init(opts, 0);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;
diff --git a/vl.c b/vl.c
index 0292184..5ffa12b 100644
--- a/vl.c
+++ b/vl.c
@@ -631,11 +631,9 @@ static int bt_parse(const char *opt)
 static int drive_init_func(QemuOpts *opts, void *opaque)
 {
     int *use_scsi = opaque;
-    int fatal_error = 0;
 
-    if (drive_init(opts, *use_scsi, &fatal_error) == NULL) {
-        if (fatal_error)
-            return 1;
+    if (drive_init(opts, *use_scsi) == NULL) {
+        return 1;
     }
     return 0;
 }
-- 
1.7.2.3