subject:"\[Qemu\-devel\] \[PATCH\] make screendump an async command"

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-17 Thread Gerd Hoffmann

  Hi,

 Why is QXL unable to do a synchronous screendump?

Lazy rendering.  By default spice-server doesn't render anything, it
just sends over all drawing ops to the client who does the actual
rendering for the user.

qemu can kick spice-server and ask it to render stuff locally if needed.
 Happens when either the guest asks for it or when qemu needs it for its
own purposes.  One reason is screendump, the other is qxl being used
with gtk/sdl/vnc ui instead of spice.

Today we kick the spice server worker thread to do the rendering, but we
don't wait for it completing the rendering before writing out the
screendump.  That isn't perfect of course because you are not guaranteed
to get a up-to-date dump.  But works good enougth for some use cases
like autotest, which does screendumps each second anyway, so the actual
screen dump is at most one second old.


Hmm, while thinking about it:  There is another screendump change in the
pipeline: allow screen dumps from *any* device.  So, I think this is
actually a very good reason to implement a new screendump command as I
think we can kill two birds with one stone then:

First we can add a new parameter to specify the device we want dump
from.  And second we are not bound to the behavior of the existing
screendump command.  So we could simply send out a qmp event as
completion (or error) notification.

Comments?

cheers,
  Gerd

 
 Regards,
 
 Anthony Liguori
 
 ---
  hmp.c |  2 +-
  hw/display/qxl-render.c   |  1 +
  hw/display/vga.c  |  1 +
  include/qapi/qmp/qerror.h |  6 +
  include/ui/console.h  | 10 
  qapi-schema.json  | 13 ---
  qmp-commands.hx   |  3 ++-
  ui/console.c  | 58 
 ++-
  8 files changed, 78 insertions(+), 16 deletions(-)

 diff --git a/hmp.c b/hmp.c
 index 494a9aa..2a37975 100644
 --- a/hmp.c
 +++ b/hmp.c
 @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
  const char *filename = qdict_get_str(qdict, filename);
  Error *err = NULL;
  
 -qmp_screendump(filename, err);
 +hmp_screen_dump_helper(filename, err);
  hmp_handle_error(mon, err);
  }
  
 diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
 index f511a62..d719448 100644
 --- a/hw/display/qxl-render.c
 +++ b/hw/display/qxl-render.c
 @@ -139,6 +139,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice 
 *qxl)
 qxl-dirty[i].bottom - qxl-dirty[i].top);
  }
  qxl-num_dirty_rects = 0;
 +console_screendump_complete(vga-con);
  }
  
  /*
 diff --git a/hw/display/vga.c b/hw/display/vga.c
 index 21a108d..1fc52eb 100644
 --- a/hw/display/vga.c
 +++ b/hw/display/vga.c
 @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
  break;
  }
  }
 +console_screendump_complete(s-con);
  }
  
  /* force a full display refresh */
 diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
 index 6c0a18d..1bd7efa 100644
 --- a/include/qapi/qmp/qerror.h
 +++ b/include/qapi/qmp/qerror.h
 @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
  #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
  ERROR_CLASS_GENERIC_ERROR, Migration is disabled when VirtFS export 
 path '%s' is mounted in the guest using mount_tag '%s'
  
 +#define QERR_SCREENDUMP_NOT_AVAILABLE \
 +ERROR_CLASS_GENERIC_ERROR, There is no QemuConsole I can screendump 
 from
 +
 +#define QERR_SCREENDUMP_IN_PROGRESS \
 +ERROR_CLASS_GENERIC_ERROR, Existing screendump in progress
 +
  #define QERR_SOCKET_CONNECT_FAILED \
  ERROR_CLASS_GENERIC_ERROR, Failed to connect to socket
  
 diff --git a/include/ui/console.h b/include/ui/console.h
 index 4307b5f..643da45 100644
 --- a/include/ui/console.h
 +++ b/include/ui/console.h
 @@ -341,4 +341,14 @@ int index_from_keycode(int code);
  void early_gtk_display_init(void);
  void gtk_display_init(DisplayState *ds);
  
 +/* hw/display/vga.c hw/display/qxl.c */
 +void console_screendump_complete(QemuConsole *con);
 +
 +/* monitor.c */
 +int qmp_screendump(Monitor *mon, const QDict *qdict,
 +   MonitorCompletion cb, void *opaque);
 +
 +/* hmp.c */
 +void hmp_screen_dump_helper(const char *filename, Error **errp);
 +
  #endif
 diff --git a/qapi-schema.json b/qapi-schema.json
 index 5ad6894..f5fdc2f 100644
 --- a/qapi-schema.json
 +++ b/qapi-schema.json
 @@ -3112,19 +3112,6 @@
'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
  
  ##
 -# @screendump:
 -#
 -# Write a PPM of the VGA screen to a file.
 -#
 -# @filename: the path of a new PPM file to store the image
 -#
 -# Returns: Nothing on success
 -#
 -# Since: 0.14.0
 -##
 -{ 'command': 'screendump', 'data': {'filename': 'str'} }
 -
 -##
  # @nbd-server-start:
  #
  # Start an NBD server listening on the given host and port.  Block
 diff --git a/qmp-commands.hx b/qmp-commands.hx
 index 8cea5e5..bde8f43 100644
 --- a/qmp-commands.hx
 +++ b/qmp-commands.hx
 @@

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-17 Thread Alon Levy

