[Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Thomas Huth]

QEMU's screendump command can only take dumps from the primary display.
When using multiple VGA cards, there is no way to get a dump from a
secondary card or other display heads yet. So let's add an 'device' and
a 'head' parameter to the HMP and QMP commands to be able to specify
alternative devices and heads with the screendump command, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2:
 - Renamed 'id' to 'device'
 - Add suport for specifying the 'head', too

 hmp-commands.hx |  7 ++++---
 hmp.c           |  4 +++-
 qapi/ui.json    |  7 ++++++-
 ui/console.c    | 22 ++++++++++++++++++----
 4 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d26eb41..ebb5fe4 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -253,9 +253,10 @@ ETEXI
 
     {
         .name       = "screendump",
-        .args_type  = "filename:F",
-        .params     = "filename",
-        .help       = "save screen into PPM image 'filename'",
+        .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,
     },
 
diff --git a/hmp.c b/hmp.c
index ae86bfb..7dcf72f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2132,9 +2132,11 @@ err_out:
 void 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);
     Error *err = NULL;
 
-    qmp_screendump(filename, &err);
+    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi/ui.json b/qapi/ui.json
index 3e82f25..e60b323 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -77,6 +77,10 @@
 #
 # @filename: the path of a new PPM file to store the image
 #
+# @device: ID of the display device that should be used (since 2.12)
+#
+# @head: head to use in case the device supports multiple heads (since 2.12)
+#
 # Returns: Nothing on success
 #
 # Since: 0.14.0
@@ -88,7 +92,8 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'screendump', 'data': {'filename': 'str'} }
+{ 'command': 'screendump',
+  'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
 
 ##
 # == Spice
diff --git a/ui/console.c b/ui/console.c
index e22931a..76b694e 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -344,14 +344,28 @@ write_err:
     goto out;
 }
 
-void qmp_screendump(const char *filename, Error **errp)
+void qmp_screendump(const char *filename, bool has_device, const char *device,
+                    bool has_head, int64_t head, Error **errp)
 {
     QemuConsole *con = qemu_console_lookup_by_index(0);
     DisplaySurface *surface;
 
-    if (con == NULL) {
-        error_setg(errp, "There is no QemuConsole I can screendump from.");
-        return;
+    if (has_device) {
+        con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
+                                                 errp);
+        if (!con) {
+            return;
+        }
+    } else {
+        if (has_head) {
+            error_setg(errp, "'head' must be specified together with 'device'");
+            return;
+        }
+        con = qemu_console_lookup_by_index(0);
+        if (!con) {
+            error_setg(errp, "There is no console to take a screendump from.");
+            return;
+        }
     }
 
     graphic_hw_update(con);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Daniel P. Berrangé]

On Mon, Mar 05, 2018 at 10:47:56AM +0100, Thomas Huth wrote:
> QEMU's screendump command can only take dumps from the primary display.
> When using multiple VGA cards, there is no way to get a dump from a
> secondary card or other display heads yet. So let's add an 'device' and
> a 'head' parameter to the HMP and QMP commands to be able to specify
> alternative devices and heads with the screendump command, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2:
>  - Renamed 'id' to 'device'
>  - Add suport for specifying the 'head', too
> 
>  hmp-commands.hx |  7 ++++---
>  hmp.c           |  4 +++-
>  qapi/ui.json    |  7 ++++++-
>  ui/console.c    | 22 ++++++++++++++++++----
>  4 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d26eb41..ebb5fe4 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -253,9 +253,10 @@ ETEXI
>  
>      {
>          .name       = "screendump",
> -        .args_type  = "filename:F",
> -        .params     = "filename",
> -        .help       = "save screen into PPM image 'filename'",
> +        .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,
>      },
>  
> diff --git a/hmp.c b/hmp.c
> index ae86bfb..7dcf72f 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2132,9 +2132,11 @@ err_out:
>  void 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);
>      Error *err = NULL;
>  
> -    qmp_screendump(filename, &err);
> +    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
>      hmp_handle_error(mon, &err);
>  }
>  
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 3e82f25..e60b323 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -77,6 +77,10 @@
>  #
>  # @filename: the path of a new PPM file to store the image
>  #
> +# @device: ID of the display device that should be used (since 2.12)
> +#
> +# @head: head to use in case the device supports multiple heads (since 2.12)
> +#
>  # Returns: Nothing on success
>  #
>  # Since: 0.14.0
> @@ -88,7 +92,8 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'screendump', 'data': {'filename': 'str'} }
> +{ 'command': 'screendump',
> +  'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
>  
>  ##
>  # == Spice
> diff --git a/ui/console.c b/ui/console.c
> index e22931a..76b694e 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -344,14 +344,28 @@ write_err:
>      goto out;
>  }
>  
> -void qmp_screendump(const char *filename, Error **errp)
> +void qmp_screendump(const char *filename, bool has_device, const char *device,
> +                    bool has_head, int64_t head, Error **errp)
>  {
>      QemuConsole *con = qemu_console_lookup_by_index(0);
>      DisplaySurface *surface;
>  
> -    if (con == NULL) {
> -        error_setg(errp, "There is no QemuConsole I can screendump from.");
> -        return;
> +    if (has_device) {
> +        con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
> +                                                 errp);
> +        if (!con) {
> +            return;
> +        }
> +    } else {
> +        if (has_head) {
> +            error_setg(errp, "'head' must be specified together with 'device'");
> +            return;
> +        }
> +        con = qemu_console_lookup_by_index(0);
> +        if (!con) {
> +            error_setg(errp, "There is no console to take a screendump from.");
> +            return;
> +        }

Nitpick, we don't terminate error messages with a "." normally.

Since this is just pre-existing problem though, I figure whoever queues the
patch can tweak it if needed, so

   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Gerd Hoffmann]

> -void qmp_screendump(const char *filename, Error **errp)
> +void qmp_screendump(const char *filename, bool has_device, const char *device,
> +                    bool has_head, int64_t head, Error **errp)
>  {
>      QemuConsole *con = qemu_console_lookup_by_index(0);

This initialization can be dropped ...

>      DisplaySurface *surface;
>  
> -    if (con == NULL) {
> -        error_setg(errp, "There is no QemuConsole I can screendump from.");
> -        return;
> +    if (has_device) {
> +        con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
> +                                                 errp);
> +        if (!con) {
> +            return;
> +        }
> +    } else {
> +        if (has_head) {
> +            error_setg(errp, "'head' must be specified together with 'device'");
> +            return;
> +        }
> +        con = qemu_console_lookup_by_index(0);

... because it is called here now.

Otherwise looks fine now.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> QEMU's screendump command can only take dumps from the primary display.
> When using multiple VGA cards, there is no way to get a dump from a
> secondary card or other display heads yet. So let's add an 'device' and
> a 'head' parameter to the HMP and QMP commands to be able to specify
> alternative devices and heads with the screendump command, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2:
>  - Renamed 'id' to 'device'
>  - Add suport for specifying the 'head', too
> 
>  hmp-commands.hx |  7 ++++---
>  hmp.c           |  4 +++-
>  qapi/ui.json    |  7 ++++++-
>  ui/console.c    | 22 ++++++++++++++++++----
>  4 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d26eb41..ebb5fe4 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -253,9 +253,10 @@ ETEXI
>  
>      {
>          .name       = "screendump",
> -        .args_type  = "filename:F",
> -        .params     = "filename",
> -        .help       = "save screen into PPM image 'filename'",
> +        .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,
>      },
>  
> diff --git a/hmp.c b/hmp.c
> index ae86bfb..7dcf72f 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2132,9 +2132,11 @@ err_out:
>  void 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);
>      Error *err = NULL;
>  
> -    qmp_screendump(filename, &err);
> +    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
>      hmp_handle_error(mon, &err);
>  }

Looks ok from HMP; one question, is there a way to give an ID to the
default VGA or only to extra devices?


Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> diff --git a/qapi/ui.json b/qapi/ui.json
> index 3e82f25..e60b323 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -77,6 +77,10 @@
>  #
>  # @filename: the path of a new PPM file to store the image
>  #
> +# @device: ID of the display device that should be used (since 2.12)
> +#
> +# @head: head to use in case the device supports multiple heads (since 2.12)
> +#
>  # Returns: Nothing on success
>  #
>  # Since: 0.14.0
> @@ -88,7 +92,8 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'screendump', 'data': {'filename': 'str'} }
> +{ 'command': 'screendump',
> +  'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
>  
>  ##
>  # == Spice
> diff --git a/ui/console.c b/ui/console.c
> index e22931a..76b694e 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -344,14 +344,28 @@ write_err:
>      goto out;
>  }
>  
> -void qmp_screendump(const char *filename, Error **errp)
> +void qmp_screendump(const char *filename, bool has_device, const char *device,
> +                    bool has_head, int64_t head, Error **errp)
>  {
>      QemuConsole *con = qemu_console_lookup_by_index(0);
>      DisplaySurface *surface;
>  
> -    if (con == NULL) {
> -        error_setg(errp, "There is no QemuConsole I can screendump from.");
> -        return;
> +    if (has_device) {
> +        con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
> +                                                 errp);
> +        if (!con) {
> +            return;
> +        }
> +    } else {
> +        if (has_head) {
> +            error_setg(errp, "'head' must be specified together with 'device'");
> +            return;
> +        }
> +        con = qemu_console_lookup_by_index(0);
> +        if (!con) {
> +            error_setg(errp, "There is no console to take a screendump from.");
> +            return;
> +        }
>      }
>  
>      graphic_hw_update(con);
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Thomas Huth]

