[Qemu-devel] [PATCH 0/5] -drive/drive_add fixes

[Qemu-devel] [PATCH 0/5] -drive/drive_add fixes

[Markus Armbruster]

Note: PATCH 3/5 makes -drive reject duplicate definitions instead of
ignoring all but the first silently.  If this isn't sufficiently
bug-compatible for you, we need to talk.

Markus Armbruster (5):
  blockdev: Fix error message for invalid -drive CHS
  blockdev: Make drive_init() use error_report()
  blockdev: Reject multiple definitions for the same drive
  blockdev: Fix drive_del not to crash when drive is not in use
  blockdev: Fix drive_add for drives without media

 blockdev.c          |   88 ++++++++++++++++++++++++--------------------------
 blockdev.h          |    2 +-
 hw/device-hotplug.c |    3 +-
 hw/usb-msd.c        |    3 +-
 vl.c                |    6 +--
 5 files changed, 47 insertions(+), 55 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/5] blockdev: Fix error message for invalid -drive CHS

[Markus Armbruster]

When cyls, heads or secs are out of range, the error message prints
buf, which points to the value of option "if".  Bogus, may even be
null.  Drop that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d7add36..b01a87c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -224,15 +224,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if (cyls || heads || secs) {
         if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
-            fprintf(stderr, "qemu: '%s' invalid physical cyls number\n", buf);
+            fprintf(stderr, "qemu: invalid physical cyls number\n");
 	    return NULL;
 	}
         if (heads < 1 || (type == IF_IDE && heads > 16)) {
-            fprintf(stderr, "qemu: '%s' invalid physical heads number\n", buf);
+            fprintf(stderr, "qemu: invalid physical heads number\n");
 	    return NULL;
 	}
         if (secs < 1 || (type == IF_IDE && secs > 63)) {
-            fprintf(stderr, "qemu: '%s' invalid physical secs number\n", buf);
+            fprintf(stderr, "qemu: invalid physical secs number\n");
 	    return NULL;
 	}
     }
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/5] blockdev: Make drive_init() use error_report()

[Markus Armbruster]

This makes the errors point to the error location, and fixes drive_add
to report errors in the monitor instead of stderr.

While there, tweak a few error messages for consistency.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |   59 ++++++++++++++++++++++++++++-------------------------------
 1 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b01a87c..127c919 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -107,7 +107,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
 
 static void bdrv_format_print(void *opaque, const char *name)
 {
-    fprintf(stderr, " %s", name);
+    error_printf(" %s", name);
 }
 
 void drive_uninit(DriveInfo *dinfo)
@@ -129,8 +129,8 @@ static int parse_block_error_action(const char *buf, int is_read)
     } else if (!strcmp(buf, "report")) {
         return BLOCK_ERR_REPORT;
     } else {
-        fprintf(stderr, "qemu: '%s' invalid %s error action\n",
-            buf, is_read ? "read" : "write");
+        error_report("'%s' invalid %s error action",
+                     buf, is_read ? "read" : "write");
         return -1;
     }
 }
@@ -217,31 +217,30 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 	    type = IF_NONE;
             max_devs = 0;
 	} else {
-            fprintf(stderr, "qemu: unsupported bus type '%s'\n", buf);
+            error_report("unsupported bus type '%s'", buf);
             return NULL;
 	}
     }
 
     if (cyls || heads || secs) {
         if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
-            fprintf(stderr, "qemu: invalid physical cyls number\n");
+            error_report("invalid physical cyls number");
 	    return NULL;
 	}
         if (heads < 1 || (type == IF_IDE && heads > 16)) {
-            fprintf(stderr, "qemu: invalid physical heads number\n");
+            error_report("invalid physical heads number");
 	    return NULL;
 	}
         if (secs < 1 || (type == IF_IDE && secs > 63)) {
-            fprintf(stderr, "qemu: invalid physical secs number\n");
+            error_report("invalid physical secs number");
 	    return NULL;
 	}
     }
 
     if ((buf = qemu_opt_get(opts, "trans")) != NULL) {
         if (!cyls) {
-            fprintf(stderr,
-                    "qemu: '%s' trans must be used with cyls,heads and secs\n",
-                    buf);
+            error_report("'%s' trans must be used with cyls,heads and secs",
+                         buf);
             return NULL;
         }
         if (!strcmp(buf, "none"))
@@ -251,7 +250,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         else if (!strcmp(buf, "auto"))
             translation = BIOS_ATA_TRANSLATION_AUTO;
 	else {
-            fprintf(stderr, "qemu: '%s' invalid translation type\n", buf);
+            error_report("'%s' invalid translation type", buf);
 	    return NULL;
 	}
     }
