[v3,13/17] monitor: Allow to exclude commands from QMP

From: Jan Kiszka <jan.kiszka@siemens.com>

Ported commands that are marked 'user_only' will not be considered for
QMP monitor sessions. This allows to implement new commands that do not
(yet) provide a sufficiently stable interface for QMP use (e.g.
device_show).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 monitor.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

On Sun, 23 May 2010 12:59:26 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Ported commands that are marked 'user_only' will not be considered for
> QMP monitor sessions. This allows to implement new commands that do not
> (yet) provide a sufficiently stable interface for QMP use (e.g.
> device_show).

 This is fine for me, but two things I've been wondering:

 1. Isn't a 'flags' struct member better? So that we can do (in the
    qemu-monitor.hx entry):

        .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,

    I'm not suggesting this is an async handler, just exemplifying multiple
    flags.

  2. Getting QMP handlers right in the first time might be difficult, so
     we could have a way to mark them unstable. Maybe a different namespace
     which is only enabled at configure time with:

         --enable-qmp-unstable-commands

     If this were possible, we could have device_show and any command we
     aren't sure is QMP-ready working in QMP this way.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  monitor.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 6766e49..5768c6e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -114,6 +114,7 @@ typedef struct mon_cmd_t {
>                            MonitorCompletion *cb, void *opaque);
>      } mhandler;
>      int async;
> +    bool user_only;
>  } mon_cmd_t;
>  
>  /* file descriptors passed via SCM_RIGHTS */
> @@ -635,6 +636,11 @@ static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          goto help;
>      }
>  
> +    if (monitor_ctrl_mode(mon) && cmd->user_only) {
> +        qerror_report(QERR_COMMAND_NOT_FOUND, item);
> +        return -1;
> +    }
> +
>      if (monitor_handler_is_async(cmd)) {
>          if (monitor_ctrl_mode(mon)) {
>              qmp_async_info_handler(mon, cmd);
> @@ -732,13 +738,14 @@ static void do_info_commands(Monitor *mon, QObject **ret_data)
>      cmd_list = qlist_new();
>  
>      for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
> -        if (monitor_handler_ported(cmd) && !compare_cmd(cmd->name, "info")) {
> +        if (monitor_handler_ported(cmd) && !cmd->user_only &&
> +            !compare_cmd(cmd->name, "info")) {
>              qlist_append_obj(cmd_list, get_cmd_dict(cmd->name));
>          }
>      }
>  
>      for (cmd = info_cmds; cmd->name != NULL; cmd++) {
> -        if (monitor_handler_ported(cmd)) {
> +        if (monitor_handler_ported(cmd) && !cmd->user_only) {
>              char buf[128];
>              snprintf(buf, sizeof(buf), "query-%s", cmd->name);
>              qlist_append_obj(cmd_list, get_cmd_dict(buf));
> @@ -4416,7 +4423,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>                        qobject_from_jsonf("{ 'item': %s }", info_item));
>      } else {
>          cmd = monitor_find_command(cmd_name);
> -        if (!cmd || !monitor_handler_ported(cmd)) {
> +        if (!cmd || !monitor_handler_ported(cmd) || cmd->user_only) {
>              qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
>              goto err_input;
>          }

Luiz Capitulino wrote:
> On Sun, 23 May 2010 12:59:26 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Ported commands that are marked 'user_only' will not be considered for
>> QMP monitor sessions. This allows to implement new commands that do not
>> (yet) provide a sufficiently stable interface for QMP use (e.g.
>> device_show).
> 
>  This is fine for me, but two things I've been wondering:
> 
>  1. Isn't a 'flags' struct member better? So that we can do (in the
>     qemu-monitor.hx entry):
> 
>         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
> 
>     I'm not suggesting this is an async handler, just exemplifying multiple
>     flags.

Yes, can refactor this.

> 
>   2. Getting QMP handlers right in the first time might be difficult, so
>      we could have a way to mark them unstable. Maybe a different namespace
>      which is only enabled at configure time with:
> 
>          --enable-qmp-unstable-commands
> 
>      If this were possible, we could have device_show and any command we
>      aren't sure is QMP-ready working in QMP this way.

Do you suggest this as an alternative to this patch? Or an extension
later on? I have no opinion on this yet, I would just like to know how
to proceed for this series.

Jan

On Fri, 28 May 2010 00:20:08 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Luiz Capitulino wrote:
> > On Sun, 23 May 2010 12:59:26 +0200
> > Jan Kiszka <jan.kiszka@web.de> wrote:
> > 
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Ported commands that are marked 'user_only' will not be considered for
> >> QMP monitor sessions. This allows to implement new commands that do not
> >> (yet) provide a sufficiently stable interface for QMP use (e.g.
> >> device_show).
> > 
> >  This is fine for me, but two things I've been wondering:
> > 
> >  1. Isn't a 'flags' struct member better? So that we can do (in the
> >     qemu-monitor.hx entry):
> > 
> >         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
> > 
> >     I'm not suggesting this is an async handler, just exemplifying multiple
> >     flags.
> 
> Yes, can refactor this.
> 
> > 
> >   2. Getting QMP handlers right in the first time might be difficult, so
> >      we could have a way to mark them unstable. Maybe a different namespace
> >      which is only enabled at configure time with:
> > 
> >          --enable-qmp-unstable-commands
> > 
> >      If this were possible, we could have device_show and any command we
> >      aren't sure is QMP-ready working in QMP this way.
> 
> Do you suggest this as an alternative to this patch? Or an extension
> later on? I have no opinion on this yet, I would just like to know how
> to proceed for this series.

 Both can be done worked later, as this is internal there's no problem in
living with a simpler solution for a while.

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sun, 23 May 2010 12:59:26 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> Ported commands that are marked 'user_only' will not be considered for
>> QMP monitor sessions. This allows to implement new commands that do not
>> (yet) provide a sufficiently stable interface for QMP use (e.g.
>> device_show).
>
>  This is fine for me, but two things I've been wondering:
>
>  1. Isn't a 'flags' struct member better? So that we can do (in the
>     qemu-monitor.hx entry):
>
>         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
>
>     I'm not suggesting this is an async handler, just exemplifying multiple
>     flags.

We also have at least one command that makes only sense in QMP:
qmp_capabilities.  Maybe we could use separate flags controlling command
availability in human monitor and QMP.

[...]

Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> On Sun, 23 May 2010 12:59:26 +0200
>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Ported commands that are marked 'user_only' will not be considered for
>>> QMP monitor sessions. This allows to implement new commands that do not
>>> (yet) provide a sufficiently stable interface for QMP use (e.g.
>>> device_show).
>>  This is fine for me, but two things I've been wondering:
>>
>>  1. Isn't a 'flags' struct member better? So that we can do (in the
>>     qemu-monitor.hx entry):
>>
>>         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
>>
>>     I'm not suggesting this is an async handler, just exemplifying multiple
>>     flags.
> 
> We also have at least one command that makes only sense in QMP:
> qmp_capabilities.  Maybe we could use separate flags controlling command
> availability in human monitor and QMP.

Sounds reasonable. Then I think I will lay the ground for this by
introducing flags already in this patch. A v4 run is required anyway.

Jan