From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59348) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7WAp-0006fC-QK for qemu-devel@nongnu.org; Tue, 13 Mar 2012 14:10:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S7WAn-0000Q3-Ct for qemu-devel@nongnu.org; Tue, 13 Mar 2012 14:10:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57038) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7WAn-0000Pm-5W for qemu-devel@nongnu.org; Tue, 13 Mar 2012 14:10:13 -0400 Date: Tue, 13 Mar 2012 15:07:19 -0300 From: Luiz Capitulino Message-ID: <20120313150719.08171bf8@doriath.home> In-Reply-To: <20120313173524.GO27659@garlic.redhat.com> References: <1331483977-18910-1-git-send-email-alevy@redhat.com> <1331494004-26177-1-git-send-email-alevy@redhat.com> <1331494004-26177-5-git-send-email-alevy@redhat.com> <20120313103555.0ae4b834@doriath.home> <20120313144514.GK27659@garlic.redhat.com> <20120313125917.2b03a70c@doriath.home> <20120313173524.GO27659@garlic.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, kraxel@redhat.com On Tue, 13 Mar 2012 19:35:24 +0200 Alon Levy 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 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 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.