Patchwork [3/5] blockdev: Reject multiple definitions for the same drive

login
register
mail settings
Submitter Markus Armbruster
Date Jan. 17, 2011, 6:31 p.m.
Message ID <1295289090-18236-4-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/79211/
State New
Headers show

Comments

Markus Armbruster - Jan. 17, 2011, 6:31 p.m.
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(-)
Kevin Wolf - Jan. 21, 2011, 2:33 p.m.
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
Markus Armbruster - Jan. 21, 2011, 4:58 p.m.
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...
Kevin Wolf - Jan. 21, 2011, 5:07 p.m.
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
Markus Armbruster - Jan. 21, 2011, 5:30 p.m.
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?
Kevin Wolf - Jan. 21, 2011, 6:13 p.m.
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
Markus Armbruster - Jan. 24, 2011, 3:36 p.m.
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!

[...]

Patch

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;
     }