| Message ID | 1310478932-25370-14-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/12/11 15:55, Alon Levy wrote:
> Later the save will happen asynchronously on surface_updated callback.
Hmm. I can see why you are doing that. It makes the file being written
*after* the monitor command finishes though, which I think we should avoid.
cheers,
Gerd
On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > On 07/12/11 15:55, Alon Levy wrote: > >Later the save will happen asynchronously on surface_updated callback. > > Hmm. I can see why you are doing that. It makes the file being > written *after* the monitor command finishes though, which I think > we should avoid. I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? > > cheers, > Gerd >
On 07/13/11 11:29, Alon Levy wrote: > On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: >> On 07/12/11 15:55, Alon Levy wrote: >>> Later the save will happen asynchronously on surface_updated callback. >> >> Hmm. I can see why you are doing that. It makes the file being >> written *after* the monitor command finishes though, which I think >> we should avoid. > > I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? Not sure. Luiz, do we have async monitor commands meanwhile? Background: screendump for qxl vga can take a while as the spice-server might have to render everything first ... cheers, Gerd
On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > On 07/12/11 15:55, Alon Levy wrote: > >Later the save will happen asynchronously on surface_updated callback. > > Hmm. I can see why you are doing that. It makes the file being > written *after* the monitor command finishes though, which I think > we should avoid. Yes, that will very likely break applications using the screenshot command which expect to be able to load/read the whole image immediately after the screnshot command returns from the monitor Daniel
On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote: > On 07/13/11 11:29, Alon Levy wrote: > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > >>On 07/12/11 15:55, Alon Levy wrote: > >>>Later the save will happen asynchronously on surface_updated callback. > >> > >>Hmm. I can see why you are doing that. It makes the file being > >>written *after* the monitor command finishes though, which I think > >>we should avoid. > > > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? > > Not sure. Luiz, do we have async monitor commands meanwhile? > > Background: screendump for qxl vga can take a while as the > spice-server might have to render everything first ... Another option would be for the screenshot command to allow a pre-opened FD to be passed in. So libvirt would create a pipe(2) pass the write end of the pipe to the 'screenshot' command. The screenshot command can return from the monitor the moment it has validated that it can perform the screenshot. QEMU can then write data to the pipe in the background. libvirt (or equiv app) can likewise safely read data out of the pipe as it becomes available without any race condition. This kind of approach would actually fit better with what libvirt wants from a screenshot command, because we don't really want to have the screenshot saved to disk at all. Our API just provides applications with a stream to data the screenshot data from. Currently we create a temporary file, save the screenshot to it, and then immediately unlink it and stream data back to the app from the deleted file. If we could just use a anonymous pipe it would be much nicer Daniel
On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote: > On 07/13/11 11:29, Alon Levy wrote: > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > >>On 07/12/11 15:55, Alon Levy wrote: > >>>Later the save will happen asynchronously on surface_updated callback. > >> > >>Hmm. I can see why you are doing that. It makes the file being > >>written *after* the monitor command finishes though, which I think > >>we should avoid. > > > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? > > Not sure. Luiz, do we have async monitor commands meanwhile? > > Background: screendump for qxl vga can take a while as the > spice-server might have to render everything first ... I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty ugly. Also I guess what Daniel described is possible, but it changes the usage of screendump even more. Is turning do_screen_dump to async viable? I think I'll work on it. > > cheers, > Gerd >
Hi, > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty > ugly. Also I guess what Daniel described is possible, but it changes the usage of screendump > even more. Is turning do_screen_dump to async viable? I think I'll work on it. Daniel's suggestion is a nice option which goes on top ;) The filename-based screendump has to continue working for backward compatibility reasons. cheers, Gerd
On Wed, 13 Jul 2011 12:41:48 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > On 07/13/11 11:29, Alon Levy wrote: > > On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > >> On 07/12/11 15:55, Alon Levy wrote: > >>> Later the save will happen asynchronously on surface_updated callback. > >> > >> Hmm. I can see why you are doing that. It makes the file being > >> written *after* the monitor command finishes though, which I think > >> we should avoid. > > > > I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? > > Not sure. Luiz, do we have async monitor commands meanwhile? Not yet, this is a QAPI feature that should land soon, but it's not available yet. > > Background: screendump for qxl vga can take a while as the spice-server > might have to render everything first ... > > cheers, > Gerd >
On Wed, 13 Jul 2011 14:29:16 +0300 Alon Levy <alevy@redhat.com> wrote: > On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote: > > On 07/13/11 11:29, Alon Levy wrote: > > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > > >>On 07/12/11 15:55, Alon Levy wrote: > > >>>Later the save will happen asynchronously on surface_updated callback. > > >> > > >>Hmm. I can see why you are doing that. It makes the file being > > >>written *after* the monitor command finishes though, which I think > > >>we should avoid. > > > > > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? > > > > Not sure. Luiz, do we have async monitor commands meanwhile? > > > > Background: screendump for qxl vga can take a while as the > > spice-server might have to render everything first ... > > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty > ugly. IIRC, that interface doesn't work as expected and is going to be replaced by the QAPI's one. > Also I guess what Daniel described is possible, but it changes the usage of screendump > even more. Is turning do_screen_dump to async viable? I think I'll work on it. > > > > > cheers, > > Gerd > > >
On Wed, 13 Jul 2011 13:46:50 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty > > ugly. Also I guess what Daniel described is possible, but it changes the usage of screendump > > even more. Is turning do_screen_dump to async viable? I think I'll work on it. > > Daniel's suggestion is a nice option which goes on top ;) Some months ago it was suggested that we returned the screenshot as a base64 string. But this might have issues if the screenshot is/becomes too large, as we're imposing some limits on the json token size (I think it's 64MB today). Daniel's idea seems to be a good one though. > > The filename-based screendump has to continue working for backward > compatibility reasons. > > cheers, > Gerd >
On Wed, Jul 13, 2011 at 09:33:26AM -0300, Luiz Capitulino wrote: > On Wed, 13 Jul 2011 14:29:16 +0300 > Alon Levy <alevy@redhat.com> wrote: > > > On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote: > > > On 07/13/11 11:29, Alon Levy wrote: > > > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > > > >>On 07/12/11 15:55, Alon Levy wrote: > > > >>>Later the save will happen asynchronously on surface_updated callback. > > > >> > > > >>Hmm. I can see why you are doing that. It makes the file being > > > >>written *after* the monitor command finishes though, which I think > > > >>we should avoid. > > > > > > > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? > > > > > > Not sure. Luiz, do we have async monitor commands meanwhile? > > > > > > Background: screendump for qxl vga can take a while as the > > > spice-server might have to render everything first ... > > > > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty > > ugly. > > IIRC, that interface doesn't work as expected and is going to be replaced > by the QAPI's one. In what way is in wrong? it seems to work fine here - monitor is blocked until I call the callback, after which it returns. Didn't test the qmp part though. > > > Also I guess what Daniel described is possible, but it changes the usage of screendump > > even more. Is turning do_screen_dump to async viable? I think I'll work on it. > > > > > > > > cheers, > > > Gerd > > > > > >
On Wed, 13 Jul 2011 15:56:55 +0300 Alon Levy <alevy@redhat.com> wrote: > On Wed, Jul 13, 2011 at 09:33:26AM -0300, Luiz Capitulino wrote: > > On Wed, 13 Jul 2011 14:29:16 +0300 > > Alon Levy <alevy@redhat.com> wrote: > > > > > On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote: > > > > On 07/13/11 11:29, Alon Levy wrote: > > > > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote: > > > > >>On 07/12/11 15:55, Alon Levy wrote: > > > > >>>Later the save will happen asynchronously on surface_updated callback. > > > > >> > > > > >>Hmm. I can see why you are doing that. It makes the file being > > > > >>written *after* the monitor command finishes though, which I think > > > > >>we should avoid. > > > > > > > > > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok? > > > > > > > > Not sure. Luiz, do we have async monitor commands meanwhile? > > > > > > > > Background: screendump for qxl vga can take a while as the > > > > spice-server might have to render everything first ... > > > > > > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty > > > ugly. > > > > IIRC, that interface doesn't work as expected and is going to be replaced > > by the QAPI's one. > > In what way is in wrong? it seems to work fine here - monitor is blocked until > I call the callback, after which it returns. Didn't test the qmp part though. One problem is that the error is global and could be overwritten by other handlers. Another problem is that there's no way for a client to kill an async handler that got stuck, this could cause a client to wait for the handler forever. > > > > > > Also I guess what Daniel described is possible, but it changes the usage of screendump > > > even more. Is turning do_screen_dump to async viable? I think I'll work on it. > > > > > > > > > > > cheers, > > > > Gerd > > > > > > > > > >
On 07/13/11 14:32, Luiz Capitulino wrote: >> Not sure. Luiz, do we have async monitor commands meanwhile? > > Not yet, this is a QAPI feature that should land soon, but it's not > available yet. Hmm. Alon, is it an option to just leave the whole qxl-render stuff in sync mode for now and convert it later? Or will that have bad interactions with QXL_IO_UPDATE_AREA_ASYNC being used by the guest? cheers, Gerd
On Wed, Jul 13, 2011 at 03:45:16PM +0200, Gerd Hoffmann wrote: > On 07/13/11 14:32, Luiz Capitulino wrote: > >>Not sure. Luiz, do we have async monitor commands meanwhile? > > > >Not yet, this is a QAPI feature that should land soon, but it's not > >available yet. > > Hmm. Alon, is it an option to just leave the whole qxl-render stuff > in sync mode for now and convert it later? Or will that have bad > interactions with QXL_IO_UPDATE_AREA_ASYNC being used by the guest? > It's not a problem. I do have a working version using async monitor command, but I will gladly put it off if the monitor async is considered harmful. > cheers, > Gerd >
Hi, >> Hmm. Alon, is it an option to just leave the whole qxl-render stuff >> in sync mode for now and convert it later? Or will that have bad >> interactions with QXL_IO_UPDATE_AREA_ASYNC being used by the guest? >> > It's not a problem. I do have a working version using async monitor command, but > I will gladly put it off if the monitor async is considered harmful. Good, so we know the libspice-server API is sane. Lets put it off for now and resume later when QAPI and the other stuff is sorted, async i/o commands and s3/s4 support are more important now. cheers, Gerd
diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 60b822d..e64b646 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -70,6 +70,15 @@ void qxl_render_resize(PCIQXLDevice *qxl) } } +static void qxl_save_ppm(PCIQXLDevice *qxl) +{ + if (qxl->ppm_save_filename) { + ppm_save(qxl->ppm_save_filename, qxl->ssd.ds->surface); + free(qxl->ppm_save_filename); + qxl->ppm_save_filename = NULL; + } +} + void qxl_render_update(PCIQXLDevice *qxl) { VGACommonState *vga = &qxl->vga; @@ -139,6 +148,7 @@ void qxl_render_update(PCIQXLDevice *qxl) dirty[i].right - dirty[i].left, dirty[i].bottom - dirty[i].top); } + qxl_save_ppm(qxl); } static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor) diff --git a/hw/qxl.c b/hw/qxl.c index bd540c0..de93efa 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1462,8 +1462,8 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename) switch (qxl->mode) { case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: + qxl->ppm_save_filename = qemu_strdup(filename); qxl_render_update(qxl); - ppm_save(filename, qxl->ssd.ds->surface); break; case QXL_MODE_VGA: vga->screen_dump(vga, filename); diff --git a/hw/qxl.h b/hw/qxl.h index 064311a..2c7f94a 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -32,6 +32,8 @@ typedef struct PCIQXLDevice { int32_t num_memslots; int32_t num_surfaces; + char *ppm_save_filename; + #if SPICE_INTERFACE_QXL_MINOR >= 1 uint32_t current_async; QemuMutex async_lock;