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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

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

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.  Libvirt's API is already adequately flexible to cover an
arbitrary head of an arbitrary device, once mapped down to QMP, so the
question at hand now is whether to map it down to QMP by adding optional
parameters to the existing command or adding a new command.

> 
> Is there any big issue with adding a new command?

I haven't yet mentioned any tricks, but now that you bring it up, there
are two options:

Option 1:
reuse the same QMP command, but add optional parameters (ability to
specify device in this patch, but libvirt would also want the ability to
specify which head of a multi-head device).
Libvirt always calls screendump, and omits the optional parameters if
the user asked libvirt for screen 0.  If user asks libvirt for screen 1,
libvirt passes the optional parameter, and if qemu is too old, qemu
gives an error about invalid argument, which libvirt then feeds back to
the user as a 'screen 1 not supported'.

Option 2:
existing 'screendump' command can ONLY dump the primary head of the
primary device, and a new QMP command is added that supports head and
device selection.  Libvirt uses query-commands to determine whether new
command has been backported; if so, it uses the new command, if not, it
gives the user a nice error unless screen 0 was selected.

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]