On Mon, 2013-06-17 at 08:06 +0200, Gerd Hoffmann wrote:
   Hi,
 
  Why is QXL unable to do a synchronous screendump?
 
 Lazy rendering.  By default spice-server doesn't render anything, it
 just sends over all drawing ops to the client who does the actual
 rendering for the user.
 
 qemu can kick spice-server and ask it to render stuff locally if needed.
  Happens when either the guest asks for it or when qemu needs it for its
 own purposes.  One reason is screendump, the other is qxl being used
 with gtk/sdl/vnc ui instead of spice.
 
 Today we kick the spice server worker thread to do the rendering, but we
 don't wait for it completing the rendering before writing out the
 screendump.  That isn't perfect of course because you are not guaranteed
 to get a up-to-date dump.  But works good enougth for some use cases
 like autotest, which does screendumps each second anyway, so the actual
 screen dump is at most one second old.
 
 
 Hmm, while thinking about it:  There is another screendump change in the
 pipeline: allow screen dumps from *any* device.  So, I think this is
 actually a very good reason to implement a new screendump command as I
 think we can kill two birds with one stone then:
 
 First we can add a new parameter to specify the device we want dump
 from.  And second we are not bound to the behavior of the existing
 screendump command.  So we could simply send out a qmp event as
 completion (or error) notification.
 
 Comments?

I personally think having an event is not really required, it
complicates things in qemu by requiring the command to return before it
issues any events, i.e. this would be illegal (and breaks libvirt):

Libvirt: { execute: screendump, arguments: { filename:
/tmp/image } }
Qemu: { event: SCREENDUMP_COMPLETE, data : { filename:
screenshot.ppm} }
Qemu: { return: {} }

But on the other hand if this is something that would be acceptable now
and having proper Async error handling is not forthcoming (why btw? is
anyone working on it) . But it would become baggage later on..

 
 cheers,
   Gerd
 
  
  Regards,
  
  Anthony Liguori
  
  ---
   hmp.c |  2 +-
   hw/display/qxl-render.c   |  1 +
   hw/display/vga.c  |  1 +
   include/qapi/qmp/qerror.h |  6 +
   include/ui/console.h  | 10 
   qapi-schema.json  | 13 ---
   qmp-commands.hx   |  3 ++-
   ui/console.c  | 58 
  ++-
   8 files changed, 78 insertions(+), 16 deletions(-)
 
  diff --git a/hmp.c b/hmp.c
  index 494a9aa..2a37975 100644
  --- a/hmp.c
  +++ b/hmp.c
  @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict 
  *qdict)
   const char *filename = qdict_get_str(qdict, filename);
   Error *err = NULL;
   
  -qmp_screendump(filename, err);
  +hmp_screen_dump_helper(filename, err);
   hmp_handle_error(mon, err);
   }
   
  diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
  index f511a62..d719448 100644
  --- a/hw/display/qxl-render.c
  +++ b/hw/display/qxl-render.c
  @@ -139,6 +139,7 @@ static void 
  qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
  qxl-dirty[i].bottom - qxl-dirty[i].top);
   }
   qxl-num_dirty_rects = 0;
  +console_screendump_complete(vga-con);
   }
   
   /*
  diff --git a/hw/display/vga.c b/hw/display/vga.c
  index 21a108d..1fc52eb 100644
  --- a/hw/display/vga.c
  +++ b/hw/display/vga.c
  @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
   break;
   }
   }
  +console_screendump_complete(s-con);
   }
   
   /* force a full display refresh */
  diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
  index 6c0a18d..1bd7efa 100644
  --- a/include/qapi/qmp/qerror.h
  +++ b/include/qapi/qmp/qerror.h
  @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
   #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
   ERROR_CLASS_GENERIC_ERROR, Migration is disabled when VirtFS export 
  path '%s' is mounted in the guest using mount_tag '%s'
   
  +#define QERR_SCREENDUMP_NOT_AVAILABLE \
  +ERROR_CLASS_GENERIC_ERROR, There is no QemuConsole I can screendump 
  from
  +
  +#define QERR_SCREENDUMP_IN_PROGRESS \
  +ERROR_CLASS_GENERIC_ERROR, Existing screendump in progress
  +
   #define QERR_SOCKET_CONNECT_FAILED \
   ERROR_CLASS_GENERIC_ERROR, Failed to connect to socket
   
  diff --git a/include/ui/console.h b/include/ui/console.h
  index 4307b5f..643da45 100644
  --- a/include/ui/console.h
  +++ b/include/ui/console.h
  @@ -341,4 +341,14 @@ int index_from_keycode(int code);
   void early_gtk_display_init(void);
   void gtk_display_init(DisplayState *ds);
   
  +/* hw/display/vga.c hw/display/qxl.c */
  +void console_screendump_complete(QemuConsole *con);
  +
  +/* monitor.c */
  +int qmp_screendump(Monitor *mon, const QDict *qdict,
  +   MonitorCompletion cb, void *opaque);
  +
  +/* hmp.c */
  +void

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-17 Thread Luiz Capitulino