@@ -261,13 +260,12 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 	    media = MEDIA_DISK;
 	} else if (!strcmp(buf, "cdrom")) {
             if (cyls || secs || heads) {
-                fprintf(stderr,
-                        "qemu: '%s' invalid physical CHS format\n", buf);
+                error_report("'%s' invalid physical CHS format", buf);
 	        return NULL;
             }
 	    media = MEDIA_CDROM;
 	} else {
-	    fprintf(stderr, "qemu: '%s' invalid media\n", buf);
+	    error_report("'%s' invalid media", buf);
 	    return NULL;
 	}
     }
@@ -283,7 +281,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         } else if (!strcmp(buf, "writethrough")) {
             /* this is the default */
         } else {
-           fprintf(stderr, "qemu: invalid cache option\n");
+           error_report("invalid cache option");
            return NULL;
         }
     }
@@ -295,7 +293,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         } else if (!strcmp(buf, "threads")) {
             /* this is the default */
         } else {
-           fprintf(stderr, "qemu: invalid aio option\n");
+           error_report("invalid aio option");
            return NULL;
         }
     }
@@ -303,14 +301,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if ((buf = qemu_opt_get(opts, "format")) != NULL) {
        if (strcmp(buf, "?") == 0) {
-            fprintf(stderr, "qemu: Supported formats:");
-            bdrv_iterate_format(bdrv_format_print, NULL);
-            fprintf(stderr, "\n");
-	    return NULL;
+           error_printf("Supported formats:");
+           bdrv_iterate_format(bdrv_format_print, NULL);
+           error_printf("\n");
+           return NULL;
         }
         drv = bdrv_find_whitelisted_format(buf);
         if (!drv) {
-            fprintf(stderr, "qemu: '%s' invalid format\n", buf);
+            error_report("'%s' invalid format", buf);
             return NULL;
         }
     }
@@ -318,7 +316,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     on_write_error = BLOCK_ERR_STOP_ENOSPC;
     if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
         if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
-            fprintf(stderr, "werror is not supported by this format\n");
+            error_report("werror is not supported by this bus type");
             return NULL;
         }
 
@@ -331,7 +329,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     on_read_error = BLOCK_ERR_REPORT;
     if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
         if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
-            fprintf(stderr, "rerror is not supported by this format\n");
+            error_report("rerror is not supported by this bus type");
             return NULL;
         }
 
@@ -343,7 +341,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if ((devaddr = qemu_opt_get(opts, "addr")) != NULL) {
         if (type != IF_VIRTIO) {
-            fprintf(stderr, "addr is not supported\n");
+            error_report("addr is not supported by this bus type");
             return NULL;
         }
     }
@@ -352,8 +350,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if (index != -1) {
         if (bus_id != 0 || unit_id != -1) {
-            fprintf(stderr,
-                    "qemu: index cannot be used with bus and unit\n");
+            error_report("index cannot be used with bus and unit");
             return NULL;
         }
         if (max_devs == 0)
@@ -384,8 +381,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     /* check unit id */
 
     if (max_devs && unit_id >= max_devs) {
-        fprintf(stderr, "qemu: unit %d too big (max is %d)\n",
-                unit_id, max_devs - 1);
+        error_report("unit %d too big (max is %d)",
+                     unit_id, max_devs - 1);
         return NULL;
     }
 
@@ -479,7 +476,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         ro = 1;
     } else if (ro == 1) {
         if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
-            fprintf(stderr, "qemu: readonly flag not supported for drive with this interface\n");
+            error_report("readonly not supported by this bus type");
             return NULL;
         }
     }
@@ -488,8 +485,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     ret = bdrv_open(dinfo->bdrv, file, bdrv_flags, drv);
     if (ret < 0) {
-        fprintf(stderr, "qemu: could not open disk image %s: %s\n",
-                        file, strerror(-ret));
+        error_report("could not open disk image %s: %s",
+                     file, strerror(-ret));
         return NULL;
     }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/5] blockdev: Reject multiple definitions for the same drive

[Markus Armbruster]

For reasons lost in the mist of time, we silently ignore multiple
definitions for the same drive:

    $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
    QEMU 0.13.50 monitor - type 'help' for more information
    (qemu) info block
    ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0

With if=none, this can become quite confusing:

    $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
    qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'

The second -device fails, because it refers to drive zwei, which got
silently ignored.

Make multiple drive definitions fail cleanly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 127c919..04a0e84 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -387,11 +387,12 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     }
 
     /*
-     * ignore multiple definitions
+     * catch multiple definitions
      */
 
     if (drive_get(type, bus_id, unit_id) != NULL) {
-        *fatal_error = 0;
+        error_report("drive with bus=%d, unit=%d (index=%d) exists",
+                     bus_id, unit_id, index);
         return NULL;
     }
 
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 3/5] blockdev: Reject multiple definitions for the same drive

[Kevin Wolf]

Am 17.01.2011 19:31, schrieb Markus Armbruster:
> For reasons lost in the mist of time, we silently ignore multiple
> definitions for the same drive:
> 
>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
>     QEMU 0.13.50 monitor - type 'help' for more information
>     (qemu) info block
>     ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0
> 
> With if=none, this can become quite confusing:
> 
>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
>     qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'
> 
> The second -device fails, because it refers to drive zwei, which got
> silently ignored.
> 
> Make multiple drive definitions fail cleanly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Dropped this one (and patch 5, which depends on it) from the block
branch again, it breaks -cdrom and probably other drives which are
created by default.

Kevin

[Qemu-devel] Re: [PATCH 3/5] blockdev: Reject multiple definitions for the same drive

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.01.2011 19:31, schrieb Markus Armbruster:
>> For reasons lost in the mist of time, we silently ignore multiple
>> definitions for the same drive:
>> 
>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
>>     QEMU 0.13.50 monitor - type 'help' for more information
>>     (qemu) info block
>>     ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0
>> 
>> With if=none, this can become quite confusing:
>> 
>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
>>     qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'
>> 
>> The second -device fails, because it refers to drive zwei, which got
>> silently ignored.
>> 
>> Make multiple drive definitions fail cleanly.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Dropped this one (and patch 5, which depends on it) from the block
> branch again, it breaks -cdrom and probably other drives which are
> created by default.

--verbose?

I was wondering what crap could depend on the crazy silent ignore...

[Qemu-devel] Re: [PATCH 3/5] blockdev: Reject multiple definitions for the same drive

[Kevin Wolf]

Am 21.01.2011 17:58, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 17.01.2011 19:31, schrieb Markus Armbruster:
>>> For reasons lost in the mist of time, we silently ignore multiple
>>> definitions for the same drive:
>>>
>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
>>>     QEMU 0.13.50 monitor - type 'help' for more information
>>>     (qemu) info block
>>>     ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0
>>>
>>> With if=none, this can become quite confusing:
>>>
>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
>>>     qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'
>>>
>>> The second -device fails, because it refers to drive zwei, which got
>>> silently ignored.
>>>
>>> Make multiple drive definitions fail cleanly.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Dropped this one (and patch 5, which depends on it) from the block
>> branch again, it breaks -cdrom and probably other drives which are
>> created by default.
> 
> --verbose?
> 
> I was wondering what crap could depend on the crazy silent ignore...

Just try using -cdrom and you'll see yourself.

>From what I understand, we always create the default device. If the user
has actually specified one, we still try to create the default device,
it fails and that failure was ignored until now (and with the patch
applied qemu aborts in this case).

Kevin

[Qemu-devel] Re: [PATCH 3/5] blockdev: Reject multiple definitions for the same drive

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 21.01.2011 17:58, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Am 17.01.2011 19:31, schrieb Markus Armbruster:
>>>> For reasons lost in the mist of time, we silently ignore multiple
>>>> definitions for the same drive:
>>>>
>>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
>>>>     QEMU 0.13.50 monitor - type 'help' for more information
>>>>     (qemu) info block
>>>>     ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0
>>>>
>>>> With if=none, this can become quite confusing:
>>>>
>>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
>>>>     qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'
>>>>
>>>> The second -device fails, because it refers to drive zwei, which got
>>>> silently ignored.
>>>>
>>>> Make multiple drive definitions fail cleanly.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> Dropped this one (and patch 5, which depends on it) from the block
>>> branch again, it breaks -cdrom and probably other drives which are
>>> created by default.
>> 
>> --verbose?
>> 
>> I was wondering what crap could depend on the crazy silent ignore...
>
> Just try using -cdrom and you'll see yourself.

Works for me.  Possibly due to some "it's late on Friday" stupidity on
my part.

>>From what I understand, we always create the default device. If the user
> has actually specified one, we still try to create the default device,
> it fails and that failure was ignored until now (and with the patch
> applied qemu aborts in this case).

Example command line for the mentally-challenged-on-Fridays?

[Qemu-devel] Re: [PATCH 3/5] blockdev: Reject multiple definitions for the same drive

[Kevin Wolf]

Am 21.01.2011 18:30, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 21.01.2011 17:58, schrieb Markus Armbruster:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Am 17.01.2011 19:31, schrieb Markus Armbruster:
>>>>> For reasons lost in the mist of time, we silently ignore multiple
>>>>> definitions for the same drive:
>>>>>
>>>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
>>>>>     QEMU 0.13.50 monitor - type 'help' for more information
>>>>>     (qemu) info block
>>>>>     ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0
>>>>>
>>>>> With if=none, this can become quite confusing:
>>>>>
>>>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
>>>>>     qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'
>>>>>
>>>>> The second -device fails, because it refers to drive zwei, which got
>>>>> silently ignored.
>>>>>
>>>>> Make multiple drive definitions fail cleanly.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>
>>>> Dropped this one (and patch 5, which depends on it) from the block
>>>> branch again, it breaks -cdrom and probably other drives which are
>>>> created by default.
>>>
>>> --verbose?
>>>
>>> I was wondering what crap could depend on the crazy silent ignore...
>>
>> Just try using -cdrom and you'll see yourself.
> 
> Works for me.  Possibly due to some "it's late on Friday" stupidity on
> my part.
> 
>> >From what I understand, we always create the default device. If the user
>> has actually specified one, we still try to create the default device,
>> it fails and that failure was ignored until now (and with the patch
>> applied qemu aborts in this case).
> 
> Example command line for the mentally-challenged-on-Fridays?

$ x86_64-softmmu/qemu-system-x86_64 -cdrom /dev/null
qemu-system-x86_64: drive with bus=1, unit=0 (index=2) exists

Kevin

[Qemu-devel] Re: [PATCH 3/5] blockdev: Reject multiple definitions for the same drive

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 21.01.2011 17:58, schrieb Markus Armbruster:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>> 
>>>> Am 17.01.2011 19:31, schrieb Markus Armbruster:
>>>>> For reasons lost in the mist of time, we silently ignore multiple
>>>>> definitions for the same drive:
>>>>>
>>>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
>>>>>     QEMU 0.13.50 monitor - type 'help' for more information
>>>>>     (qemu) info block
>>>>>     ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0
>>>>>
>>>>> With if=none, this can become quite confusing:
>>>>>
>>>>>     $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
>>>>>     qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'
>>>>>
>>>>> The second -device fails, because it refers to drive zwei, which got
>>>>> silently ignored.
>>>>>
>>>>> Make multiple drive definitions fail cleanly.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>
>>>> Dropped this one (and patch 5, which depends on it) from the block
>>>> branch again, it breaks -cdrom and probably other drives which are
>>>> created by default.
>>> 
>>> --verbose?
>>> 
>>> I was wondering what crap could depend on the crazy silent ignore...
>>
>> Just try using -cdrom and you'll see yourself.
>
> Works for me.  Possibly due to some "it's late on Friday" stupidity on
> my part.

Got it: it's my use of -nodefaults.  I hardly ever use QEMU without
it...

Thanks!

[...]

[Qemu-devel] [PATCH 4/5] blockdev: Fix drive_del not to crash when drive is not in use

[Markus Armbruster]

Watch this:

    (qemu) drive_add 0 if=none,file=tmp.img
    OK
    (qemu) info block
    none0: type=hd removable=0 file=tmp.img ro=0 drv=raw encrypted=0
    (qemu) drive_del none0
    Segmentation fault (core dumped)

do_drive_del()'s code to clean up the pointer from a qdev using the
drive back to the drive needs to check whether such a device exists.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 04a0e84..51a2da3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -682,13 +682,15 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
     /* clean up guest state from pointing to host resource by
      * finding and removing DeviceState "drive" property */
-    for (prop = bs->peer->info->props; prop && prop->name; prop++) {
-        if (prop->info->type == PROP_TYPE_DRIVE) {
-            ptr = qdev_get_prop_ptr(bs->peer, prop);
-            if ((*ptr) == bs) {
-                bdrv_detach(bs, bs->peer);
-                *ptr = NULL;
-                break;
+    if (bs->peer) {
+        for (prop = bs->peer->info->props; prop && prop->name; prop++) {
+            if (prop->info->type == PROP_TYPE_DRIVE) {
+                ptr = qdev_get_prop_ptr(bs->peer, prop);
+                if (*ptr == bs) {
+                    bdrv_detach(bs, bs->peer);
+                    *ptr = NULL;
+                    break;
+                }
             }
         }
     }
-- 
1.7.2.3

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

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH 0/5] -drive/drive_add fixes

[Stefan Hajnoczi]

On Mon, Jan 17, 2011 at 07:31:25PM +0100, Markus Armbruster wrote:
> Note: PATCH 3/5 makes -drive reject duplicate definitions instead of
> ignoring all but the first silently.  If this isn't sufficiently
> bug-compatible for you, we need to talk.
> 
> Markus Armbruster (5):
>   blockdev: Fix error message for invalid -drive CHS
>   blockdev: Make drive_init() use error_report()
>   blockdev: Reject multiple definitions for the same drive
>   blockdev: Fix drive_del not to crash when drive is not in use
>   blockdev: Fix drive_add for drives without media
> 
>  blockdev.c          |   88 ++++++++++++++++++++++++--------------------------
>  blockdev.h          |    2 +-
>  hw/device-hotplug.c |    3 +-
>  hw/usb-msd.c        |    3 +-
>  vl.c                |    6 +--
>  5 files changed, 47 insertions(+), 55 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH 0/5] -drive/drive_add fixes

[Kevin Wolf]

Am 17.01.2011 19:31, schrieb Markus Armbruster:
> Note: PATCH 3/5 makes -drive reject duplicate definitions instead of
> ignoring all but the first silently.  If this isn't sufficiently
> bug-compatible for you, we need to talk.
> 
> Markus Armbruster (5):
>   blockdev: Fix error message for invalid -drive CHS
>   blockdev: Make drive_init() use error_report()
>   blockdev: Reject multiple definitions for the same drive
>   blockdev: Fix drive_del not to crash when drive is not in use
>   blockdev: Fix drive_add for drives without media
> 
>  blockdev.c          |   88 ++++++++++++++++++++++++--------------------------
>  blockdev.h          |    2 +-
>  hw/device-hotplug.c |    3 +-
>  hw/usb-msd.c        |    3 +-
>  vl.c                |    6 +--
>  5 files changed, 47 insertions(+), 55 deletions(-)

Thanks, applied all to the block branch.

Kevin