Patchwork [v4,11/23] monitor: Add completion support for option lists

login
register
mail settings
Submitter Jan Kiszka
Date June 15, 2010, 10:38 p.m.
Message ID <68962306385412f48130690d29a0349b322b9930.1276641524.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/55815/
State New
Headers show

Comments

Jan Kiszka - June 15, 2010, 10:38 p.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

This enables command line completion inside option strings. A list of
expected key names and their completion type can be appended to the 'O'
inside parentheses ('O(key:type,...)'). The first use case is block
device completion for the 'drive' option of 'device_add'.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
 qemu-monitor.hx |    2 +-
 2 files changed, 58 insertions(+), 12 deletions(-)
Markus Armbruster - June 23, 2010, 9:45 a.m.
Jan Kiszka <jan.kiszka@web.de> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> This enables command line completion inside option strings. A list of
> expected key names and their completion type can be appended to the 'O'
> inside parentheses ('O(key:type,...)'). The first use case is block
> device completion for the 'drive' option of 'device_add'.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  qemu-monitor.hx |    2 +-
>  2 files changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index c1006b4..3e0d862 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -68,6 +68,9 @@
>   * 'O'          option string of the form NAME=VALUE,...
>   *              parsed according to QemuOptsList given by its name
>   *              Example: 'device:O' uses qemu_device_opts.
> + *              Command completion for specific keys can be requested via
> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>   *              Restriction: only lists with empty desc are supported
>   *              TODO lift the restriction
>   * 'i'          32 bit integer

Ugh.

Replacement of args_type by a proper data structure is long overdue.  We
keep piling features into that poor, hapless string.

Information on how to complete QemuOptsList options arguably belongs
into the option description, i.e. QemuOptDesc.

[...]
Jan Kiszka - June 23, 2010, 10:28 a.m.
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This enables command line completion inside option strings. A list of
>> expected key names and their completion type can be appended to the 'O'
>> inside parentheses ('O(key:type,...)'). The first use case is block
>> device completion for the 'drive' option of 'device_add'.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>  qemu-monitor.hx |    2 +-
>>  2 files changed, 58 insertions(+), 12 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c1006b4..3e0d862 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -68,6 +68,9 @@
>>   * 'O'          option string of the form NAME=VALUE,...
>>   *              parsed according to QemuOptsList given by its name
>>   *              Example: 'device:O' uses qemu_device_opts.
>> + *              Command completion for specific keys can be requested via
>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>>   *              Restriction: only lists with empty desc are supported
>>   *              TODO lift the restriction
>>   * 'i'          32 bit integer
> 
> Ugh.
> 
> Replacement of args_type by a proper data structure is long overdue.  We
> keep piling features into that poor, hapless string.
> 
> Information on how to complete QemuOptsList options arguably belongs
> into the option description, i.e. QemuOptDesc.

For sure, that would be better. I just wonder how much of it should be
stuffed into this series. I guess I will drop this part for now, just
focusing on what device_show makes direct use of. Same for separate args
for HMP and QMP.

Jan
Markus Armbruster - June 23, 2010, 5:08 p.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> This enables command line completion inside option strings. A list of
>>> expected key names and their completion type can be appended to the 'O'
>>> inside parentheses ('O(key:type,...)'). The first use case is block
>>> device completion for the 'drive' option of 'device_add'.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>>  qemu-monitor.hx |    2 +-
>>>  2 files changed, 58 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index c1006b4..3e0d862 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -68,6 +68,9 @@
>>>   * 'O'          option string of the form NAME=VALUE,...
>>>   *              parsed according to QemuOptsList given by its name
>>>   *              Example: 'device:O' uses qemu_device_opts.
>>> + *              Command completion for specific keys can be requested via
>>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
>>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>>>   *              Restriction: only lists with empty desc are supported
>>>   *              TODO lift the restriction
>>>   * 'i'          32 bit integer
>> 
>> Ugh.
>> 
>> Replacement of args_type by a proper data structure is long overdue.  We
>> keep piling features into that poor, hapless string.
>> 
>> Information on how to complete QemuOptsList options arguably belongs
>> into the option description, i.e. QemuOptDesc.
>
> For sure, that would be better. I just wonder how much of it should be
> stuffed into this series. I guess I will drop this part for now, just
> focusing on what device_show makes direct use of. Same for separate args
> for HMP and QMP.

Sensible.


Speaking of args_type.  We've accumulated too many ways to represent
dynamic data types, and too many ways to describe data types.

The most powerful dynamic data type is QObject.  It comes with a useful
plain-text representation (JSON).  We lack a data type for describing
QObjects.  We use args_type to describe special QObjects, namely monitor
command arguments.  We've clearly stretched that to the limit and then
some.  We could use a general solution.  JSON Schema as its plain-text
representation could be nice.

Then we have QemuOpts.  It's more limited (flat list instead of trees),
but it comes with a real data type to describe it (QemuOptsList).

There's also QEMUOptionParameter.  Serves a similar purpose.  I don't
know why we have both.

Over in qdev-land, we find Property, which can be viewed a data type to
describe (parts of) a struct.

Some unification would be nice.
Luiz Capitulino - June 28, 2010, 2:28 p.m.
On Wed, 23 Jun 2010 12:28:27 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Markus Armbruster wrote:
> > Jan Kiszka <jan.kiszka@web.de> writes:
> > 
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> This enables command line completion inside option strings. A list of
> >> expected key names and their completion type can be appended to the 'O'
> >> inside parentheses ('O(key:type,...)'). The first use case is block
> >> device completion for the 'drive' option of 'device_add'.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
> >>  qemu-monitor.hx |    2 +-
> >>  2 files changed, 58 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index c1006b4..3e0d862 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -68,6 +68,9 @@
> >>   * 'O'          option string of the form NAME=VALUE,...
> >>   *              parsed according to QemuOptsList given by its name
> >>   *              Example: 'device:O' uses qemu_device_opts.
> >> + *              Command completion for specific keys can be requested via
> >> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> >> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
> >>   *              Restriction: only lists with empty desc are supported
> >>   *              TODO lift the restriction
> >>   * 'i'          32 bit integer
> > 
> > Ugh.
> > 
> > Replacement of args_type by a proper data structure is long overdue.  We
> > keep piling features into that poor, hapless string.
> > 
> > Information on how to complete QemuOptsList options arguably belongs
> > into the option description, i.e. QemuOptDesc.
> 
> For sure, that would be better. I just wonder how much of it should be
> stuffed into this series. I guess I will drop this part for now, just
> focusing on what device_show makes direct use of. Same for separate args
> for HMP and QMP.

IIRC, the separate args idea use case was to allow commands like device_del
to have an ID argument and a path argument, right? If so, I think it doesn't
matter anymore as we have agreed on having a device argument which would
accept both, even under QMP, right Markus?

By the way, if you send patches 09/23, 10/23, 15/23, (maybe) 16/23, 21/23
and 22/23 in a different series, I could try pushing them in my next
pull request.
Jan Kiszka - June 28, 2010, 2:40 p.m.
Luiz Capitulino wrote:
> On Wed, 23 Jun 2010 12:28:27 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> This enables command line completion inside option strings. A list of
>>>> expected key names and their completion type can be appended to the 'O'
>>>> inside parentheses ('O(key:type,...)'). The first use case is block
>>>> device completion for the 'drive' option of 'device_add'.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>>>  qemu-monitor.hx |    2 +-
>>>>  2 files changed, 58 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index c1006b4..3e0d862 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -68,6 +68,9 @@
>>>>   * 'O'          option string of the form NAME=VALUE,...
>>>>   *              parsed according to QemuOptsList given by its name
>>>>   *              Example: 'device:O' uses qemu_device_opts.
>>>> + *              Command completion for specific keys can be requested via
>>>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
>>>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>>>>   *              Restriction: only lists with empty desc are supported
>>>>   *              TODO lift the restriction
>>>>   * 'i'          32 bit integer
>>> Ugh.
>>>
>>> Replacement of args_type by a proper data structure is long overdue.  We
>>> keep piling features into that poor, hapless string.
>>>
>>> Information on how to complete QemuOptsList options arguably belongs
>>> into the option description, i.e. QemuOptDesc.
>> For sure, that would be better. I just wonder how much of it should be
>> stuffed into this series. I guess I will drop this part for now, just
>> focusing on what device_show makes direct use of. Same for separate args
>> for HMP and QMP.
> 
> IIRC, the separate args idea use case was to allow commands like device_del
> to have an ID argument and a path argument, right? If so, I think it doesn't
> matter anymore as we have agreed on having a device argument which would
> accept both, even under QMP, right Markus?

To my understanding: As a leading element in qdev path, at least to
address a device, maybe also to abbreviate only the beginning of a full
path (that's currently to major remaining open issue).

> 
> By the way, if you send patches 09/23, 10/23, 15/23, (maybe) 16/23, 21/23
> and 22/23 in a different series, I could try pushing them in my next
> pull request.

Do they need rebasing? If not, feel free to pick them up as you like. My
series requires a v5 round anyway once discussion on path construction
finally came to an end.

Jan
Luiz Capitulino - June 28, 2010, 4:20 p.m.
On Mon, 28 Jun 2010 16:40:58 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Luiz Capitulino wrote:
> > On Wed, 23 Jun 2010 12:28:27 +0200
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > 
> >> Markus Armbruster wrote:
> >>> Jan Kiszka <jan.kiszka@web.de> writes:
> >>>
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> This enables command line completion inside option strings. A list of
> >>>> expected key names and their completion type can be appended to the 'O'
> >>>> inside parentheses ('O(key:type,...)'). The first use case is block
> >>>> device completion for the 'drive' option of 'device_add'.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> ---
> >>>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
> >>>>  qemu-monitor.hx |    2 +-
> >>>>  2 files changed, 58 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/monitor.c b/monitor.c
> >>>> index c1006b4..3e0d862 100644
> >>>> --- a/monitor.c
> >>>> +++ b/monitor.c
> >>>> @@ -68,6 +68,9 @@
> >>>>   * 'O'          option string of the form NAME=VALUE,...
> >>>>   *              parsed according to QemuOptsList given by its name
> >>>>   *              Example: 'device:O' uses qemu_device_opts.
> >>>> + *              Command completion for specific keys can be requested via
> >>>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> >>>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
> >>>>   *              Restriction: only lists with empty desc are supported
> >>>>   *              TODO lift the restriction
> >>>>   * 'i'          32 bit integer
> >>> Ugh.
> >>>
> >>> Replacement of args_type by a proper data structure is long overdue.  We
> >>> keep piling features into that poor, hapless string.
> >>>
> >>> Information on how to complete QemuOptsList options arguably belongs
> >>> into the option description, i.e. QemuOptDesc.
> >> For sure, that would be better. I just wonder how much of it should be
> >> stuffed into this series. I guess I will drop this part for now, just
> >> focusing on what device_show makes direct use of. Same for separate args
> >> for HMP and QMP.
> > 
> > IIRC, the separate args idea use case was to allow commands like device_del
> > to have an ID argument and a path argument, right? If so, I think it doesn't
> > matter anymore as we have agreed on having a device argument which would
> > accept both, even under QMP, right Markus?
> 
> To my understanding: As a leading element in qdev path, at least to
> address a device, maybe also to abbreviate only the beginning of a full
> path (that's currently to major remaining open issue).

I'm ok with it if it's unambiguous.

> > By the way, if you send patches 09/23, 10/23, 15/23, (maybe) 16/23, 21/23
> > and 22/23 in a different series, I could try pushing them in my next
> > pull request.
> 
> Do they need rebasing? If not, feel free to pick them up as you like. My
> series requires a v5 round anyway once discussion on path construction
> finally came to an end.

Done for them all, except 16/23 which mentions device_show in the changelog.

I should send a pull request until this Wednesday.

Patch

diff --git a/monitor.c b/monitor.c
index c1006b4..3e0d862 100644
--- a/monitor.c
+++ b/monitor.c
@@ -68,6 +68,9 @@ 
  * 'O'          option string of the form NAME=VALUE,...
  *              parsed according to QemuOptsList given by its name
  *              Example: 'device:O' uses qemu_device_opts.
+ *              Command completion for specific keys can be requested via
+ *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
+ *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
  *              Restriction: only lists with empty desc are supported
  *              TODO lift the restriction
  * 'i'          32 bit integer
@@ -3353,6 +3356,11 @@  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 QemuOptsList *opts_list;
                 QemuOpts *opts;
 
+                if (*typestr == '(') {
+                    while (*typestr++ != ')') {
+                        assert(*typestr != '\0');
+                    }
+                }
                 opts_list = qemu_find_opts(key);
                 if (!opts_list || opts_list->desc->name) {
                     goto bad_type;
@@ -3857,12 +3865,30 @@  static const char *next_arg_type(const char *typestr)
     return (p != NULL ? ++p : typestr);
 }
 
+static bool process_completion_type(char type, const char *str)
+{
+    switch(type) {
+    case 'F':
+        /* file completion */
+        readline_set_completion_index(cur_mon->rs, strlen(str));
+        file_completion(str);
+        return true;
+    case 'B':
+        /* block device name completion */
+        readline_set_completion_index(cur_mon->rs, strlen(str));
+        bdrv_iterate(block_completion_it, (void *)str);
+        return true;
+    default:
+        return false;
+    }
+}
+
 static void monitor_find_completion(const char *cmdline)
 {
     const char *cmdname;
     char *args[MAX_ARGS];
     int nb_args, i, len;
-    const char *ptype, *str;
+    const char *ptype, *str, *opt, *sep;
     const mon_cmd_t *cmd;
     const KeyDef *key;
 
@@ -3915,16 +3941,31 @@  static void monitor_find_completion(const char *cmdline)
         if (*ptype == '-' && ptype[1] != '\0') {
             ptype = next_arg_type(ptype);
         }
+        if (process_completion_type(*ptype, str)) {
+            goto cleanup;
+        }
         switch(*ptype) {
-        case 'F':
-            /* file completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            file_completion(str);
-            break;
-        case 'B':
-            /* block device name completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            bdrv_iterate(block_completion_it, (void *)str);
+        case 'O':
+            sep = strrchr(str, ',');
+            opt = sep ? sep + 1 : str;
+            sep = strchr(opt, '=');
+            if (!sep) {
+                break;
+            }
+            len = sep - opt;
+            str = sep + 1;
+            ptype += 2;
+            while (*ptype != ')') {
+                if (strlen(ptype) > len+1 && ptype[len] == ':' &&
+                    strncmp(ptype, opt, len) == 0) {
+                    process_completion_type(ptype[len+1], str);
+                }
+                while (*ptype++ != ',') {
+                    if (*ptype == ')') {
+                        break;
+                    }
+                }
+            }
             break;
         case 's':
             /* XXX: more generic ? */
@@ -3934,7 +3975,7 @@  static void monitor_find_completion(const char *cmdline)
                     cmd_completion(str, cmd->name);
                 }
             } else if (!strcmp(cmd->name, "sendkey")) {
-                char *sep = strrchr(str, '-');
+                sep = strrchr(str, '-');
                 if (sep)
                     str = sep + 1;
                 readline_set_completion_index(cur_mon->rs, strlen(str));
@@ -4114,6 +4155,11 @@  static int monitor_check_qmp_args(const mon_cmd_t *cmd, QDict *args)
                 cmd_args.flag = *p++;
                 cmd_args.optional = 1;
             } else if (cmd_args.type == 'O') {
+                if (*p == '(') {
+                    while (*p++ != ')') {
+                        assert(*p != '\0');
+                    }
+                }
                 opts_list = qemu_find_opts(qstring_get_str(cmd_args.name));
                 assert(opts_list);
             } else if (*p == '?') {
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 0ea0555..b5d0f6d 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -660,7 +660,7 @@  ETEXI
 
     {
         .name       = "device_add",
-        .args_type  = "device:O",
+        .args_type  = "device:O(drive:B)",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
         .user_print = monitor_user_noop,