make screendump an async command
PATCH 1/1 make screendump an async command
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.
--- 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(-)
hmp.c +1 -1
@@ -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); }
hw/display/qxl-render.c +1 -0
@@ -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); }/*
hw/display/vga.c +1 -0
@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)break ; } } +console_screendump_complete (s->con); }/* force a full display refresh */
include/qapi/qmp/qerror.h +6 -0
@@ -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"
include/ui/console.h +10 -0
@@ -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
qapi-schema.json +0 -13
@@ -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
qmp-commands.hx +2 -1
@@ -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
ui/console.c +57 -1
74 lines not on this page · raw message
Signature · 1 line
1.8.2.1
Il 13/06/2013 15:27, Alon Levy ha scritto:
Quoted 63 lines
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 <[email protected]>--- 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(-)hmp.c +1
-1@@ -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); }hw/display/qxl-render.c +1
-0@@ -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); }/* hw/display/vga.c +1
-0@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)break ; } } +console_screendump_complete (s->con); }/* force a full display refresh */ include/qapi/qmp/qerror.h +0
-0
include/qapi/qmp/qerror.h void assert_no_error(Error *err); · line 241
Quoted 5 lines
include/qapi/qmp/qerror.h +2
-0@@ -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/
include/qapi/qmp/qerror.h void assert_no_error(Error *err); · line 245
Quoted 3 lines
include/qapi/qmp/qerror.h +3
-0+#define QERR_SCREENDUMP_IN_PROGRESS \ + ERROR_CLASS_GENERIC_ERROR, "Existing screendump in progress" +
Please use error_setg instead.
Quoted 31 lines
include/qapi/qmp/qerror.h +0
-0#define QERR_SOCKET_CONNECT_FAILED \ ERROR_CLASS_GENERIC_ERROR, "Failed to connect to socket"include/ui/console.h +10
-0@@ -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 qapi-schema.json +0
-1@@ -3112,19 +3112,6 @@ 'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } } ## -# @screendump:
qapi-schema.json line 3125
Quoted 10 lines
qapi-schema.json +0
-10-# -# 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
Quoted 100 lines
qapi-schema.json +0
-2- -## # @nbd-server-start: # # Start an NBD server listening on the given host and port. Blockqmp-commands.hx +2
-1@@ -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, }, SQMPui/console.c +57
-1@@ -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 ==NULL ) { +qerror_report (QERR_MISSING_PARAMETER ,"filename" ); +return -1 ; + } +if (con->screendump.filename !=NULL ) { +qerror_report (QERR_SCREENDUMP_IN_PROGRESS ); +return -1 ; + } + con->screendump.filename =g_strdup (filename); + con->screendump.completion_cb = cb; + con->screendump.completion_opaque = opaque; + +graphic_hw_update (con); +return 0 ; +} + +void console_screendump_complete (QemuConsole *con) +{ +Error *errp =NULL ;DisplaySurface *surface; +if (!con->screendump.filename) { +return ; + } +assert (con->screendump.completion_cb); + surface =qemu_console_surface (con); +ppm_save (con->screendump.filename, surface, &errp); +if (errp) { +/* +* TODO: return data? raise error somehow? read qmp-spec for async +* error reporting. +*/ + } + con->screendump.completion_cb (con->screendump.completion_opaque,NULL ); +g_free (con->screendump.filename); + con->screendump.filename =NULL ; + con->screendump.completion_cb =NULL ; + con->screendump.completion_opaque =NULL ; +} + +void hmp_screen_dump_helper (const char *filename,Error **errp) +{ +DisplaySurface *surface; +QemuConsole *con =qemu_console_lookup_by_index (0 ); +if (con ==NULL ) {error_setg (errp,"There is no QemuConsole I can screendump from." );return ;
Quoted 78 lines
Il 13/06/2013 15:27, Alon Levy ha scritto:
Quoted 73 lines
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 <[email protected]>--- 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(-)hmp.c +1
-1@@ -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); }hw/display/qxl-render.c +1
-0@@ -139,6 +139,7 @@ static voidqxl_render_update_area_unlocked (PCIQXLDevice *qxl) qxl->dirty[i].bottom - qxl->dirty[i].top); } qxl->num_dirty_rects =0 ; +console_screendump_complete (vga->con); }/*
hw/display/vga.c +1
-0@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)break ; } } +console_screendump_complete (s->con); }/* force a full display refresh */ include/qapi/qmp/qerror.h +2
-0@@ -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 screendumpQuoted 1 line
from"
s/QemuConsole/graphical console/
Quoted 5 lines
include/qapi/qmp/qerror.h
Quoted 3 lines
include/qapi/qmp/qerror.h +3
-0+#define QERR_SCREENDUMP_IN_PROGRESS \ + ERROR_CLASS_GENERIC_ERROR, "Existing screendump in progress" +Please use
error_setginstead.
I'm not sure what you are proposing. Something like this?
ui/console.c +3 -1
@@ -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)
Quoted 152 lines
Quoted 32 lines
#define
QERR_SOCKET_CONNECT_FAILED\
ERROR_CLASS_GENERIC_ERROR, "Failed to connect to socket"include/ui/console.h +10
-0@@ -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 qapi-schema.json +0
-1@@ -3112,19 +3112,6 @@ 'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } } ## -# @screendump:qapi-schema.json line 3125
Quoted 10 lines
qapi-schema.json +0
-10-# -# 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
Quoted 102 lines
qapi-schema.json +0
-2- -## # @nbd-server-start: # # Start an NBD server listening on the given host and port. Blockqmp-commands.hx +2
-1@@ -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, }, SQMPui/console.c +57
-1@@ -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 ==NULL ) { +qerror_report (QERR_MISSING_PARAMETER ,"filename" ); +return -1 ; + } +if (con->screendump.filename !=NULL ) { +qerror_report (QERR_SCREENDUMP_IN_PROGRESS ); +return -1 ; + } + con->screendump.filename =g_strdup (filename); + con->screendump.completion_cb = cb; + con->screendump.completion_opaque = opaque; + +graphic_hw_update (con); +return 0 ; +} + +void console_screendump_complete (QemuConsole *con) +{ +Error *errp =NULL ;DisplaySurface *surface; +if (!con->screendump.filename) { +return ; + } +assert (con->screendump.completion_cb); + surface =qemu_console_surface (con); +ppm_save (con->screendump.filename, surface, &errp); +if (errp) { +/* +* TODO: return data? raise error somehow? read qmp-spec for async +* error reporting. +*/ + } + con->screendump.completion_cb (con->screendump.completion_opaque,NULL ); +g_free (con->screendump.filename); + con->screendump.filename =NULL ; + con->screendump.completion_cb =NULL ; + con->screendump.completion_opaque =NULL ; +} + +void hmp_screen_dump_helper (const char *filename,Error **errp) +{ +DisplaySurface *surface; +QemuConsole *con =qemu_console_lookup_by_index (0 ); +if (con ==NULL ) { error_setg(errp, "There is no QemuConsole I can screendumpfrom."); return;
Hi,
Quoted 2 lines
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
Quoted 7 lines
Hi,
Quoted 2 lines
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": {}}Quoted 7 lines
cheers,
Gerd
Quoted 9 lines
Quoted 7 lines
Hi,
Quoted 2 lines
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.
Quoted 22 lines
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": {}}Quoted 7 lines
cheers,
Gerd
Quoted 10 lines · 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 <[email protected]>
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
Quoted 204 lines
--- 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(-)hmp.c +1
-1@@ -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); }hw/display/qxl-render.c +1
-0@@ -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); }/* hw/display/vga.c +1
-0@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)break ; } } +console_screendump_complete (s->con); }/* force a full display refresh */ include/qapi/qmp/qerror.h +6
-0@@ -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"include/ui/console.h +10
-0@@ -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 qapi-schema.json +0
-13@@ -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. Blockqmp-commands.hx +2
-1@@ -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, }, SQMPui/console.c +57
-1@@ -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 ==NULL ) { +qerror_report (QERR_MISSING_PARAMETER ,"filename" ); +return -1 ; + } +if (con->screendump.filename !=NULL ) { +qerror_report (QERR_SCREENDUMP_IN_PROGRESS ); +return -1 ; + } + con->screendump.filename =g_strdup (filename); + con->screendump.completion_cb = cb; + con->screendump.completion_opaque = opaque; + +graphic_hw_update (con); +return 0 ; +} + +void console_screendump_complete (QemuConsole *con) +{ +Error *errp =NULL ;DisplaySurface *surface; +if (!con->screendump.filename) { +return ; + } +assert (con->screendump.completion_cb); + surface =qemu_console_surface (con); +ppm_save (con->screendump.filename, surface, &errp); +if (errp) { +/* +* TODO: return data? raise error somehow? read qmp-spec for async +* error reporting. +*/ + } + con->screendump.completion_cb (con->screendump.completion_opaque,NULL ); +g_free (con->screendump.filename); + con->screendump.filename =NULL ; + con->screendump.completion_cb =NULL ; + con->screendump.completion_opaque =NULL ; +} + +void hmp_screen_dump_helper (const char *filename,Error **errp) +{ +DisplaySurface *surface; +QemuConsole *con =qemu_console_lookup_by_index (0 ); +if (con ==NULL ) {error_setg (errp,"There is no QemuConsole I can screendump from." );return ;Signature · 1 line
1.8.2.1
Quoted 8 lines · Anthony Liguori
Quoted 6 lines · 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.
Quoted 6 lines
Quoted 4 lines
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 <[email protected]>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.
Quoted 2 lines
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).
Quoted 2 lines
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.
Quoted 211 lines
Regards,
Anthony Liguori
Quoted 204 lines
--- 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(-)hmp.c +1
-1@@ -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); }hw/display/qxl-render.c +1
-0@@ -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); }/* hw/display/vga.c +1
-0@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)break ; } } +console_screendump_complete (s->con); }/* force a full display refresh */ include/qapi/qmp/qerror.h +6
-0@@ -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"include/ui/console.h +10
-0@@ -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 qapi-schema.json +0
-13@@ -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. Blockqmp-commands.hx +2
-1@@ -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, }, SQMPui/console.c +57
-1@@ -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 ==NULL ) { +qerror_report (QERR_MISSING_PARAMETER ,"filename" ); +return -1 ; + } +if (con->screendump.filename !=NULL ) { +qerror_report (QERR_SCREENDUMP_IN_PROGRESS ); +return -1 ; + } + con->screendump.filename =g_strdup (filename); + con->screendump.completion_cb = cb; + con->screendump.completion_opaque = opaque; + +graphic_hw_update (con); +return 0 ; +} + +void console_screendump_complete (QemuConsole *con) +{ +Error *errp =NULL ;DisplaySurface *surface; +if (!con->screendump.filename) { +return ; + } +assert (con->screendump.completion_cb); + surface =qemu_console_surface (con); +ppm_save (con->screendump.filename, surface, &errp); +if (errp) { +/* +* TODO: return data? raise error somehow? read qmp-spec for async +* error reporting. +*/ + } + con->screendump.completion_cb (con->screendump.completion_opaque,NULL ); +g_free (con->screendump.filename); + con->screendump.filename =NULL ; + con->screendump.completion_cb =NULL ; + con->screendump.completion_opaque =NULL ; +} + +void hmp_screen_dump_helper (const char *filename,Error **errp) +{ +DisplaySurface *surface; +QemuConsole *con =qemu_console_lookup_by_index (0 ); +if (con ==NULL ) {error_setg (errp,"There is no QemuConsole I can screendump from." );return ;Signature · 1 line
1.8.2.1
Quoted 20 lines · On Fri, 14 Jun 2013 19:55:21 -0400 Alon Levy <[email protected]> wrote:
Quoted 8 lines · Anthony Liguori
Quoted 6 lines · 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.
Quoted 6 lines
Quoted 4 lines
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 <[email protected]>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.
Quoted 1 line
Quoted 6 lines
Quoted 1 line
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.
Hi,
Quoted 1 line
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
Quoted 210 lines
Regards,
Anthony Liguori
Quoted 204 lines
--- 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(-)hmp.c +1
-1@@ -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); }hw/display/qxl-render.c +1
-0@@ -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); }/* hw/display/vga.c +1
-0@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)break ; } } +console_screendump_complete (s->con); }/* force a full display refresh */ include/qapi/qmp/qerror.h +6
-0@@ -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"include/ui/console.h +10
-0@@ -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 qapi-schema.json +0
-13@@ -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. Blockqmp-commands.hx +2
-1@@ -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, }, SQMPui/console.c +57
-1@@ -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 ==NULL ) { +qerror_report (QERR_MISSING_PARAMETER ,"filename" ); +return -1 ; + } +if (con->screendump.filename !=NULL ) { +qerror_report (QERR_SCREENDUMP_IN_PROGRESS ); +return -1 ; + } + con->screendump.filename =g_strdup (filename); + con->screendump.completion_cb = cb; + con->screendump.completion_opaque = opaque; + +graphic_hw_update (con); +return 0 ; +} + +void console_screendump_complete (QemuConsole *con) +{ +Error *errp =NULL ;DisplaySurface *surface; +if (!con->screendump.filename) { +return ; + } +assert (con->screendump.completion_cb); + surface =qemu_console_surface (con); +ppm_save (con->screendump.filename, surface, &errp); +if (errp) { +/* +* TODO: return data? raise error somehow? read qmp-spec for async +* error reporting. +*/ + } + con->screendump.completion_cb (con->screendump.completion_opaque,NULL ); +g_free (con->screendump.filename); + con->screendump.filename =NULL ; + con->screendump.completion_cb =NULL ; + con->screendump.completion_opaque =NULL ; +} + +void hmp_screen_dump_helper (const char *filename,Error **errp) +{ +DisplaySurface *surface; +QemuConsole *con =qemu_console_lookup_by_index (0 ); +if (con ==NULL ) {error_setg (errp,"There is no QemuConsole I can screendump from." );return ;Signature · 1 line
1.8.2.1
Quoted 26 lines · Gerd Hoffmann
Hi,
Quoted 1 line
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:
Quoted 6 lines
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..
Quoted 215 lines
cheers,
GerdQuoted 210 lines
Regards,
Anthony Liguori
Quoted 204 lines
--- 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(-)hmp.c +1
-1@@ -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); }hw/display/qxl-render.c +1
-0@@ -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); }/* hw/display/vga.c +1
-0@@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)break ; } } +console_screendump_complete (s->con); }/* force a full display refresh */ include/qapi/qmp/qerror.h +6
-0@@ -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"include/ui/console.h +10
-0@@ -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 qapi-schema.json +0
-13@@ -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. Blockqmp-commands.hx +2
-1@@ -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, }, SQMPui/console.c +57
-1@@ -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 ==NULL ) { +qerror_report (QERR_MISSING_PARAMETER ,"filename" ); +return -1 ; + } +if (con->screendump.filename !=NULL ) { +qerror_report (QERR_SCREENDUMP_IN_PROGRESS ); +return -1 ; + } + con->screendump.filename =g_strdup (filename); + con->screendump.completion_cb = cb; + con->screendump.completion_opaque = opaque; + +graphic_hw_update (con); +return 0 ; +} + +void console_screendump_complete (QemuConsole *con) +{ +Error *errp =NULL ;DisplaySurface *surface; +if (!con->screendump.filename) { +return ; + } +assert (con->screendump.completion_cb); + surface =qemu_console_surface (con); +ppm_save (con->screendump.filename, surface, &errp); +if (errp) { +/* +* TODO: return data? raise error somehow? read qmp-spec for async +* error reporting. +*/ + } + con->screendump.completion_cb (con->screendump.completion_opaque,NULL ); +g_free (con->screendump.filename); + con->screendump.filename =NULL ; + con->screendump.completion_cb =NULL ; + con->screendump.completion_opaque =NULL ; +} + +void hmp_screen_dump_helper (const char *filename,Error **errp) +{ +DisplaySurface *surface; +QemuConsole *con =qemu_console_lookup_by_index (0 ); +if (con ==NULL ) {error_setg (errp,"There is no QemuConsole I can screendump from." );return ;Signature · 1 line
1.8.2.1
Quoted 3 lines · Alon Levy
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
Quoted 9 lines · On Mon, 17 Jun 2013 08:06:58 +0200 Gerd Hoffmann <[email protected]> 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.