Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Alon Levy]

On Mon, Oct 24, 2011 at 10:31:48PM -0200, Luiz Capitulino wrote:
> On Mon, 24 Oct 2011 19:29:37 +0200
> Alon Levy <address@hidden> wrote:
> 
> > On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> > > On Mon, 24 Oct 2011 17:13:14 +0200
> > > Gerd Hoffmann <address@hidden> wrote:
> > > 
> > > > On 10/24/11 14:02, Alon Levy wrote:
> > > > > Make screen_dump monitor command an async command to allow next for 
> > > > > qxl
> > > > > to implement it as a initiating call to red_worker and completion on
> > > > > callback, to fix a deadlock when issueing a screendump command via
> > > > > libvirt while connected with a libvirt controlled spice-gtk client.
> > > > 
> > > > Approach looks reasonable to me.  Patch breaks the build though, you've
> > > > missed a bunch of screen_dump functions in non-x86 targets.
> > > 
> > > There are two problems actually.
> > > 
> > > The first one is that changing an existing command from synchronous
> > > to asynchronous is an incompatible change because asynchronous commands
> > > semantics is different. For an example of possible problems please
> > > check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> > > 
> > > The second problem is that the existing asynchronous interface in the
> > > monitor is incomplete and has never been used for real. Our plan is to
> > > use QAPI's async support, but that has not landed in master yet and iirc
> > > there wasn't consensus about it. I also think it's a bit late for its
> > > inclusion in 1.0 (and certainly not a candidate for stable).
> > > 
> > > If all you need here is to delay sending the response, then maybe the
> > > current interface could work (although I honestly don't trust it and
> > > regret not having dropped it). Otherwise our only choice would be to
> > > work on getting the QAPI async support merged.
> > 
> > My problem is that the io thread keeps the global mutex during the wait,
> > that's why the async monitor is perfect for what I want - this is
> > exactly what it does.
> 
> Let's not mix internal implementation details with what we want as
> an external interface.
> 
> Can't you just make a vga_hw_screen_dump() specific callback?
> 

I don't understand how that would help - if the monitor command doesn't
return (normal sync operation) then the mutex is never dropped, and any
callback won't change that.

On the other hand, thinking a bit about the reference to 623903 baloon
bug, I don't see a problem: the change doesn't affect the semantics for
any other device except qxl, which I've tested. For any other device,
the only difference is that instead of:

do_screen_dump call
 device specific implementation
 return

It becomes

do_screen_dump call
 device specific implementation (not qxl)
  callback called (always - not conditional, no one stores it except
   qxl)
 return

> > I haven't looked at QAPI async support, but I
> > understand it's a bit in the future.
> 
> Yes, it's not for the immediate term.