On Thu, 27 Jun 2013 12:05:33 +0200
Gerd Hoffmann <kra...@redhat.com> wrote:
> Hi,
>
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -3608,3 +3608,46 @@
> >> '*cpuid-input-ecx': 'int',
> >> 'cpuid-register': 'X86CPURegister32',
> >> 'features': 'int' } }
> >> +
> >> +##
> >> +# @framebuffer-display:
> >
> > Let me bike-shed: we're trying to make command's names verbs. So, we
> > could call this framebuffer-display-set or maybe have two commands,
> > framebuffer-display-enable and framebuffer-display-disable. I prefer
> > the latter.
>
> Can do that.
>
> >> +#
> >> +# Enable/disable linux console framebuffer display.
> >> +#
> >> +# @enable: whenever the framebuffer display should be enabled or disabled.
> >> +#
> >> +# @scale: #optional enables display scaling, default: off
> >> +#
> >> +# @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.
>
> >> +# 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.
>
> >> +void qmp_framebuffer_display(bool enable,
> >> + bool has_scale, bool scale,
> >> + bool has_device, const char *device,
> >> + Error **errp)
> >> +{
> >> +#if defined(CONFIG_FBDEV)
> >> + if (enable) {
> >> + if (fbdev_display_init(has_device ? device : NULL,
> >> + has_scale ? scale : false,
> >> + errp) != 0) {
> >> + if (!error_is_set(errp)) {
> >> + error_setg(errp, "fbdev initialization failed");
> >
> > You should use error_propagate() in order to propagate the error
> > information filled by fbdev_display_init().
>
> Why? What is wrong with simply passing down errp?
Because errp can be NULL, then you'll be unable to detect errors if
fbdev_display_init() is changed to return void. But you're right you
won't need to propagate errors if you drop the if () test, as you
won't be checking for errors anymore.
>
> > Also, I'd move
> > the generic error_setg() above to fbdev_display_init() and make it
> > return void.
>
> Makes sense indeed.
>
> cheers,
> Gerd
>
