On 01/04/22 4:50 pm, Markus Armbruster wrote:
Dave, please have a look at the HMP compatibility issue in
hmp-command.hx below.
Kshitij Suri <kshitij.s...@nutanix.com> writes:
Currently screendump only supports PPM format, which is un-compressed and not
standard.
If "standard" means "have to pay a standards organization $$$ to access
the spec", PPM is not standard. If it means "widely supported", it
certainly is. I'd drop "and not standard". Suggestion, not demand.
Makes sense. Will modify it in the updated patch.
Added a "format" parameter to qemu monitor screendump capabilites
to support PNG image capture using libpng. The param was added in QAPI schema
of screendump present in ui.json along with png_save() function which converts
pixman_image to PNG. HMP command equivalent was also modified to support the
feature.
Suggest to use imperative mood to describe the commit, and omit details
that aren't necessary here:
Add a "format" parameter to QMP and HMP screendump command
to support PNG image capture using libpng.
Yes, will reduce the verbosity of the commit message.
Example usage:
{ "execute": "screendump", "arguments": { "filename": "/tmp/image",
"format":"png" } }
Providing an example in the commit message is always nice, thanks!
Thank you!
Resolves:
https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=oODILSxODcEhktuPJ-SfVt-MW867cpF_TvDe-WJyNRXx84FinSifhtp6-Racosb0&s=89nTa5MLAr16WtPfrm4aYkwWlPuRs6yuaD22dZTE_pM&e=
Signed-off-by: Kshitij Suri <kshitij.s...@nutanix.com>
Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
---
hmp-commands.hx | 11 ++---
monitor/hmp-cmds.c | 12 +++++-
qapi/ui.json | 24 +++++++++--
ui/console.c | 101 +++++++++++++++++++++++++++++++++++++++++++--
4 files changed, 136 insertions(+), 12 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8476277aa9..19b7cab595 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -244,11 +244,12 @@ ERST
{
.name = "screendump",
- .args_type = "filename:F,device:s?,head:i?",
- .params = "filename [device [head]]",
- .help = "save screen from head 'head' of display device 'device'
"
- "into PPM image 'filename'",
- .cmd = hmp_screendump,
+ .args_type = "filename:F,format:s?,device:s?,head:i?",
Incompatible change: meaning of "screendump ONE TWO" changes from
filename=ONE, device=TWO to filename=ONE, format=TWO.
As HMP is not a stable interface, incompatible change is permissible.
But is this one wise?
Could we add the new argument at the end instead?
.args_type = "filename:F,device:s?,head:i?,format:s?",
Could we do *without* an argument, and derive the format from the
filename extension? .png means format=png, anything else format=ppm.
Would be a bad idea for QMP. Okay for HMP?
Should I go ahead with extracting format from filename provided for HMP?
+ .params = "filename [format] [device [head]]",
This tells us that parameter format can be omitted like so
screendump foo.ppm device-id
which isn't true. Better: "filename [format [device [head]]".
Thank you will modify it!
+ .help = "save screen from head 'head' of display device 'device'"
+ "in specified format 'format' as image 'filename'."
+ "Currently only 'png' and 'ppm' formats are supported.",
+ .cmd = hmp_screendump,
.coroutine = true,
},
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 634968498b..2442bfa989 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1720,9 +1720,19 @@ hmp_screendump(Monitor *mon, const QDict *qdict)
const char *filename = qdict_get_str(qdict, "filename");
const char *id = qdict_get_try_str(qdict, "device");
int64_t head = qdict_get_try_int(qdict, "head", 0);
+ const char *input_format = qdict_get_try_str(qdict, "format");
Error *err = NULL;
+ ImageFormat format;
- qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
+ format = qapi_enum_parse(&ImageFormat_lookup, input_format,
+ IMAGE_FORMAT_PPM, &err);
+ if (err) {
+ goto end;
+ }
+
+ qmp_screendump(filename, id != NULL, id, id != NULL, head,
+ input_format != NULL, format, &err);
+end:
hmp_handle_error(mon, err);
}
diff --git a/qapi/ui.json b/qapi/ui.json
index 664da9e462..24371fce05 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -157,12 +157,27 @@
##
{ 'command': 'expire_password', 'boxed': true, 'data':
'ExpirePasswordOptions' }
+##
+# @ImageFormat:
+#
+# Supported image format types.
+#
+# @png: PNG format
+#
+# @ppm: PPM format
+#
+# Since: 7.1
+#
+##
+{ 'enum': 'ImageFormat',
+ 'data': ['ppm', 'png'] }
+
##
# @screendump:
#
-# Write a PPM of the VGA screen to a file.
+# Capture the contents of a screen and write it to a file.
#
-# @filename: the path of a new PPM file to store the image
+# @filename: the path of a new file to store the image
#
# @device: ID of the display device that should be dumped. If this parameter
# is missing, the primary display will be used. (Since 2.12)
@@ -171,6 +186,8 @@
# parameter is missing, head #0 will be used. Also note that the head
# can only be specified in conjunction with the device ID. (Since 2.12)
#
+# @format: image format for screendump is specified. (default: ppm) (Since 7.1)
Recommend
# @format: image format for screendump (default: ppm) (Since 7.1)
Will change it in the updated patch
+#
# Returns: Nothing on success
#
# Since: 0.14
@@ -183,7 +200,8 @@
#
##
{ 'command': 'screendump',
- 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
+ 'data': {'filename': 'str', '*device': 'str', '*head': 'int',
+ '*format': 'ImageFormat'},
'coroutine': true }
##
Thank you for your review!
Regards,
Kshitij Suri
QAPI schema
Acked-by: Markus Armbruster <arm...@redhat.com>
[...]
