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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 27 Jun 2013 12:05:33 +0200
Gerd Hoffmann <kraxel@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
>