On Fri, 14 Jun 2013 19:55:21 -0400
Alon Levy al...@redhat.com wrote:

 On Fri, 2013-06-14 at 13:21 -0500, Anthony Liguori wrote:
  Alon Levy al...@redhat.com writes:
  
   This fixes the broken screendump behavior for qxl devices in native mode
   since 81fb6f1504fb9ef71f2382f44af34756668296e8.
  
   Note: due to QAPI not generating async commands yet I had to remove the
   schema screendump definition.
  
   Related RHBZ: 973374
   This patch is not enough to fix said bz, with the linux qxl driver you 
   get garbage still, just not out of date garbage.
  
   Signed-off-by: Alon Levy al...@redhat.com
  
  Async commands are badly broken with respect to error handling.
 
 Are there any ideas on how this should work? From the user perspective
 nothing changes, so this is an internal qemu design issue afaict.
 
  
  This needs to be done after we get the new QMP server.
 
 Is there any ETA on this? I'm not pressuring, I'm just trying to figure
 out if it is eminent or else I'll add a temporary patch to the
 downstream (which is what I'm trying to avoid by sending this patch in
 the first place).

No ETA. We have to finish QAPI conversion work first (which is halted ATM)
_or_ we lazily port the missing conversions by doing error conversion
only _or_ we add some legacy mechanism to the new QMP server.

More importantly though, IIRC we have agreed on not having async commands
at all. All async stuff should be done with command plus event.

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-17 Thread Luiz Capitulino

On Mon, 17 Jun 2013 08:06:58 +0200
Gerd Hoffmann kra...@redhat.com wrote:

 Hmm, while thinking about it:  There is another screendump change in the
 pipeline: allow screen dumps from *any* device.  So, I think this is
 actually a very good reason to implement a new screendump command as I
 think we can kill two birds with one stone then:
 
 First we can add a new parameter to specify the device we want dump
 from.  And second we are not bound to the behavior of the existing
 screendump command.  So we could simply send out a qmp event as
 completion (or error) notification.

That's how we agreed on doing async commands, and looks like a good
solution to me.

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-17 Thread Gerd Hoffmann

On 06/17/13 11:30, Alon Levy wrote:
 But on the other hand if this is something that would be acceptable now
 and having proper Async error handling is not forthcoming (why btw? is
 anyone working on it) . But it would become baggage later on..

Don't think it is too bad, RfC patches will go out in a moment ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-14 Thread Gerd Hoffmann

  Hi,

 Note: due to QAPI not generating async commands yet I had to remove the
 schema screendump definition.

Hmm, that will break libvirt I suspect.  Guess this one has to wait
until QAPI gained proper async command support.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-14 Thread Alon Levy

 Hi,
 
  Note: due to QAPI not generating async commands yet I had to remove the
  schema screendump definition.
 
 Hmm, that will break libvirt I suspect.  Guess this one has to wait
 until QAPI gained proper async command support.

It doesn't break anything. I've tested, and here is the QMP transcript, the 
only difference is internal to qemu, from libvirt point of view it issues the 
same command and gets the same response:

$ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio
{QMP: {version: {qemu: {micro: 50, minor: 5, major: 1}, package: 
}, capabilities: []}}
{execute:qmp_capabilities}
{return: {}}
{execute:screendump,arguments:{filename:test2.ppm}}
{return: {}}


 
 cheers,
   Gerd

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-14 Thread Alon Levy

  Hi,
  
   Note: due to QAPI not generating async commands yet I had to remove the
   schema screendump definition.
  
  Hmm, that will break libvirt I suspect.  Guess this one has to wait
  until QAPI gained proper async command support.
 
 It doesn't break anything. I've tested,

To clarify, I tested with virsh screenshot as well.

 and here is the QMP transcript, the
 only difference is internal to qemu, from libvirt point of view it issues
 the same command and gets the same response:
 
 $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio
 {QMP: {version: {qemu: {micro: 50, minor: 5, major: 1},
 package: }, capabilities: []}}
 {execute:qmp_capabilities}
 {return: {}}
 {execute:screendump,arguments:{filename:test2.ppm}}
 {return: {}}
 
 
  
  cheers,
Gerd

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-14 Thread Anthony Liguori

Alon Levy al...@redhat.com writes:

 This fixes the broken screendump behavior for qxl devices in native mode
 since 81fb6f1504fb9ef71f2382f44af34756668296e8.

 Note: due to QAPI not generating async commands yet I had to remove the
 schema screendump definition.

 Related RHBZ: 973374
 This patch is not enough to fix said bz, with the linux qxl driver you get 
 garbage still, just not out of date garbage.

 Signed-off-by: Alon Levy al...@redhat.com

Async commands are badly broken with respect to error handling.

This needs to be done after we get the new QMP server.

Why is QXL unable to do a synchronous screendump?

Regards,

Anthony Liguori

 ---
  hmp.c |  2 +-
  hw/display/qxl-render.c   |  1 +
  hw/display/vga.c  |  1 +
  include/qapi/qmp/qerror.h |  6 +
  include/ui/console.h  | 10 
  qapi-schema.json  | 13 ---
  qmp-commands.hx   |  3 ++-
  ui/console.c  | 58 
 ++-
  8 files changed, 78 insertions(+), 16 deletions(-)

 diff --git a/hmp.c b/hmp.c
 index 494a9aa..2a37975 100644
 --- a/hmp.c
 +++ b/hmp.c
 @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
  const char *filename = qdict_get_str(qdict, filename);
  Error *err = NULL;
  
 -qmp_screendump(filename, err);
 +hmp_screen_dump_helper(filename, err);
  hmp_handle_error(mon, err);
  }
  
 diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
 index f511a62..d719448 100644
 --- a/hw/display/qxl-render.c
 +++ b/hw/display/qxl-render.c
 @@ -139,6 +139,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice 
 *qxl)
 qxl-dirty[i].bottom - qxl-dirty[i].top);
  }
  qxl-num_dirty_rects = 0;
 +console_screendump_complete(vga-con);
  }
  
  /*
 diff --git a/hw/display/vga.c b/hw/display/vga.c
 index 21a108d..1fc52eb 100644
 --- a/hw/display/vga.c
 +++ b/hw/display/vga.c
 @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
  break;
  }
  }
 +console_screendump_complete(s-con);
  }
  
  /* force a full display refresh */
 diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
 index 6c0a18d..1bd7efa 100644
 --- a/include/qapi/qmp/qerror.h
 +++ b/include/qapi/qmp/qerror.h
 @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
  #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
  ERROR_CLASS_GENERIC_ERROR, Migration is disabled when VirtFS export 
 path '%s' is mounted in the guest using mount_tag '%s'
  
 +#define QERR_SCREENDUMP_NOT_AVAILABLE \
 +ERROR_CLASS_GENERIC_ERROR, There is no QemuConsole I can screendump 
 from
 +
 +#define QERR_SCREENDUMP_IN_PROGRESS \
 +ERROR_CLASS_GENERIC_ERROR, Existing screendump in progress
 +
  #define QERR_SOCKET_CONNECT_FAILED \
  ERROR_CLASS_GENERIC_ERROR, Failed to connect to socket
  
 diff --git a/include/ui/console.h b/include/ui/console.h
 index 4307b5f..643da45 100644
 --- a/include/ui/console.h
 +++ b/include/ui/console.h
 @@ -341,4 +341,14 @@ int index_from_keycode(int code);
  void early_gtk_display_init(void);
  void gtk_display_init(DisplayState *ds);
  
 +/* hw/display/vga.c hw/display/qxl.c */
 +void console_screendump_complete(QemuConsole *con);
 +
 +/* monitor.c */
 +int qmp_screendump(Monitor *mon, const QDict *qdict,
 +   MonitorCompletion cb, void *opaque);
 +
 +/* hmp.c */
 +void hmp_screen_dump_helper(const char *filename, Error **errp);
 +
  #endif
 diff --git a/qapi-schema.json b/qapi-schema.json
 index 5ad6894..f5fdc2f 100644
 --- a/qapi-schema.json
 +++ b/qapi-schema.json
 @@ -3112,19 +3112,6 @@
