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
