Re: [Qemu-devel] [PATCH 2/2] fbdev: add monitor commands to enable/disable/query

[Eric Blake <eblake@redhat.com>]

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

On 06/27/2013 07:29 AM, Luiz Capitulino wrote:
> On Thu, 27 Jun 2013 12:05:33 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 

>>>> +# @device: #optional specifies framebuffer device, default: /dev/fb0
>>>
>>> Actually, it will try to get the device name from an env variable first,
>>> which sounds too automatic for a building block API like QMP.
>>
>> The env variable is icky indeed.  But a fixed default (which will be the
>> one you need in 99% of the cases) is fine IMO.
> 
> I prefer having no defaults, but I'm not strong about this.

I also think that letting HMP leave the parameter optional, with
first-level fallback to the environment variable and secondary fallback
to the hardcoded /dev/fv0, while making the parameter mandatory in QMP,
makes more sense.

> 
>>
>>>> +# Since 1.6
>>>> +##
>>>> +{ 'type': 'FramebufferInfo',
>>>> +  'data': { 'enabled': 'bool',
>>>> +            '*scale' : 'bool',
>>>> +            '*device': 'str',
>>>
>>> Why is device optional?
>>
>> When the framebuffer is'nt active you'll get just
>> { "enabled" : "false" }, thats why the other ones are optional.  They
>> all are filled in case the framebuffer is active.

Would it be any simpler to just have the query command fail when the
framebuffer is disabled, leave out the 'enabled' member from
FramebufferInfo, and have all the other members mandatory?

Also, would it ever make sense for a future enhancement to allow having
more than one framebuffer in use by a single domain?  Even if your
initial implementation supports at most one /dev/fb* device per guest,
it might be worth having the query-framebuffer command return an array
of all enabled framebuffers (a 0-length array when none is enabled),
rather than hard-coding it to a single FramebufferInfo return.
Likewise, make sure that framebuffer-display can deal with extension if
it makes sense (the suggestion to split into -enable and -disable
commands might be better stated as splitting into
framebuffer-display-add and framebuffer-display-delete, more like
hotplugging of other devices).

-- 
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 --]