On 05.03.2018 11:09, Gerd Hoffmann wrote:
>> -void qmp_screendump(const char *filename, Error **errp)
>> +void qmp_screendump(const char *filename, bool has_device, const char *device,
>> +                    bool has_head, int64_t head, Error **errp)
>>  {
>>      QemuConsole *con = qemu_console_lookup_by_index(0);
> 
> This initialization can be dropped ...

D'oh, copy-n-paste error ... I'll send a v3 ...

 Thomas


>>      DisplaySurface *surface;
>>  
>> -    if (con == NULL) {
>> -        error_setg(errp, "There is no QemuConsole I can screendump from.");
>> -        return;
>> +    if (has_device) {
>> +        con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
>> +                                                 errp);
>> +        if (!con) {
>> +            return;
>> +        }
>> +    } else {
>> +        if (has_head) {
>> +            error_setg(errp, "'head' must be specified together with 'device'");
>> +            return;
>> +        }
>> +        con = qemu_console_lookup_by_index(0);
> 
> ... because it is called here now.
> 
> Otherwise looks fine now.
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Gerd Hoffmann]

> > +    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
> >      hmp_handle_error(mon, &err);
> >  }
> 
> Looks ok from HMP; one question, is there a way to give an ID to the
> default VGA or only to extra devices?

It'll be whatever id you give to your video device.  For libvirt this is
"video$nr".  When starting qemu from the command line just use
"-device VGA,id=$yourchoice".

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] Allow to specify a display device ID and head whith the screendump command

[Dr. David Alan Gilbert]

* Gerd Hoffmann (kraxel@redhat.com) wrote:
> > > +    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
> > >      hmp_handle_error(mon, &err);
> > >  }
> > 
> > Looks ok from HMP; one question, is there a way to give an ID to the
> > default VGA or only to extra devices?
> 
> It'll be whatever id you give to your video device.  For libvirt this is
> "video$nr".  When starting qemu from the command line just use
> "-device VGA,id=$yourchoice".

Ah OK, that works, what I'd tried was
     -device qxl,id=foo      rather than
     -device qxl-vga,id=foo  which works.

Dave

> cheers,
>   Gerd
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK