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

[Gerd Hoffmann <kraxel@redhat.com>]

  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.

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

> Also, I'd move
> the generic error_setg() above to fbdev_display_init() and make it
> return void.

Makes sense indeed.

cheers,
  Gerd