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

login
register
mail settings
Submitter Jan Kiszka
Date May 23, 2010, 10:59 a.m.
Message ID <5d0139dc62706a1efcbb6a63d5936484ad279916.1274612367.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/53311/
State New
Headers show

Comments

Jan Kiszka - May 23, 2010, 10:59 a.m.
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(-)
Luiz Capitulino - May 27, 2010, 8:31 p.m.
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;
>          }
Jan Kiszka - May 27, 2010, 10:20 p.m.
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
Luiz Capitulino - May 28, 2010, 1:45 p.m.
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.
Markus Armbruster - May 29, 2010, 8:15 a.m.
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.

[...]
Jan Kiszka - May 29, 2010, 8:33 a.m.
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

Patch

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;
         }