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

[Eric Blake <eblake@redhat.com>]

On 03/05/2018 05:08 AM, Thomas Huth wrote:

In the subject line: s/whith/with/

Also, it's rather long (over 75 characters, while typical messages are 
under 65, since it is nice to still fit in 80 columns even when 'git 
log' adds indentation), better might be:

qapi: Add parameters to screendump

then go into details about the parameters in the commit body

> 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.
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v3:
>   - Drop redundant qemu_console_lookup_by_index(0);
>   - Remove trailing "." from error message
> 

> +++ 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)

Worth mentioning how the default value is computed?

> +#
> +# @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'} }
>   

QAPI changes are reasonable from a UI point of view.

> +    } else {
> +        if (has_head) {
> +            error_setg(errp, "'head' must be specified together with 'device'");

This isn't documented.

> +            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);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org