Patchwork [2/4] monitor: New argument type 'b'

login
register
mail settings
Submitter Markus Armbruster
Date March 23, 2010, 10:27 a.m.
Message ID <1269340078-16446-3-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/48331/
State New
Headers show

Comments

Markus Armbruster - March 23, 2010, 10:27 a.m.
This is a boolean value.  Human monitor accepts "on" or "off".
Consistent with option parsing (see parse_option_bool()).

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)
Luiz Capitulino - March 24, 2010, 7:31 p.m.
On Tue, 23 Mar 2010 11:27:56 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> This is a boolean value.  Human monitor accepts "on" or "off".
> Consistent with option parsing (see parse_option_bool()).
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c |   31 +++++++++++++++++++++++++++++++
>  1 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 3ce9a4e..47b68a2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -85,6 +85,8 @@
>   *
>   * '?'          optional type (for all types, except '/')
>   * '.'          other form of optional type (for 'i' and 'l')
> + * 'b'          boolean
> + *              user mode accepts "on" or "off"
>   * '-'          optional parameter (eg. '-f')
>   *
>   */
> @@ -3841,6 +3843,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                  qdict_put(qdict, key, qfloat_from_double(val));
>              }
>              break;
> +        case 'b':
> +            {
> +                const char *beg;
> +                int val;
> +
> +                while (qemu_isspace(*p)) {
> +                    p++;
> +                }
> +                beg = p;
> +                while (qemu_isgraph(*p)) {
> +                    p++;
> +                }
> +                if (!strncmp(beg, "on", p - beg)) {
> +                    val = 1;
> +                } else if (!strncmp(beg, "off", p - beg)) {
> +                    val = 0;
> +                } else {
> +                    monitor_printf(mon, "Expected 'on' or 'off'\n");
> +                    goto fail;
> +                }

 This will make 'on' be the default when no on/off is specified, is that
your intention? I'm wondering if this can cause problems when you add
optional support for it and mixes it with other arguments.

> +                qdict_put(qdict, key, qbool_from_int(val));
> +            }
> +            break;
>          case '-':
>              {
>                  const char *tmp = p;
> @@ -4322,6 +4347,12 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
>                  return -1;
>              }
>              break;
> +        case 'b':
> +            if (qobject_type(value) != QTYPE_QBOOL) {
> +                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "bool");
> +                return -1;
> +            }
> +            break;
>          case '-':
>              if (qobject_type(value) != QTYPE_QINT &&
>                  qobject_type(value) != QTYPE_QBOOL) {
Markus Armbruster - March 25, 2010, 5:28 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 23 Mar 2010 11:27:56 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> This is a boolean value.  Human monitor accepts "on" or "off".
>> Consistent with option parsing (see parse_option_bool()).
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c |   31 +++++++++++++++++++++++++++++++
>>  1 files changed, 31 insertions(+), 0 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 3ce9a4e..47b68a2 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -85,6 +85,8 @@
>>   *
>>   * '?'          optional type (for all types, except '/')
>>   * '.'          other form of optional type (for 'i' and 'l')
>> + * 'b'          boolean
>> + *              user mode accepts "on" or "off"
>>   * '-'          optional parameter (eg. '-f')
>>   *
>>   */
>> @@ -3841,6 +3843,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                  qdict_put(qdict, key, qfloat_from_double(val));
>>              }
>>              break;
>> +        case 'b':
>> +            {
>> +                const char *beg;
>> +                int val;
>> +
>> +                while (qemu_isspace(*p)) {
>> +                    p++;
>> +                }
>> +                beg = p;
>> +                while (qemu_isgraph(*p)) {
>> +                    p++;
>> +                }
>> +                if (!strncmp(beg, "on", p - beg)) {
>> +                    val = 1;
>> +                } else if (!strncmp(beg, "off", p - beg)) {
>> +                    val = 0;
>> +                } else {
>> +                    monitor_printf(mon, "Expected 'on' or 'off'\n");
>> +                    goto fail;
>> +                }
>
>  This will make 'on' be the default when no on/off is specified, is that
> your intention? I'm wondering if this can cause problems when you add
> optional support for it and mixes it with other arguments.

No.  Intended behavior: the argument must be either "on" or "off".  With
"on", (KEY, true) is put into the dictionary, for "off" it's (KEY,
false).

We get a third case for optional argument if we support that: KEY not in
dictionary.  The handler decides how to interpret that.

[...]
Luiz Capitulino - March 25, 2010, 5:39 p.m.
On Thu, 25 Mar 2010 18:28:43 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Tue, 23 Mar 2010 11:27:56 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> This is a boolean value.  Human monitor accepts "on" or "off".
> >> Consistent with option parsing (see parse_option_bool()).
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  monitor.c |   31 +++++++++++++++++++++++++++++++
> >>  1 files changed, 31 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/monitor.c b/monitor.c
> >> index 3ce9a4e..47b68a2 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -85,6 +85,8 @@
> >>   *
> >>   * '?'          optional type (for all types, except '/')
> >>   * '.'          other form of optional type (for 'i' and 'l')
> >> + * 'b'          boolean
> >> + *              user mode accepts "on" or "off"
> >>   * '-'          optional parameter (eg. '-f')
> >>   *
> >>   */
> >> @@ -3841,6 +3843,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>                  qdict_put(qdict, key, qfloat_from_double(val));
> >>              }
> >>              break;
> >> +        case 'b':
> >> +            {
> >> +                const char *beg;
> >> +                int val;
> >> +
> >> +                while (qemu_isspace(*p)) {
> >> +                    p++;
> >> +                }
> >> +                beg = p;
> >> +                while (qemu_isgraph(*p)) {
> >> +                    p++;
> >> +                }
> >> +                if (!strncmp(beg, "on", p - beg)) {
> >> +                    val = 1;
> >> +                } else if (!strncmp(beg, "off", p - beg)) {
> >> +                    val = 0;
> >> +                } else {
> >> +                    monitor_printf(mon, "Expected 'on' or 'off'\n");
> >> +                    goto fail;
> >> +                }
> >
> >  This will make 'on' be the default when no on/off is specified, is that
> > your intention? I'm wondering if this can cause problems when you add
> > optional support for it and mixes it with other arguments.
> 
> No.  Intended behavior: the argument must be either "on" or "off".  With
> "on", (KEY, true) is put into the dictionary, for "off" it's (KEY,
> false).
> 
> We get a third case for optional argument if we support that: KEY not in
> dictionary.  The handler decides how to interpret that.

 Ok, but strncmp() will return 0 if p - beg = 0, right? In this
case the current implementation will put true on the dict for a line like:

(qemu) set_link foo

 Which should return an error to the user then.
Markus Armbruster - March 26, 2010, 7:43 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

>  Ok, but strncmp() will return 0 if p - beg = 0, right? In this
> case the current implementation will put true on the dict for a line like:
>
> (qemu) set_link foo
>
>  Which should return an error to the user then.

Brain fart.  I'll respin.  Thanks for catching this!

Patch

diff --git a/monitor.c b/monitor.c
index 3ce9a4e..47b68a2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -85,6 +85,8 @@ 
  *
  * '?'          optional type (for all types, except '/')
  * '.'          other form of optional type (for 'i' and 'l')
+ * 'b'          boolean
+ *              user mode accepts "on" or "off"
  * '-'          optional parameter (eg. '-f')
  *
  */
@@ -3841,6 +3843,29 @@  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 qdict_put(qdict, key, qfloat_from_double(val));
             }
             break;
+        case 'b':
+            {
+                const char *beg;
+                int val;
+
+                while (qemu_isspace(*p)) {
+                    p++;
+                }
+                beg = p;
+                while (qemu_isgraph(*p)) {
+                    p++;
+                }
+                if (!strncmp(beg, "on", p - beg)) {
+                    val = 1;
+                } else if (!strncmp(beg, "off", p - beg)) {
+                    val = 0;
+                } else {
+                    monitor_printf(mon, "Expected 'on' or 'off'\n");
+                    goto fail;
+                }
+                qdict_put(qdict, key, qbool_from_int(val));
+            }
+            break;
         case '-':
             {
                 const char *tmp = p;
@@ -4322,6 +4347,12 @@  static int check_arg(const CmdArgs *cmd_args, QDict *args)
                 return -1;
             }
             break;
+        case 'b':
+            if (qobject_type(value) != QTYPE_QBOOL) {
+                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "bool");
+                return -1;
+            }
+            break;
         case '-':
             if (qobject_type(value) != QTYPE_QINT &&
                 qobject_type(value) != QTYPE_QBOOL) {