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.

I understood that the boot order is passed in fw cfg and updated only once at
"machine done". There is no update of this list after this point.
Modifying the boot order from monitor does not work at all.

So in order to solve this issue we can:
1. Don't allow use of bootindex at hot-plug
2. Change the architecture so boot order changing during hot-plug will be 
possible.

> >> 
> >> > 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 really wanted to take this, but I think we need first to sort out
the issues mentioned above and only then follow this path

Thanks,
Marcel

> 
> 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.