Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Gerd Hoffmann <kraxel@redhat.com>]

On 04/18/13 17:34, Eric Blake wrote:
> On 04/18/2013 09:21 AM, Luiz Capitulino wrote:
>>>> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>>>> existing screendump command?  Can libvirt figure there is a new
>>>> (optional) parameter?  See patch #5.
>>>
> 
>>> So yes, I think libvirt will be able to drive the new command by knowing
>>> how many heads appear per device, then passing in the appropriate named
>>> device to the QMP command.  And yes, I'll review patch 5 regarding
>>> interface design.
>>
>> We can extend screendump on HMP, but the general rule for QMP is to add a
>> new command instead so that clients don't have to play tricks like Eric is
>> suggesting :)

Ahem.  It didn't sound like libvirt plays tricks there, to me it simply
looked like an description of how libvirt manages devices + heads and
the confirmation that libvirt indeed know which device it wants a
screenshot from.

We also have to care to not mix up two things:  #1 is whenever we'll go
extent screendump or add screendump2 (see sub-thread started by markus
reply).  #2 the actual design of the new/extended command.

> The problem at hand is that your proposal in patch 5:
> 
> -{ 'command': 'screendump', 'data': {'filename': 'str'} }
> +{ 'command': 'screendump', 'data': {'filename': 'str',
> +                                    '*device' : 'str'} }
> 
> still doesn't support the case of dumping just one head of a multi-head
> device.

qxl is the only device with multihead support, and it works by defining
a large framebuffer and defining the heads as rectangles within this
framebuffer:

   +-------------------------------------------+
   | framebuffer                               |
   |    +-------------+   +----------+         |
   |    | head 1      |   | head 2   |         |
   |    |             |   |          |         |
   |    +-------------+   +----------+         |
   +-------------------------------------------+

(in practice you wouldn't have unused space around the heads of course,
but it's easier to draw this way ;)

So a recent spice client will get the head configuration info, then open
two windows, one for each head.  A old spice client without multihead
support will create a single window covering the whole framebuffer
instead.  Asking qemu to screendump a multihead qxl device will likewise
write out a dump of the whole framebuffer, i.e. the dump will have *all*
heads.

> device selection.  Libvirt uses query-commands to determine whether new

So query-commands doesn't list the arguments supported by a command,
only the commands themself.

> Option 2 is probably slightly nicer for guaranteeing a sane error
> message back to the user, but option 1 (the approach of this series)
> still seems workable.

Sane error messaging is good, I'd still prefer Option 1 though, to get
both we'll need a new query-arguments command I guess ...

cheers,
  Gerd