Re: [Qemu-devel] [PATCH 5/9] fbdev: add monitor command to enable/disable

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 18 Sep 2012 09:17:10 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> This patch adds a fbdev monitor command to enable/disable
> the fbdev display at runtime to both qmp and hmp.
> 
> qmp: fbdev enable=on|off
> hmp: fbdev on|off
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hmp-commands.hx  |   15 +++++++++++++++
>  hmp.c            |    9 +++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   14 ++++++++++++++
>  qmp-commands.hx  |    6 ++++++
>  qmp.c            |   17 +++++++++++++++++
>  6 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed67e99..366a92b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1377,6 +1377,21 @@ passed since 1970, i.e. unix epoch.
>  ETEXI
>  
>      {
> +        .name       = "fbdev",
> +        .args_type  = "enable:b",
> +        .params     = "on|off",
> +        .help       = "enable/disable fbdev",
> +        .mhandler.cmd = hmp_fbdev,
> +    },
> +
> +STEXI
> +@item fbdev on | off
> +@findex fbdev
> +
> +enable/disable fbdev
> +ETEXI
> +
> +    {
>          .name       = "info",
>          .args_type  = "item:s?",
>          .params     = "[subcommand]",
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..a7feec5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1168,3 +1168,12 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>      qmp_screendump(filename, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_fbdev(Monitor *mon, const QDict *qdict)
> +{
> +    int enable = qdict_get_bool(qdict, "enable");
> +    Error *errp = NULL;
> +
> +    qmp_fbdev(enable, &errp);
> +    hmp_handle_error(mon, &errp);
> +}
> diff --git a/hmp.h b/hmp.h
> index 48b9c59..9c3d315 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -73,5 +73,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
>  void hmp_closefd(Monitor *mon, const QDict *qdict);
>  void hmp_send_key(Monitor *mon, const QDict *qdict);
>  void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> +void hmp_fbdev(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 14e4419..901c2e8 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2619,3 +2619,17 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'screendump', 'data': {'filename': 'str'} }
> +
> +# @fbdev:

Please, use more descriptive names for qmp. Maybe something
like frame-buffer-device-add/-enable.

> +#
> +# Enable/disable fbdev.
> +#
> +# @enable: whenever fbdev should be enabled or disabled.
> +#
> +# Returns: Nothing on success
> +#          GenericError on failure.

It's not needed to list GenericError as an error.

> +#
> +# Since: 1.3
> +#
> +##
> +{ 'command': 'fbdev', 'data': {'enable': 'bool'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6e21ddb..4b95fd0 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2539,3 +2539,9 @@ EQMP
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_target,
>      },
> +
> +    {
> +        .name       = "fbdev",
> +        .args_type  = "enable:b",
> +        .mhandler.cmd_new = qmp_marshal_input_fbdev,
> +    },
> diff --git a/qmp.c b/qmp.c
> index 8463922..7f6cc0b 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -391,6 +391,23 @@ void qmp_change(const char *device, const char *target,
>      }
>  }
>  
> +void qmp_fbdev(bool enable, Error **errp)
> +{
> +#if defined(CONFIG_LINUX)
> +    DisplayState *ds = get_displaystate();
> +
> +    if (enable) {
> +        if (fbdev_display_init(ds, NULL) != 0) {
> +            error_setg(errp, "fbdev initialization failed");

Would be nice to tell the reason if you have it (error_setg() has
printf()-likeformat).

> +        }
> +    } else {
> +        fbdev_display_uninit(ds);
> +    }
> +#else
> +    error_set(errp, QERR_FEATURE_DISABLED, "fbdev");

We shouldn't use QERR_ macros in new code. You have two options:

 1. use error_setg()

 2. add error_set_disabled() in error.h, similar to error_setg(),
    and use it

> +#endif
> +}
> +
>  static void qom_list_types_tramp(ObjectClass *klass, void *data)
>  {
>      ObjectTypeInfoList *e, **pret = data;