Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 13 Mar 2012 16:46:12 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Tue, Mar 13, 2012 at 10:35:55AM -0300, Luiz Capitulino wrote:
> > On Sun, 11 Mar 2012 21:26:43 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > Passes the Monitor ptr to the screendump implementation to all for
> > > monitor suspend and resume for qxl to fix screendump regression.
> > > 
> > > graphics_console_init signature change required touching every
> > > implemented of screen_dump. There is no change other then an added
> > > parameter. qxl will make use of it in the next patch.
> > 
> > NACK on this one.
> > 
> > The Monitor object should be restricted to HMP. This patch spreads it to
> > what's going to be the QMP implementation of screendump.
> > 
> > The first step here should be to convert the screendump command to the qapi,
> > and lock the HMP shell in hmp_screendump().
> > 
> > However, this brings a new interesting problem: the HMP implementation is
> > actually a QMP client, meaning that it won't have a way to figure out
> > screendump completion either :)
> > 
> > Some solutions that come to my mind:
> > 
> >  1. Pool the screendump file creation from a timer.
> > 
> >     Cons: it may return before the file is fully written to disk
> > 
> 
> We know what the file size should be, so we can poll for the actual
> size. Actually why do we need to poll? we could add a
> "internal.screendump.complete" or "internal-query-screendump", no?

Neither is possible because hmp.c really uses QMP as client, except that it's
done via C.

> >  2. Use inotify
> > 
> >     Cons: what about windows?
> > 
> >  3. Introduce query-screendump that returns the last screendump status
> > 
> >     Cons: this is actually making screendump async
> > 
> > 
> > Anthony, do you have any ideas?
> > 
> > Btw, I've started doing the screendump conversion to the qapi, I'll post it
> > soon.
> 
> I've already sent patches once for a new qapi command, I don't mind you
> doing this of course.

It would actually help me if you do it, I have an initial version at:

 git://repo.or.cz/qemu/qmp-unstable.git qmp-wip/qapi-commands-conv/screendump/rfc

It's old and it's on top of some uninteresting stuff, the only commits that
matter there are 48e7c01b and 0f5509a8. But this rfc is just to have an idea
how the final command will look like.

From what I barely remember, I'd suggest you to do the following:

 1. Pass the Error object to hw_screen_dump()
 2. Convert the screendump command to the qapi
 3. Report errors from ppm_save() via Error

Important note: my code reports only QERR_OPEN_FILE_FAILED in ppm_save(), but
the right thing to do is to report most likely errors like EACCESS, ENOSPC,
EPERM, EIO etc.