'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
  
  ##
 -# @screendump:
 -#
 -# Write a PPM of the VGA screen to a file.
 -#
 -# @filename: the path of a new PPM file to store the image
 -#
 -# Returns: Nothing on success
 -#
 -# Since: 0.14.0
 -##
 -{ 'command': 'screendump', 'data': {'filename': 'str'} }
 -
 -##
  # @nbd-server-start:
  #
  # Start an NBD server listening on the given host and port.  Block
 diff --git a/qmp-commands.hx b/qmp-commands.hx
 index 8cea5e5..bde8f43 100644
 --- a/qmp-commands.hx
 +++ b/qmp-commands.hx
 @@ -146,7 +146,8 @@ EQMP
  {
  .name   = screendump,
  .args_type  = filename:F,
 -.mhandler.cmd_new = qmp_marshal_input_screendump,
 +.mhandler.cmd_async = qmp_screendump,
 +.flags  = MONITOR_CMD_ASYNC,
  },
  
  SQMP
 diff --git a/ui/console.c b/ui/console.c
 index 28bba6d..0efd588 100644
 --- a/ui/console.c
 +++ b/ui/console.c
 @@ -116,6 +116,12 @@ typedef enum {
  struct QemuConsole {
  Object parent;
  
 +struct {
 +char *filename;
 +MonitorCompletion *completion_cb;
 +void *completion_opaque;
 +} screendump;
 +
  int index;
  console_type_t console_type;
  DisplayState *ds;
 @@ -313,11 +319,61 @@ write_err:
  goto out;
  }
  
 -void qmp_screendump(const

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-14 Thread Alon Levy

On Fri, 2013-06-14 at 13:21 -0500, Anthony Liguori wrote:
 Alon Levy al...@redhat.com writes:
 
  This fixes the broken screendump behavior for qxl devices in native mode
  since 81fb6f1504fb9ef71f2382f44af34756668296e8.
 
  Note: due to QAPI not generating async commands yet I had to remove the
  schema screendump definition.
 
  Related RHBZ: 973374
  This patch is not enough to fix said bz, with the linux qxl driver you get 
  garbage still, just not out of date garbage.
 
  Signed-off-by: Alon Levy al...@redhat.com
 
 Async commands are badly broken with respect to error handling.

Are there any ideas on how this should work? From the user perspective
nothing changes, so this is an internal qemu design issue afaict.

 
 This needs to be done after we get the new QMP server.

Is there any ETA on this? I'm not pressuring, I'm just trying to figure
out if it is eminent or else I'll add a temporary patch to the
downstream (which is what I'm trying to avoid by sending this patch in
the first place).

 
 Why is QXL unable to do a synchronous screendump?

The qxl device needs to flush all the commands that haven't been
rendered. The rendering is done off thread in the worker thread for that
device. The communication between the threads happens via a pipe that
the io thread is listening to. That is the same thread the qmp command
is received on, so there is no option to block unless I start a new
event loop in there, which is ugly. i.e. I need some kind of bottom
half, like the async command provides, which client_migrate_info uses.

 
 Regards,
 
 Anthony Liguori
 
  ---
   hmp.c |  2 +-
   hw/display/qxl-render.c   |  1 +
   hw/display/vga.c  |  1 +
   include/qapi/qmp/qerror.h |  6 +
   include/ui/console.h  | 10 
   qapi-schema.json  | 13 ---
   qmp-commands.hx   |  3 ++-
   ui/console.c  | 58 
  ++-
   8 files changed, 78 insertions(+), 16 deletions(-)
 
  diff --git a/hmp.c b/hmp.c
  index 494a9aa..2a37975 100644
  --- a/hmp.c
  +++ b/hmp.c
  @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
   const char *filename = qdict_get_str(qdict, filename);
   Error *err = NULL;
   
  -qmp_screendump(filename, err);
  +hmp_screen_dump_helper(filename, err);
   hmp_handle_error(mon, err);
   }
   
  diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
  index f511a62..d719448 100644
  --- a/hw/display/qxl-render.c
  +++ b/hw/display/qxl-render.c
  @@ -139,6 +139,7 @@ static void 
  qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
  qxl-dirty[i].bottom - qxl-dirty[i].top);
   }
   qxl-num_dirty_rects = 0;
  +console_screendump_complete(vga-con);
   }
   
   /*
  diff --git a/hw/display/vga.c b/hw/display/vga.c
  index 21a108d..1fc52eb 100644
  --- a/hw/display/vga.c
  +++ b/hw/display/vga.c
  @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
   break;
   }
   }
  +console_screendump_complete(s-con);
   }
   
   /* force a full display refresh */
  diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
  index 6c0a18d..1bd7efa 100644
  --- a/include/qapi/qmp/qerror.h
  +++ b/include/qapi/qmp/qerror.h
  @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
   #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
   ERROR_CLASS_GENERIC_ERROR, Migration is disabled when VirtFS export 
  path '%s' is mounted in the guest using mount_tag '%s'
   
  +#define QERR_SCREENDUMP_NOT_AVAILABLE \
  +ERROR_CLASS_GENERIC_ERROR, There is no QemuConsole I can screendump 
  from
  +
  +#define QERR_SCREENDUMP_IN_PROGRESS \
  +ERROR_CLASS_GENERIC_ERROR, Existing screendump in progress
  +
   #define QERR_SOCKET_CONNECT_FAILED \
   ERROR_CLASS_GENERIC_ERROR, Failed to connect to socket
   
  diff --git a/include/ui/console.h b/include/ui/console.h
  index 4307b5f..643da45 100644
  --- a/include/ui/console.h
  +++ b/include/ui/console.h
  @@ -341,4 +341,14 @@ int index_from_keycode(int code);
   void early_gtk_display_init(void);
   void gtk_display_init(DisplayState *ds);
   
  +/* hw/display/vga.c hw/display/qxl.c */
  +void console_screendump_complete(QemuConsole *con);
  +
  +/* monitor.c */
  +int qmp_screendump(Monitor *mon, const QDict *qdict,
  +   MonitorCompletion cb, void *opaque);
  +
  +/* hmp.c */
  +void hmp_screen_dump_helper(const char *filename, Error **errp);
  +
   #endif
  diff --git a/qapi-schema.json b/qapi-schema.json
  index 5ad6894..f5fdc2f 100644
  --- a/qapi-schema.json
  +++ b/qapi-schema.json
  @@ -3112,19 +3112,6 @@
 'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
   
   ##
  -# @screendump:
  -#
  -# Write a PPM of the VGA screen to a file.
  -#
  -# @filename: the path of a new PPM file to store the image
  -#
  -# Returns: Nothing on success
  -#
  -# Since: 0.14.0
  -##
  -{ 'command':

[Qemu-devel] [PATCH] make screendump an async command

2013-06-13 Thread Alon Levy

This fixes the broken screendump behavior for qxl devices in native mode
since 81fb6f1504fb9ef71f2382f44af34756668296e8.

Note: due to QAPI not generating async commands yet I had to remove the
schema screendump definition.

Related RHBZ: 973374
This patch is not enough to fix said bz, with the linux qxl driver you get 
garbage still, just not out of date garbage.

Signed-off-by: Alon Levy al...@redhat.com
---
 hmp.c |  2 +-
 hw/display/qxl-render.c   |  1 +
 hw/display/vga.c  |  1 +
 include/qapi/qmp/qerror.h |  6 +
 include/ui/console.h  | 10 
 qapi-schema.json  | 13 ---
 qmp-commands.hx   |  3 ++-
 ui/console.c  | 58 ++-
 8 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/hmp.c b/hmp.c
index 494a9aa..2a37975 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
 const char *filename = qdict_get_str(qdict, filename);
 Error *err = NULL;
 
-qmp_screendump(filename, err);
+hmp_screen_dump_helper(filename, err);
 hmp_handle_error(mon, err);
 }
 
diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index f511a62..d719448 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -139,6 +139,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice 
*qxl)
qxl-dirty[i].bottom - qxl-dirty[i].top);
 }
 qxl-num_dirty_rects = 0;
+console_screendump_complete(vga-con);
 }
 
 /*
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 21a108d..1fc52eb 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
 break;
 }
 }
+console_screendump_complete(s-con);
 }
 
 /* force a full display refresh */
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 6c0a18d..1bd7efa 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -237,6 +237,12 @@ void assert_no_error(Error *err);
 #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
 ERROR_CLASS_GENERIC_ERROR, Migration is disabled when VirtFS export path 
'%s' is mounted in the guest using mount_tag '%s'
 
+#define QERR_SCREENDUMP_NOT_AVAILABLE \
+ERROR_CLASS_GENERIC_ERROR, There is no QemuConsole I can screendump from
+
+#define QERR_SCREENDUMP_IN_PROGRESS \
+ERROR_CLASS_GENERIC_ERROR, Existing screendump in progress
+
 #define QERR_SOCKET_CONNECT_FAILED \
 ERROR_CLASS_GENERIC_ERROR, Failed to connect to socket
 
diff --git a/include/ui/console.h b/include/ui/console.h
index 4307b5f..643da45 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -341,4 +341,14 @@ int index_from_keycode(int code);
 void early_gtk_display_init(void);
 void gtk_display_init(DisplayState *ds);
 
+/* hw/display/vga.c hw/display/qxl.c */
+void console_screendump_complete(QemuConsole *con);
+
+/* monitor.c */
+int qmp_screendump(Monitor *mon, const QDict *qdict,
+   MonitorCompletion cb, void *opaque);
+
+/* hmp.c */
+void hmp_screen_dump_helper(const char *filename, Error **errp);
+
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 5ad6894..f5fdc2f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3112,19 +3112,6 @@
   'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
 
 ##
-# @screendump:
-#
-# Write a PPM of the VGA screen to a file.
-#
-# @filename: the path of a new PPM file to store the image
-#
-# Returns: Nothing on success
-#
-# Since: 0.14.0
-##
-{ 'command': 'screendump', 'data': {'filename': 'str'} }
-
-##
 # @nbd-server-start:
 #
 # Start an NBD server listening on the given host and port.  Block
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8cea5e5..bde8f43 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -146,7 +146,8 @@ EQMP
 {
 .name   = screendump,
 .args_type  = filename:F,
-.mhandler.cmd_new = qmp_marshal_input_screendump,
+.mhandler.cmd_async = qmp_screendump,
+.flags  = MONITOR_CMD_ASYNC,
 },
 
 SQMP
diff --git a/ui/console.c b/ui/console.c
index 28bba6d..0efd588 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -116,6 +116,12 @@ typedef enum {
 struct QemuConsole {
 Object parent;
 
+struct {
+char *filename;
+MonitorCompletion *completion_cb;
+void *completion_opaque;
+} screendump;
+
 int index;
 console_type_t console_type;
 DisplayState *ds;
@@ -313,11 +319,61 @@ write_err:
 goto out;
 }
 
-void qmp_screendump(const char *filename, Error **errp)
+int qmp_screendump(Monitor *mon, const QDict *qdict,
+   MonitorCompletion cb, void *opaque)
 {
 QemuConsole *con = qemu_console_lookup_by_index(0);
+const char *filename = qdict_get_str(qdict, filename);
+
+if (con == NULL) {
+qerror_report(QERR_SCREENDUMP_NOT_AVAILABLE);
+return -1;
+}
+if (filename ==

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-13 Thread Paolo Bonzini

Il 13/06/2013 15:27, Alon Levy ha scritto:
 This fixes the broken screendump behavior for qxl devices in native mode
 since 81fb6f1504fb9ef71f2382f44af34756668296e8.
 
 Note: due to QAPI not generating async commands yet I had to remove the
 schema screendump definition.
 
 Related RHBZ: 973374
 This patch is not enough to fix said bz, with the linux qxl driver you get 
 garbage still, just not out of date garbage.
 
 Signed-off-by: Alon Levy al...@redhat.com
 ---
  hmp.c |  2 +-
  hw/display/qxl-render.c   |  1 +
  hw/display/vga.c  |  1 +
  include/qapi/qmp/qerror.h |  6 +
  include/ui/console.h  | 10 
  qapi-schema.json  | 13 ---
  qmp-commands.hx   |  3 ++-
  ui/console.c  | 58 
 ++-
  8 files changed, 78 insertions(+), 16 deletions(-)
 
 diff --git a/hmp.c b/hmp.c
 index 494a9aa..2a37975 100644
 --- a/hmp.c
 +++ b/hmp.c
 @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
  const char *filename = qdict_get_str(qdict, filename);
  Error *err = NULL;
  
 -qmp_screendump(filename, err);
 +hmp_screen_dump_helper(filename, err);
  hmp_handle_error(mon, err);
  }
  
 diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
 index f511a62..d719448 100644
 --- a/hw/display/qxl-render.c
 +++ b/hw/display/qxl-render.c
 @@ -139,6 +139,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice 
 *qxl)
 qxl-dirty[i].bottom - qxl-dirty[i].top);
  }
  qxl-num_dirty_rects = 0;
 +console_screendump_complete(vga-con);
  }
  
  /*
 diff --git a/hw/display/vga.c b/hw/display/vga.c
 index 21a108d..1fc52eb 100644
 --- a/hw/display/vga.c
 +++ b/hw/display/vga.c
 @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
  break;
  }
  }
 +console_screendump_complete(s-con);
  }
  
  /* force a full display refresh */
 diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
 index 6c0a18d..1bd7efa 100644
 --- a/include/qapi/qmp/qerror.h
 +++ b/include/qapi/qmp/qerror.h
 @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
  #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
  ERROR_CLASS_GENERIC_ERROR, Migration is disabled when VirtFS export 
 path '%s' is mounted in the guest using mount_tag '%s'
  
 +#define QERR_SCREENDUMP_NOT_AVAILABLE \
 +ERROR_CLASS_GENERIC_ERROR, There is no QemuConsole I can screendump 
 from

s/QemuConsole/graphical console/

 +#define QERR_SCREENDUMP_IN_PROGRESS \
 +ERROR_CLASS_GENERIC_ERROR, Existing screendump in progress
 +

Please use error_setg instead.

  #define QERR_SOCKET_CONNECT_FAILED \
  ERROR_CLASS_GENERIC_ERROR, Failed to connect to socket
  
 diff --git a/include/ui/console.h b/include/ui/console.h
 index 4307b5f..643da45 100644
 --- a/include/ui/console.h
 +++ b/include/ui/console.h
 @@ -341,4 +341,14 @@ int index_from_keycode(int code);
  void early_gtk_display_init(void);
  void gtk_display_init(DisplayState *ds);
  
 +/* hw/display/vga.c hw/display/qxl.c */
 +void console_screendump_complete(QemuConsole *con);
 +
 +/* monitor.c */
 +int qmp_screendump(Monitor *mon, const QDict *qdict,
 +   MonitorCompletion cb, void *opaque);
 +
 +/* hmp.c */
 +void hmp_screen_dump_helper(const char *filename, Error **errp);
 +
  #endif
 diff --git a/qapi-schema.json b/qapi-schema.json
 index 5ad6894..f5fdc2f 100644
 --- a/qapi-schema.json
 +++ b/qapi-schema.json
 @@ -3112,19 +3112,6 @@
'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
  
  ##
 -# @screendump:
 -#
 -# Write a PPM of the VGA screen to a file.
 -#
 -# @filename: the path of a new PPM file to store the image
 -#
 -# Returns: Nothing on success
 -#
 -# Since: 0.14.0
 -##
 -{ 'command': 'screendump', 'data': {'filename': 'str'} }

Please just comment out the declaration with an explanation.

Paolo

 -
 -##
  # @nbd-server-start:
  #
  # Start an NBD server listening on the given host and port.  Block
 diff --git a/qmp-commands.hx b/qmp-commands.hx
 index 8cea5e5..bde8f43 100644
 --- a/qmp-commands.hx
 +++ b/qmp-commands.hx
 @@ -146,7 +146,8 @@ EQMP
  {
  .name   = screendump,
  .args_type  = filename:F,
 -.mhandler.cmd_new = qmp_marshal_input_screendump,
 +.mhandler.cmd_async = qmp_screendump,
 +.flags  = MONITOR_CMD_ASYNC,
  },
  
  SQMP
 diff --git a/ui/console.c b/ui/console.c
 index 28bba6d..0efd588 100644
 --- a/ui/console.c
 +++ b/ui/console.c
 @@ -116,6 +116,12 @@ typedef enum {
  struct QemuConsole {
  Object parent;
  
 +struct {
 +char *filename;
 +MonitorCompletion *completion_cb;
 +void *completion_opaque;
 +} screendump;
 +
  int index;
  console_type_t console_type;
  DisplayState *ds;
 @@ -313,11 +319,61 @@ write_err:
  goto out;
  }
  
 -void qmp_screendump(const char *filename, Error **errp)
 +int

Re: [Qemu-devel] [PATCH] make screendump an async command

2013-06-13 Thread Alon Levy

 Il 13/06/2013 15:27, Alon Levy ha scritto:
  This fixes the broken screendump behavior for qxl devices in native mode
  since 81fb6f1504fb9ef71f2382f44af34756668296e8.
  
  Note: due to QAPI not generating async commands yet I had to remove the
  schema screendump definition.
  
  Related RHBZ: 973374
  This patch is not enough to fix said bz, with the linux qxl driver you get
  garbage still, just not out of date garbage.
  
  Signed-off-by: Alon Levy al...@redhat.com
  ---
   hmp.c |  2 +-
   hw/display/qxl-render.c   |  1 +
   hw/display/vga.c  |  1 +
   include/qapi/qmp/qerror.h |  6 +
   include/ui/console.h  | 10 
   qapi-schema.json  | 13 ---
   qmp-commands.hx   |  3 ++-
   ui/console.c  | 58
   ++-
   8 files changed, 78 insertions(+), 16 deletions(-)
  
  diff --git a/hmp.c b/hmp.c
  index 494a9aa..2a37975 100644
  --- a/hmp.c
  +++ b/hmp.c
  @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict
  *qdict)
   const char *filename = qdict_get_str(qdict, filename);
   Error *err = NULL;
   
  -qmp_screendump(filename, err);
  +hmp_screen_dump_helper(filename, err);
   hmp_handle_error(mon, err);
   }
   
  diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
  index f511a62..d719448 100644
  --- a/hw/display/qxl-render.c
  +++ b/hw/display/qxl-render.c
  @@ -139,6 +139,7 @@ static void
  qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
  qxl-dirty[i].bottom - qxl-dirty[i].top);
   }
   qxl-num_dirty_rects = 0;
  +console_screendump_complete(vga-con);
   }
   
   /*
  diff --git a/hw/display/vga.c b/hw/display/vga.c
  index 21a108d..1fc52eb 100644
  --- a/hw/display/vga.c
  +++ b/hw/display/vga.c
  @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
   break;
   }
   }
  +console_screendump_complete(s-con);
   }
   
   /* force a full display refresh */
  diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
  index 6c0a18d..1bd7efa 100644
  --- a/include/qapi/qmp/qerror.h
  +++ b/include/qapi/qmp/qerror.h
  @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
   #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
   ERROR_CLASS_GENERIC_ERROR, Migration is disabled when VirtFS export
   path '%s' is mounted in the guest using mount_tag '%s'
   
  +#define QERR_SCREENDUMP_NOT_AVAILABLE \
  +ERROR_CLASS_GENERIC_ERROR, There is no QemuConsole I can screendump
  from
 
 s/QemuConsole/graphical console/
 
  +#define QERR_SCREENDUMP_IN_PROGRESS \
  +ERROR_CLASS_GENERIC_ERROR, Existing screendump in progress
  +
 
 Please use error_setg instead.

I'm not sure what you are proposing. Something like this?

diff --git a/ui/console.c b/ui/console.c
index 0efd588..3a1e6ed 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -334,7 +334,9 @@ int qmp_screendump(Monitor *mon, const QDict *qdict,
 return -1;
 }
 if (con-screendump.filename != NULL) {
-qerror_report(QERR_SCREENDUMP_IN_PROGRESS);
+QError *err = NULL;
+error_setg(err, Existing screendump in progress);
+monitor_set_error(mon, err);
 return -1;
 }
 con-screendump.filename = g_strdup(filename);

(minus defining err in the inner scope)


 
   #define QERR_SOCKET_CONNECT_FAILED \
   ERROR_CLASS_GENERIC_ERROR, Failed to connect to socket
   
  diff --git a/include/ui/console.h b/include/ui/console.h
  index 4307b5f..643da45 100644
  --- a/include/ui/console.h
  +++ b/include/ui/console.h
  @@ -341,4 +341,14 @@ int index_from_keycode(int code);
   void early_gtk_display_init(void);
   void gtk_display_init(DisplayState *ds);
   
  +/* hw/display/vga.c hw/display/qxl.c */
  +void console_screendump_complete(QemuConsole *con);
  +
  +/* monitor.c */
  +int qmp_screendump(Monitor *mon, const QDict *qdict,
  +   MonitorCompletion cb, void *opaque);
  +
  +/* hmp.c */
  +void hmp_screen_dump_helper(const char *filename, Error **errp);
  +
   #endif
  diff --git a/qapi-schema.json b/qapi-schema.json
  index 5ad6894..f5fdc2f 100644
  --- a/qapi-schema.json
  +++ b/qapi-schema.json
  @@ -3112,19 +3112,6 @@
 'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
   
   ##
  -# @screendump:
  -#
  -# Write a PPM of the VGA screen to a file.
  -#
  -# @filename: the path of a new PPM file to store the image
  -#
  -# Returns: Nothing on success
  -#
  -# Since: 0.14.0
  -##
  -{ 'command': 'screendump', 'data': {'filename': 'str'} }
 
 Please just comment out the declaration with an explanation.
 
 Paolo
 
  -
  -##
   # @nbd-server-start:
   #
   # Start an NBD server listening on the given host and port.  Block
  diff --git a/qmp-commands.hx b/qmp-commands.hx
  index 8cea5e5..bde8f43 100644
  --- a/qmp-commands.hx
  +++ b/qmp-commands.hx
  @@ -146,7 +146,8 @@ EQMP
   {
   .name   = screendump,

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

[Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

Re: [Qemu-devel] [PATCH] make screendump an async command

13 matches

Site Navigation

Mail list logo

Footer information