Patchwork [07/13] Monitor: handle optional '-' arg as a bool

login
register
mail settings
Submitter Luiz Capitulino
Date June 22, 2010, 5:40 p.m.
Message ID <1277228451-7741-8-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/56555/
State New
Headers show

Comments

Luiz Capitulino - June 22, 2010, 5:40 p.m.
Historically, user monitor arguments beginning with '-' (eg. '-f')
were passed as integers down to handlers.

I've maintained this behavior in the new monitor because we didn't
have a boolean type at the very beginning of QMP. Today we have it
and this behavior is causing trouble to QMP's argument checker.

This commit fixes the problem by doing the following changes:

1. User Monitor

   Before: the optional arg was represented as a QInt, we'd pass 1
           down to handlers if the user specified the argument or
           0 otherwise

   This commit: the optional arg is represented as a QBool, we pass
                true down to handlers if the user specified the
                argument, otherwise _nothing_ is passed

2. QMP

   Before: the client was required to pass the arg as QBool, but we'd
           convert it to QInt internally. If the argument wasn't passed,
           we'd pass 0 down

   This commit: still require a QBool, but doesn't do any conversion and
                doesn't pass any default value

3. Convert existing handlers (do_eject()/do_migrate()) to the new way

   Before: Both handlers would expect a QInt value, either 0 or 1

   This commit: Change the handlers to accept a QBool, they handle the
                following cases:

                   A) true is passed: the option is enabled
                   B) false is passed: the option is disabled
                   C) nothing is passed: option not specified, use
                                         default behavior

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c  |    2 +-
 migration.c |   16 +++++++---------
 monitor.c   |   17 +++--------------
 3 files changed, 11 insertions(+), 24 deletions(-)
Markus Armbruster - June 23, 2010, 3:05 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> Historically, user monitor arguments beginning with '-' (eg. '-f')
> were passed as integers down to handlers.
>
> I've maintained this behavior in the new monitor because we didn't
> have a boolean type at the very beginning of QMP. Today we have it
> and this behavior is causing trouble to QMP's argument checker.
>
> This commit fixes the problem by doing the following changes:
>
> 1. User Monitor
>
>    Before: the optional arg was represented as a QInt, we'd pass 1
>            down to handlers if the user specified the argument or
>            0 otherwise
>
>    This commit: the optional arg is represented as a QBool, we pass
>                 true down to handlers if the user specified the
>                 argument, otherwise _nothing_ is passed
>
> 2. QMP
>
>    Before: the client was required to pass the arg as QBool, but we'd
>            convert it to QInt internally. If the argument wasn't passed,
>            we'd pass 0 down
>
>    This commit: still require a QBool, but doesn't do any conversion and
>                 doesn't pass any default value
>
> 3. Convert existing handlers (do_eject()/do_migrate()) to the new way
>
>    Before: Both handlers would expect a QInt value, either 0 or 1
>
>    This commit: Change the handlers to accept a QBool, they handle the
>                 following cases:
>
>                    A) true is passed: the option is enabled
>                    B) false is passed: the option is disabled
>                    C) nothing is passed: option not specified, use
>                                          default behavior

Because the user monitor can't pass false, the only sensible default
behavior is "disabled".
Luiz Capitulino - June 23, 2010, 4:14 p.m.
On Wed, 23 Jun 2010 17:05:02 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Historically, user monitor arguments beginning with '-' (eg. '-f')
> > were passed as integers down to handlers.
> >
> > I've maintained this behavior in the new monitor because we didn't
> > have a boolean type at the very beginning of QMP. Today we have it
> > and this behavior is causing trouble to QMP's argument checker.
> >
> > This commit fixes the problem by doing the following changes:
> >
> > 1. User Monitor
> >
> >    Before: the optional arg was represented as a QInt, we'd pass 1
> >            down to handlers if the user specified the argument or
> >            0 otherwise
> >
> >    This commit: the optional arg is represented as a QBool, we pass
> >                 true down to handlers if the user specified the
> >                 argument, otherwise _nothing_ is passed
> >
> > 2. QMP
> >
> >    Before: the client was required to pass the arg as QBool, but we'd
> >            convert it to QInt internally. If the argument wasn't passed,
> >            we'd pass 0 down
> >
> >    This commit: still require a QBool, but doesn't do any conversion and
> >                 doesn't pass any default value
> >
> > 3. Convert existing handlers (do_eject()/do_migrate()) to the new way
> >
> >    Before: Both handlers would expect a QInt value, either 0 or 1
> >
> >    This commit: Change the handlers to accept a QBool, they handle the
> >                 following cases:
> >
> >                    A) true is passed: the option is enabled
> >                    B) false is passed: the option is disabled
> >                    C) nothing is passed: option not specified, use
> >                                          default behavior
> 
> Because the user monitor can't pass false, the only sensible default
> behavior is "disabled".

Yes, but I think we shouldn't impose it. I mean, handlers are still free
to choose an 'enabled' default state.
Markus Armbruster - June 23, 2010, 4:31 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 23 Jun 2010 17:05:02 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > Historically, user monitor arguments beginning with '-' (eg. '-f')
>> > were passed as integers down to handlers.
>> >
>> > I've maintained this behavior in the new monitor because we didn't
>> > have a boolean type at the very beginning of QMP. Today we have it
>> > and this behavior is causing trouble to QMP's argument checker.
>> >
>> > This commit fixes the problem by doing the following changes:
>> >
>> > 1. User Monitor
>> >
>> >    Before: the optional arg was represented as a QInt, we'd pass 1
>> >            down to handlers if the user specified the argument or
>> >            0 otherwise
>> >
>> >    This commit: the optional arg is represented as a QBool, we pass
>> >                 true down to handlers if the user specified the
>> >                 argument, otherwise _nothing_ is passed
>> >
>> > 2. QMP
>> >
>> >    Before: the client was required to pass the arg as QBool, but we'd
>> >            convert it to QInt internally. If the argument wasn't passed,
>> >            we'd pass 0 down
>> >
>> >    This commit: still require a QBool, but doesn't do any conversion and
>> >                 doesn't pass any default value
>> >
>> > 3. Convert existing handlers (do_eject()/do_migrate()) to the new way
>> >
>> >    Before: Both handlers would expect a QInt value, either 0 or 1
>> >
>> >    This commit: Change the handlers to accept a QBool, they handle the
>> >                 following cases:
>> >
>> >                    A) true is passed: the option is enabled
>> >                    B) false is passed: the option is disabled
>> >                    C) nothing is passed: option not specified, use
>> >                                          default behavior
>> 
>> Because the user monitor can't pass false, the only sensible default
>> behavior is "disabled".
>
> Yes, but I think we shouldn't impose it. I mean, handlers are still free
> to choose an 'enabled' default state.

They are.  Renders the option entirely useless in the user monitor,
though.

I don't want you to change anything, I just wanted to point out that the
human monitor can't yet support boolean arguments defaulting to true.
QMP can, of course.
Luiz Capitulino - June 23, 2010, 4:43 p.m.
On Wed, 23 Jun 2010 18:31:53 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 23 Jun 2010 17:05:02 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > Historically, user monitor arguments beginning with '-' (eg. '-f')
> >> > were passed as integers down to handlers.
> >> >
> >> > I've maintained this behavior in the new monitor because we didn't
> >> > have a boolean type at the very beginning of QMP. Today we have it
> >> > and this behavior is causing trouble to QMP's argument checker.
> >> >
> >> > This commit fixes the problem by doing the following changes:
> >> >
> >> > 1. User Monitor
> >> >
> >> >    Before: the optional arg was represented as a QInt, we'd pass 1
> >> >            down to handlers if the user specified the argument or
> >> >            0 otherwise
> >> >
> >> >    This commit: the optional arg is represented as a QBool, we pass
> >> >                 true down to handlers if the user specified the
> >> >                 argument, otherwise _nothing_ is passed
> >> >
> >> > 2. QMP
> >> >
> >> >    Before: the client was required to pass the arg as QBool, but we'd
> >> >            convert it to QInt internally. If the argument wasn't passed,
> >> >            we'd pass 0 down
> >> >
> >> >    This commit: still require a QBool, but doesn't do any conversion and
> >> >                 doesn't pass any default value
> >> >
> >> > 3. Convert existing handlers (do_eject()/do_migrate()) to the new way
> >> >
> >> >    Before: Both handlers would expect a QInt value, either 0 or 1
> >> >
> >> >    This commit: Change the handlers to accept a QBool, they handle the
> >> >                 following cases:
> >> >
> >> >                    A) true is passed: the option is enabled
> >> >                    B) false is passed: the option is disabled
> >> >                    C) nothing is passed: option not specified, use
> >> >                                          default behavior
> >> 
> >> Because the user monitor can't pass false, the only sensible default
> >> behavior is "disabled".
> >
> > Yes, but I think we shouldn't impose it. I mean, handlers are still free
> > to choose an 'enabled' default state.
> 
> They are.  Renders the option entirely useless in the user monitor,
> though.
> 
> I don't want you to change anything, I just wanted to point out that the
> human monitor can't yet support boolean arguments defaulting to true.
> QMP can, of course.

Exactly, that was my point too.

Patch

diff --git a/blockdev.c b/blockdev.c
index b376884..5844eac 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -523,7 +523,7 @@  static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
 int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     BlockDriverState *bs;
-    int force = qdict_get_int(qdict, "force");
+    int force = qdict_get_try_bool(qdict, "force", 0);
     const char *filename = qdict_get_str(qdict, "device");
 
     bs = bdrv_find(filename);
diff --git a/migration.c b/migration.c
index 64ed36e..4f466b7 100644
--- a/migration.c
+++ b/migration.c
@@ -58,7 +58,9 @@  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = NULL;
     const char *p;
-    int detach = qdict_get_int(qdict, "detach");
+    int detach = qdict_get_try_bool(qdict, "detach", 0);
+    int blk = qdict_get_try_bool(qdict, "blk", 0);
+    int inc = qdict_get_try_bool(qdict, "inc", 0);
     const char *uri = qdict_get_str(qdict, "uri");
 
     if (current_migration &&
@@ -69,21 +71,17 @@  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
     if (strstart(uri, "tcp:", &p)) {
         s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
-                                         (int)qdict_get_int(qdict, "blk"), 
-                                         (int)qdict_get_int(qdict, "inc"));
+                                         blk, inc);
 #if !defined(WIN32)
     } else if (strstart(uri, "exec:", &p)) {
         s = exec_start_outgoing_migration(mon, p, max_throttle, detach,
-                                          (int)qdict_get_int(qdict, "blk"), 
-                                          (int)qdict_get_int(qdict, "inc"));
+                                          blk, inc);
     } else if (strstart(uri, "unix:", &p)) {
         s = unix_start_outgoing_migration(mon, p, max_throttle, detach,
-					  (int)qdict_get_int(qdict, "blk"), 
-                                          (int)qdict_get_int(qdict, "inc"));
+                                          blk, inc);
     } else if (strstart(uri, "fd:", &p)) {
         s = fd_start_outgoing_migration(mon, p, max_throttle, detach, 
-                                        (int)qdict_get_int(qdict, "blk"), 
-                                        (int)qdict_get_int(qdict, "inc"));
+                                        blk, inc);
 #endif
     } else {
         monitor_printf(mon, "unknown migration protocol: %s\n", uri);
diff --git a/monitor.c b/monitor.c
index 05a7ed1..3df8174 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3560,7 +3560,7 @@  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
         case '-':
             {
                 const char *tmp = p;
-                int has_option, skip_key = 0;
+                int skip_key = 0;
                 /* option */
 
                 c = *typestr++;
@@ -3568,7 +3568,6 @@  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                     goto bad_type;
                 while (qemu_isspace(*p))
                     p++;
-                has_option = 0;
                 if (*p == '-') {
                     p++;
                     if(c != *p) {
@@ -3584,11 +3583,11 @@  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                     if(skip_key) {
                         p = tmp;
                     } else {
+                        /* has option */
                         p++;
-                        has_option = 1;
+                        qdict_put(qdict, key, qbool_from_int(1));
                     }
                 }
-                qdict_put(qdict, key, qint_from_int(has_option));
             }
             break;
         default:
@@ -3973,11 +3972,6 @@  static int check_opt(const CmdArgs *cmd_args, const char *name, QDict *args)
         return -1;
     }
 
-    if (cmd_args->type == '-') {
-        /* handlers expect a value, they need to be changed */
-        qdict_put(args, name, qint_from_int(0));
-    }
-
     return 0;
 }
 
@@ -4050,11 +4044,6 @@  static int check_arg(const CmdArgs *cmd_args, QDict *args)
                 qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "bool");
                 return -1;
             }
-            if (qobject_type(value) == QTYPE_QBOOL) {
-                /* handlers expect a QInt, they need to be changed */
-                qdict_put(args, name,
-                         qint_from_int(qbool_get_int(qobject_to_qbool(value))));
-            }
             break;
         case 'O':
         default: