On Tue, 13 Mar 2012 19:35:24 +0200 Alon Levy <al...@redhat.com> wrote:
> On Tue, Mar 13, 2012 at 12:59:17PM -0300, Luiz Capitulino wrote: > > On Tue, 13 Mar 2012 16:46:12 +0200 > > Alon Levy <al...@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 <al...@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. > > ok, I'll take it. Note that I'm going to not send the qxl screendump > behavior changing patch again, I think all alternatives to a real async > screendump suck, including libvirt changes, and the current behavior of > giving an old screendump is good enough hopefully for the real users, > which are: Fine with me, you can add a note in screendump's documentation in qapi-schema.json explaining qxl's behavior.
