| Message ID | 1329686886-6853-6-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/19/12 22:28, Alon Levy wrote: > This changes the behavior of the monitor command. After the previous > patch, there is no longer an option of deadlock with virt-manager, but > ppm_save is called too early, before the update has completed. With this > patch it is called at the correct moment, but that means there is a race > between the monitor command completing and the screendump file being saved. > > The only solution is to use an asynchronous monitor command. For a > previous round of this see: > http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02810.html > > Since that's contentious, I'm suggesting we do something that is almost > correct and doesn't hang, instead of correct and hangs. The screendump > user can inotify on the directory and the file if need be. For casual > monitor usage there is no difference. Hmm, that is pretty lame. There are users like autotest which expect the screen dump being there when the monitor command is finished, that change will break them. Unfortunaly there is no easy way out. I think the options are: (1) Keep existing behavior. That means the screenshot might show old screen content. Not very nice too. Would work sort-of ok for autotest though as autotest does screenshots every second and thus the screen content wouldn't be older than a second. (2) Async monitor command. Keeps interface and works nicely. A bunch of QAPI bits tickled into master meanwhile, so we could look at this again. Luiz? What is the status here? (3) Something like this patch + additionally introduce a "your-screenshot-is-finished-now" qmp event. Will break existing users too. But at least they can be adapted without requiring some external, nonportable service like inotify ... cheers, Gerd
On Mon, Feb 20, 2012 at 12:32:44PM +0100, Gerd Hoffmann wrote: > On 02/19/12 22:28, Alon Levy wrote: > > This changes the behavior of the monitor command. After the previous > > patch, there is no longer an option of deadlock with virt-manager, but > > ppm_save is called too early, before the update has completed. With this > > patch it is called at the correct moment, but that means there is a race > > between the monitor command completing and the screendump file being saved. > > > > The only solution is to use an asynchronous monitor command. For a > > previous round of this see: > > http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02810.html > > > > Since that's contentious, I'm suggesting we do something that is almost > > correct and doesn't hang, instead of correct and hangs. The screendump > > user can inotify on the directory and the file if need be. For casual > > monitor usage there is no difference. > > Hmm, that is pretty lame. There are users like autotest which expect > the screen dump being there when the monitor command is finished, that > change will break them. > > Unfortunaly there is no easy way out. I think the options are: > > (1) Keep existing behavior. That means the screenshot might show old > screen content. Not very nice too. Would work sort-of ok for > autotest though as autotest does screenshots every second and thus > the screen content wouldn't be older than a second. > > (2) Async monitor command. Keeps interface and works nicely. A bunch > of QAPI bits tickled into master meanwhile, so we could look at > this again. Luiz? What is the status here? > > (3) Something like this patch + additionally introduce a > "your-screenshot-is-finished-now" qmp event. Will break existing > users too. But at least they can be adapted without requiring > some external, nonportable service like inotify ... > I was going to look for QAPI bits after this series (i.e. (2)). Doing (3) is also possible. > cheers, > Gerd >
Hi, >> (2) Async monitor command. Keeps interface and works nicely. A bunch >> of QAPI bits tickled into master meanwhile, so we could look at >> this again. Luiz? What is the status here? > > I was going to look for QAPI bits after this series (i.e. (2)). Doing > (3) is also possible. Good. I'd just leave it as-is then for now. cheers, Gerd
On 02/20/2012 04:32 AM, Gerd Hoffmann wrote: > Hmm, that is pretty lame. There are users like autotest which expect > the screen dump being there when the monitor command is finished, that > change will break them. Libvirt is another such user. > > Unfortunaly there is no easy way out. I think the options are: > > (1) Keep existing behavior. That means the screenshot might show old > screen content. Not very nice too. Would work sort-of ok for > autotest though as autotest does screenshots every second and thus > the screen content wouldn't be older than a second. > > (2) Async monitor command. Keeps interface and works nicely. A bunch > of QAPI bits tickled into master meanwhile, so we could look at > this again. Luiz? What is the status here? > > (3) Something like this patch + additionally introduce a > "your-screenshot-is-finished-now" qmp event. Will break existing > users too. But at least they can be adapted without requiring > some external, nonportable service like inotify ... Libvirt would want 3) - any command that becomes async also needs an event to tell us when the command is completed, so that libvirt can maintain the synchronous interface to the user (and/or expose a new flag to allow the user to also benefit from the asynchronous command).
On Mon, Feb 20, 2012 at 02:29:11PM -0700, Eric Blake wrote: > On 02/20/2012 04:32 AM, Gerd Hoffmann wrote: > > Hmm, that is pretty lame. There are users like autotest which expect > > the screen dump being there when the monitor command is finished, that > > change will break them. > > Libvirt is another such user. > > > > > Unfortunaly there is no easy way out. I think the options are: > > > > (1) Keep existing behavior. That means the screenshot might show old > > screen content. Not very nice too. Would work sort-of ok for > > autotest though as autotest does screenshots every second and thus > > the screen content wouldn't be older than a second. > > > > (2) Async monitor command. Keeps interface and works nicely. A bunch > > of QAPI bits tickled into master meanwhile, so we could look at > > this again. Luiz? What is the status here? > > > > (3) Something like this patch + additionally introduce a > > "your-screenshot-is-finished-now" qmp event. Will break existing > > users too. But at least they can be adapted without requiring > > some external, nonportable service like inotify ... > > Libvirt would want 3) - any command that becomes async also needs an > event to tell us when the command is completed, so that libvirt can > maintain the synchronous interface to the user (and/or expose a new flag > to allow the user to also benefit from the asynchronous command). If I do 2) then libvirt won't notice because the monitor command will block as usual. Only change would be internal, qemu would continue processing other fds in the interim. > > -- > Eric Blake eblake@redhat.com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 02/21/2012 01:19 AM, Alon Levy wrote: >>> (2) Async monitor command. Keeps interface and works nicely. A bunch >>> of QAPI bits tickled into master meanwhile, so we could look at >>> this again. Luiz? What is the status here? >>> >>> (3) Something like this patch + additionally introduce a >>> "your-screenshot-is-finished-now" qmp event. Will break existing >>> users too. But at least they can be adapted without requiring >>> some external, nonportable service like inotify ... >> >> Libvirt would want 3) - any command that becomes async also needs an >> event to tell us when the command is completed, so that libvirt can >> maintain the synchronous interface to the user (and/or expose a new flag >> to allow the user to also benefit from the asynchronous command). > > If I do 2) then libvirt won't notice because the monitor command will > block as usual. Only change would be internal, qemu would continue > processing other fds in the interim. I guess I misunderstood things then. I was assuming that an async monitor command would mean that the monitor command would return control to libvirt prior to the screenshot file being completely written; but libvirt promises a synchronous interface to callers (that is, a caller using virDomainScreenshot won't get a response until the screenshot file has started streaming, but that means the file had better be consistent, and not something that gets modified after libvirt has streamed it to the caller). I don't particularly care which solution we have, as long as the overall result is still something where libvirt has guarantees that the complete screenshot file is available before libvirt then hands control of that file back to the caller.
On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote: > On 02/21/2012 01:19 AM, Alon Levy wrote: > > >>> (2) Async monitor command. Keeps interface and works nicely. A bunch > >>> of QAPI bits tickled into master meanwhile, so we could look at > >>> this again. Luiz? What is the status here? > >>> > >>> (3) Something like this patch + additionally introduce a > >>> "your-screenshot-is-finished-now" qmp event. Will break existing > >>> users too. But at least they can be adapted without requiring > >>> some external, nonportable service like inotify ... > >> > >> Libvirt would want 3) - any command that becomes async also needs an > >> event to tell us when the command is completed, so that libvirt can > >> maintain the synchronous interface to the user (and/or expose a new flag > >> to allow the user to also benefit from the asynchronous command). > > > > If I do 2) then libvirt won't notice because the monitor command will > > block as usual. Only change would be internal, qemu would continue > > processing other fds in the interim. > > I guess I misunderstood things then. I was assuming that an async > monitor command would mean that the monitor command would return control > to libvirt prior to the screenshot file being completely written; but > libvirt promises a synchronous interface to callers (that is, a caller > using virDomainScreenshot won't get a response until the screenshot file > has started streaming, but that means the file had better be consistent, > and not something that gets modified after libvirt has streamed it to > the caller). I don't particularly care which solution we have, as long > as the overall result is still something where libvirt has guarantees > that the complete screenshot file is available before libvirt then hands > control of that file back to the caller. Yes, that's the misunderstanding I think everyone is liable to have because it is called "Asyncronous", but in actuallity the implementation of an async monitor command is just what I mentioned: internal to qemu the main thread select loop continuous to run until a callback completes the monitor command. The monitor is suspended during that time, so no change to the monitor user (i.e. libvirt) is visible. Luiz said that this interface is going to be dropped, so we don't want to introduce another user to it now. So the question becomes if there is something equivalent. I totally agree the name "async monitor" should be resereved for the behavior you describe above, and not for the one I just described. In that case there may still be room for an improvement to the monitor commands, maybe only selectively used to not force a lot of code churn, that will allow any command that requires some long computation / operation to take place before returning to the caller (synchronously from it's point of view), while still being responsive by handling any other callbacks that have nothing to do with the monitor in the mean time. Something identical in practice to what is correcntly called "async monitor". > > -- > Eric Blake eblake@redhat.com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On Tue, 21 Feb 2012 19:40:16 +0200 Alon Levy <alevy@redhat.com> wrote: > On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote: > > On 02/21/2012 01:19 AM, Alon Levy wrote: > > > > >>> (2) Async monitor command. Keeps interface and works nicely. A bunch > > >>> of QAPI bits tickled into master meanwhile, so we could look at > > >>> this again. Luiz? What is the status here? The qapi infra is already in place for sometime now, but it doesn't support async commands yet. However, we're accepting a combination of command + async event on completion for commands that have to be async. We'll eventually have a good interface for async support, but it's not likely we'll have it for 1.1 (possible, but unlikely). I think item 2 above is a good way to go, considering it will add a new command, of course. > Luiz said that this interface is going to be dropped, so we don't want > to introduce another user to it now. Please don't :)
On Wed, Feb 22, 2012 at 11:17:17AM -0200, Luiz Capitulino wrote: > On Tue, 21 Feb 2012 19:40:16 +0200 > Alon Levy <alevy@redhat.com> wrote: > > > On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote: > > > On 02/21/2012 01:19 AM, Alon Levy wrote: > > > > > > >>> (2) Async monitor command. Keeps interface and works nicely. A bunch > > > >>> of QAPI bits tickled into master meanwhile, so we could look at > > > >>> this again. Luiz? What is the status here? > > The qapi infra is already in place for sometime now, but it doesn't support > async commands yet. However, we're accepting a combination of command + async > event on completion for commands that have to be async. > > We'll eventually have a good interface for async support, but it's not likely > we'll have it for 1.1 (possible, but unlikely). > > I think item 2 above is a good way to go, considering it will add a new command, > of course. > Ok, so that sounds good: I'll add an event, and later libvirt/autotest can use it. But in that case I'll need to introduce a connection between the command and the event, some id. That id will have to be generated by the command issuer, so a new command with event id + complete event? > > Luiz said that this interface is going to be dropped, so we don't want > > to introduce another user to it now. > > Please don't :) >
On Wed, 22 Feb 2012 14:22:50 +0100 Alon Levy <alevy@redhat.com> wrote: > On Wed, Feb 22, 2012 at 11:17:17AM -0200, Luiz Capitulino wrote: > > On Tue, 21 Feb 2012 19:40:16 +0200 > > Alon Levy <alevy@redhat.com> wrote: > > > > > On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote: > > > > On 02/21/2012 01:19 AM, Alon Levy wrote: > > > > > > > > >>> (2) Async monitor command. Keeps interface and works nicely. A bunch > > > > >>> of QAPI bits tickled into master meanwhile, so we could look at > > > > >>> this again. Luiz? What is the status here? > > > > The qapi infra is already in place for sometime now, but it doesn't support > > async commands yet. However, we're accepting a combination of command + async > > event on completion for commands that have to be async. > > > > We'll eventually have a good interface for async support, but it's not likely > > we'll have it for 1.1 (possible, but unlikely). > > > > I think item 2 above is a good way to go, considering it will add a new command, > > of course. > > > > Ok, so that sounds good: I'll add an event, and later libvirt/autotest > can use it. But in that case I'll need to introduce a connection between > the command and the event, some id. That id will have to be generated by > the command issuer, so a new command with event id + complete event? That's a good question. The only events we have today which are actually a response to an asynchronous command are the block streaming API ones and they don't include an id. Honestly, for this particular case, I'm not 100% sure that having an id is _required_, as I don't expect a client to submit multiple screendump calls in parallel and we don't "officially" support multiple QMP clients either. Also, having the screendump filename in the event will serve as a form of identifier too. Btw, are you planning to add the event to the already existing screendump command? Adding a new command that doesn't use the monitor async API and is truly asynchronous wouldn't better?
Hi, > Honestly, for this particular case, I'm not 100% sure that having an id is > _required_, as I don't expect a client to submit multiple screendump calls > in parallel and we don't "officially" support multiple QMP clients either. > Also, having the screendump filename in the event will serve as a form of > identifier too. That is exactly my thinking, echo the filename written in the event. > Btw, are you planning to add the event to the already existing screendump > command? Adding a new command that doesn't use the monitor async API and > is truly asynchronous wouldn't better? Good question. I'd tend to just let the existing command send trigger an event. But libvirt needs some way to figure whenever it should wait for an event ... cheers, Gerd
On Wed, Feb 22, 2012 at 11:49:27AM -0200, Luiz Capitulino wrote: > On Wed, 22 Feb 2012 14:22:50 +0100 > Alon Levy <alevy@redhat.com> wrote: > > > On Wed, Feb 22, 2012 at 11:17:17AM -0200, Luiz Capitulino wrote: > > > On Tue, 21 Feb 2012 19:40:16 +0200 > > > Alon Levy <alevy@redhat.com> wrote: > > > > > > > On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote: > > > > > On 02/21/2012 01:19 AM, Alon Levy wrote: > > > > > > > > > > >>> (2) Async monitor command. Keeps interface and works nicely. A bunch > > > > > >>> of QAPI bits tickled into master meanwhile, so we could look at > > > > > >>> this again. Luiz? What is the status here? > > > > > > The qapi infra is already in place for sometime now, but it doesn't support > > > async commands yet. However, we're accepting a combination of command + async > > > event on completion for commands that have to be async. > > > > > > We'll eventually have a good interface for async support, but it's not likely > > > we'll have it for 1.1 (possible, but unlikely). > > > > > > I think item 2 above is a good way to go, considering it will add a new command, > > > of course. > > > > > > > Ok, so that sounds good: I'll add an event, and later libvirt/autotest > > can use it. But in that case I'll need to introduce a connection between > > the command and the event, some id. That id will have to be generated by > > the command issuer, so a new command with event id + complete event? > > That's a good question. > > The only events we have today which are actually a response to an asynchronous > command are the block streaming API ones and they don't include an id. > > Honestly, for this particular case, I'm not 100% sure that having an id is > _required_, as I don't expect a client to submit multiple screendump calls > in parallel and we don't "officially" support multiple QMP clients either. > Also, having the screendump filename in the event will serve as a form of > identifier too. > > Btw, are you planning to add the event to the already existing screendump > command? Adding a new command that doesn't use the monitor async API and > is truly asynchronous wouldn't better? I was thinking to add a new command since I'll want to add the id, and if I'm already adding a new command I'll put in a display number too.
On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote: > Hi, > > > Honestly, for this particular case, I'm not 100% sure that having an id is > > _required_, as I don't expect a client to submit multiple screendump calls > > in parallel and we don't "officially" support multiple QMP clients either. > > Also, having the screendump filename in the event will serve as a form of > > identifier too. > > That is exactly my thinking, echo the filename written in the event. > > > Btw, are you planning to add the event to the already existing screendump > > command? Adding a new command that doesn't use the monitor async API and > > is truly asynchronous wouldn't better? > > Good question. I'd tend to just let the existing command send trigger > an event. But libvirt needs some way to figure whenever it should wait > for an event ... Right, that's the second reason I think a new command is needed. Additionally a new command can be implemented only by qxl and not by anything else (although I guess that would be a NACK?) > > cheers, > Gerd
Hi, > I was thinking to add a new command since I'll want to add the id, and > if I'm already adding a new command I'll put in a display number too. Big question is what the display number is supposed to be ... cheers, Gerd
On Wed, Feb 22, 2012 at 03:47:08PM +0100, Gerd Hoffmann wrote: > Hi, > > > I was thinking to add a new command since I'll want to add the id, and > > if I'm already adding a new command I'll put in a display number too. > > Big question is what the display number is supposed to be ... > Ah, yes, it's not specified well enough. So I guess it should actually be two parameters: device path - whatever we are calling it nowadays. device specific monitor number (or frame buffer id). This should be good enough for current 4 pci qxl devices, and future proof for a single qxl with multiple monitor outputs. Same goes for s/qxl/any other framebuffer device/, I'm just not sure which are there (although I'll find out a little by trying out the arm emulater for the r-pi). > cheers, > Gerd >
On Wed, 22 Feb 2012 15:29:33 +0100 Alon Levy <alevy@redhat.com> wrote: > On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote: > > Hi, > > > > > Honestly, for this particular case, I'm not 100% sure that having an id is > > > _required_, as I don't expect a client to submit multiple screendump calls > > > in parallel and we don't "officially" support multiple QMP clients either. > > > Also, having the screendump filename in the event will serve as a form of > > > identifier too. > > > > That is exactly my thinking, echo the filename written in the event. > > > > > Btw, are you planning to add the event to the already existing screendump > > > command? Adding a new command that doesn't use the monitor async API and > > > is truly asynchronous wouldn't better? > > > > Good question. I'd tend to just let the existing command send trigger > > an event. But libvirt needs some way to figure whenever it should wait > > for an event ... > > Right, that's the second reason I think a new command is needed. > Additionally a new command can be implemented only by qxl and not by > anything else (although I guess that would be a NACK?) Is there anything specific to qlx in the command? If there's, then you should also consider making this a QOM device property. The downside is that QOM commands are not going to be stable for 1.1.
On Wed, Feb 22, 2012 at 01:55:45PM -0200, Luiz Capitulino wrote: > On Wed, 22 Feb 2012 15:29:33 +0100 > Alon Levy <alevy@redhat.com> wrote: > > > On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote: > > > Hi, > > > > > > > Honestly, for this particular case, I'm not 100% sure that having an id is > > > > _required_, as I don't expect a client to submit multiple screendump calls > > > > in parallel and we don't "officially" support multiple QMP clients either. > > > > Also, having the screendump filename in the event will serve as a form of > > > > identifier too. > > > > > > That is exactly my thinking, echo the filename written in the event. > > > > > > > Btw, are you planning to add the event to the already existing screendump > > > > command? Adding a new command that doesn't use the monitor async API and > > > > is truly asynchronous wouldn't better? > > > > > > Good question. I'd tend to just let the existing command send trigger > > > an event. But libvirt needs some way to figure whenever it should wait > > > for an event ... > > > > Right, that's the second reason I think a new command is needed. > > Additionally a new command can be implemented only by qxl and not by > > anything else (although I guess that would be a NACK?) > > Is there anything specific to qlx in the command? If there's, then you > should also consider making this a QOM device property. The downside is > that QOM commands are not going to be stable for 1.1. qxl is the only one that needs the async stuff since it needs to ask spice-server to do the rendering, which is done in another thread outside of qemu control. The rest of the screendump implementations afaict don't need any such complexity. The command itself would not be specific to qxl. I thought I just need a QAPI command, and docs/ say QMP doesn't use that yet, so what's the connection to QMP?
On Wed, 22 Feb 2012 17:35:15 +0100 Alon Levy <alevy@redhat.com> wrote: > On Wed, Feb 22, 2012 at 01:55:45PM -0200, Luiz Capitulino wrote: > > On Wed, 22 Feb 2012 15:29:33 +0100 > > Alon Levy <alevy@redhat.com> wrote: > > > > > On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote: > > > > Hi, > > > > > > > > > Honestly, for this particular case, I'm not 100% sure that having an id is > > > > > _required_, as I don't expect a client to submit multiple screendump calls > > > > > in parallel and we don't "officially" support multiple QMP clients either. > > > > > Also, having the screendump filename in the event will serve as a form of > > > > > identifier too. > > > > > > > > That is exactly my thinking, echo the filename written in the event. > > > > > > > > > Btw, are you planning to add the event to the already existing screendump > > > > > command? Adding a new command that doesn't use the monitor async API and > > > > > is truly asynchronous wouldn't better? > > > > > > > > Good question. I'd tend to just let the existing command send trigger > > > > an event. But libvirt needs some way to figure whenever it should wait > > > > for an event ... > > > > > > Right, that's the second reason I think a new command is needed. > > > Additionally a new command can be implemented only by qxl and not by > > > anything else (although I guess that would be a NACK?) > > > > Is there anything specific to qlx in the command? If there's, then you > > should also consider making this a QOM device property. The downside is > > that QOM commands are not going to be stable for 1.1. > > qxl is the only one that needs the async stuff since it needs to ask > spice-server to do the rendering, which is done in another thread > outside of qemu control. The rest of the screendump implementations > afaict don't need any such complexity. In theory, any file I/O should be asynchronous. > The command itself would not be specific to qxl. > > I thought I just need a QAPI command, and docs/ say QMP doesn't use that > yet, so what's the connection to QMP? Not sure I can follow you here, "that" what?
diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 7f9fbca..7b120ab 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -75,15 +75,17 @@ typedef struct QXLRenderUpdateData { int redraw; QXLRect dirty[32]; QXLRect area; + char *filename; } QXLRenderUpdateData; -void qxl_render_update(PCIQXLDevice *qxl) +void qxl_render_update(PCIQXLDevice *qxl, const char *filename) { VGACommonState *vga = &qxl->vga; QXLRect dirty[32]; void *ptr; int redraw = 0; QXLRenderUpdateData *data; + QXLCookie *cookie; if (!is_buffer_shared(vga->ds->surface)) { dprint(qxl, 1, "%s: restoring shared displaysurface\n", __func__); @@ -139,11 +141,24 @@ void qxl_render_update(PCIQXLDevice *qxl) data->area.right = qxl->guest_primary.surface.width; data->area.top = 0; data->area.bottom = qxl->guest_primary.surface.height; + if (filename) { + data->filename = g_strdup(filename); + } + cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, + 0, + (uint64_t)data); +#if SPICE_INTERFACE_QXL_MINOR >= 1 qxl_spice_update_area(qxl, 0, &data->area, data->dirty, ARRAY_SIZE(dirty), 1, QXL_ASYNC, - qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, - 0, - (uint64_t)data)); + cookie); +#else + qxl_spice_update_area(qxl, 0, &data->area, + data->dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, NULL); + qxl_render_update_area_done(qxl, cookie); + if (filename) { + ppm_save(filename, qxl->ssd.ds->surface); + } +#endif } void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) @@ -171,6 +186,10 @@ void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) dirty[i].right - dirty[i].left, dirty[i].bottom - dirty[i].top); } + if (data->filename) { + ppm_save(data->filename, qxl->ssd.ds->surface); + g_free(data->filename); + } g_free(data); } diff --git a/hw/qxl.c b/hw/qxl.c index 5563293..2409cb3 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -145,12 +145,12 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id, uint32_t clear_dirty_region, qxl_async_io async, QXLCookie *cookie) { - struct QXLRect *area_copy; if (async == QXL_SYNC) { qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area, dirty_rects, num_dirty_rects, clear_dirty_region); } else { #if SPICE_INTERFACE_QXL_MINOR >= 1 + struct QXLRect *area_copy; if (cookie == NULL) { area_copy = g_malloc0(sizeof(*area_copy)); memcpy(area_copy, area, sizeof(*area)); @@ -1476,7 +1476,7 @@ static void qxl_hw_update(void *opaque) break; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl); + qxl_render_update(qxl, NULL); break; default: break; @@ -1499,8 +1499,15 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename) switch (qxl->mode) { case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl); - ppm_save(filename, qxl->ssd.ds->surface); + /* + * TODO: if we use an async update_area, to avoid deadlock with + * virt-manager, we postpone the saving of the image until the + * rendering is done. This means the image isn't guranteed to be + * written when we return to the monitor. Fixing this needs an async + * monitor command, whatever the implementation of the concept is + * called. + */ + qxl_render_update(qxl, filename); break; case QXL_MODE_VGA: vga->screen_dump(vga, filename); diff --git a/hw/qxl.h b/hw/qxl.h index 71d5c35..198abdc 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -132,7 +132,7 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext); /* qxl-render.c */ void qxl_render_resize(PCIQXLDevice *qxl); -void qxl_render_update(PCIQXLDevice *qxl); +void qxl_render_update(PCIQXLDevice *qxl, const char *filename); void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext); void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie);