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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 27 Jun 2013 16:03:48 -0600
Eric Blake <eblake@redhat.com> wrote:

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

I don't think this is correct. First because mngt would have to parse
the error message to learn if the query command really failed or
the framebuffer device is just disabled. Second because the command does
succeed on returning the framebuffer status even when it's disabled.

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

This is also an argument in favor of not having a default device name
in QMP.