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

[Marcel Apfelbaum]

On Thu, 2013-09-12 at 13:04 +0200, Markus Armbruster wrote:
> 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!
I'll try to finish it, thanks!
> 
> 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.
The outcome is that after hot-unplugging a device with bootindex=x,
the device still remains in the fw_boot_order and no device can 
be hot-plugged with the same bootindex 'x'. Furthermore, because
of the current issue Qemu quits on an operation that should succeed! 
I plan to send a patch to fix this too.

Thanks,
Marcel