Re: [Qemu-devel] [PATCH] qdev-monitor: Avoid exiting when hot-plugging two devices with the same bootindex value

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On Thu, 2013-09-12 at 11:43 +0200, Markus Armbruster wrote:
>> Paolo Bonzini <address@hidden> writes:
>> 
>> > Il 11/09/2013 20:26, Marcel Apfelbaum ha scritto:
>> >> Qemu is expected to quit if the same boot index value is used by
>> >> two devices.
>> >> However, hot-plugging a device with a bootindex value already used should
>> >> fail with a friendly message rather than quitting a running VM.
>> >
>> > I think the problem is right where QEMU exits, i.e. in
>> > add_boot_device_path.  This function should return an error instead, via
>> > an Error ** argument.
>> 
>> Agree.
>> 
>> > Callers, typically a device's init or realize function, will either
>> > print the error before returning an error code (e.g. -EBUSY for init) or
>> > propagate the error up (for realize).
>> >
>> > Returning/propagating failure will still cause QEMU to exit when the
>> > duplicate bootindexes are found on the command line.
>> 
>> I have an unfinished patch in my tree that does exactly that.  It's
>> unfinished, because cleanup on error paths needs work.  Current state
>> appended with FIXMEs and all.  Beware, the FIXMEs may not be correct and
>> are almost certainly not complete.
> Thanks Markus,
> Should I use it as my starting point and finish it or you intend to?

If you have cycles to spare, you're quite welcome to take this patch and
run with it!

You may have noticed that my patch moves the code to add the boot device
path in a few cases.  I did this in the hope of simplifying error paths.
Do not hesitate to undo such moves where they turn out not to simplify
anything.

Here's an issue that exists before my patch: DeviceClass method
unrealize() should clean up everything done by realize().  In
particular, unrealize() needs to remove any entry added to fw_boot_order
by realize() via add_boot_device_path().  Code to do that doesn't exist,
yet.  Hot unplug is technically broken for all devices with bootindex.
Impact unknown.  Should probably be fixed in a separate patch.