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
