[Qemu-devel] [PATCH v2 00/13]: QMP: Replace client argument checker

[Qemu-devel] [PATCH v2 00/13]: QMP: Replace client argument checker

[Luiz Capitulino]

Current QMP's client argument checker code is more complex than it should be
and has a flaw: it ignores unknown arguments.

This series solves both problems by introducing a new, simple and ultra-poweful
argument checker. This wasn't trivial to get right due to the number of errors
combinations, so review is very appreciated.

changelog
---------

v1 -> v2

- Introduce new iteration API and use it
- Handle O-type correctly (I hope so)
- Address several small issues found by Markus

[Qemu-devel] [PATCH 01/13] QDict: Rename 'err_value'

[Luiz Capitulino]

A missing key is not an error.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qdict.c |    6 +++---
 qdict.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qdict.c b/qdict.c
index 175bc17..c974d6f 100644
--- a/qdict.c
+++ b/qdict.c
@@ -272,16 +272,16 @@ const char *qdict_get_str(const QDict *qdict, const char *key)
  *
  * Return integer mapped by 'key', if it is not present in
  * the dictionary or if the stored object is not of QInt type
- * 'err_value' will be returned.
+ * 'def_value' will be returned.
  */
 int64_t qdict_get_try_int(const QDict *qdict, const char *key,
-                          int64_t err_value)
+                          int64_t def_value)
 {
     QObject *obj;
 
     obj = qdict_get(qdict, key);
     if (!obj || qobject_type(obj) != QTYPE_QINT)
-        return err_value;
+        return def_value;
 
     return qint_get_int(qobject_to_qint(obj));
 }
diff --git a/qdict.h b/qdict.h
index 5e5902c..72ea563 100644
--- a/qdict.h
+++ b/qdict.h
@@ -56,7 +56,7 @@ QList *qdict_get_qlist(const QDict *qdict, const char *key);
 QDict *qdict_get_qdict(const QDict *qdict, const char *key);
 const char *qdict_get_str(const QDict *qdict, const char *key);
 int64_t qdict_get_try_int(const QDict *qdict, const char *key,
-                          int64_t err_value);
+                          int64_t def_value);
 const char *qdict_get_try_str(const QDict *qdict, const char *key);
 
 #endif /* QDICT_H */
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 02/13] QDict: Small terminology change

[Luiz Capitulino]

Let's call a 'hash' only what is returned by our hash function,
anything else is a 'bucket'.

This helps avoiding confusion with regard to how we traverse
our table.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 check-qdict.c |    2 +-
 qdict.c       |   24 ++++++++++++------------
 qdict.h       |    4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/check-qdict.c b/check-qdict.c
index 2c3089f..1b070f4 100644
--- a/check-qdict.c
+++ b/check-qdict.c
@@ -50,7 +50,7 @@ START_TEST(qdict_put_obj_test)
     qdict_put_obj(qdict, "", QOBJECT(qint_from_int(num)));
 
     fail_unless(qdict_size(qdict) == 1);
-    ent = QLIST_FIRST(&qdict->table[12345 % QDICT_HASH_SIZE]);
+    ent = QLIST_FIRST(&qdict->table[12345 % QDICT_BUCKET_MAX]);
     qi = qobject_to_qint(ent->value);
     fail_unless(qint_get_int(qi) == num);
 
diff --git a/qdict.c b/qdict.c
index c974d6f..71be2eb 100644
--- a/qdict.c
+++ b/qdict.c
@@ -86,11 +86,11 @@ static QDictEntry *alloc_entry(const char *key, QObject *value)
  * qdict_find(): List lookup function
  */
 static QDictEntry *qdict_find(const QDict *qdict,
-                              const char *key, unsigned int hash)
+                              const char *key, unsigned int bucket)
 {
     QDictEntry *entry;
 
-    QLIST_FOREACH(entry, &qdict->table[hash], next)
+    QLIST_FOREACH(entry, &qdict->table[bucket], next)
         if (!strcmp(entry->key, key))
             return entry;
 
@@ -110,11 +110,11 @@ static QDictEntry *qdict_find(const QDict *qdict,
  */
 void qdict_put_obj(QDict *qdict, const char *key, QObject *value)
 {
-    unsigned int hash;
+    unsigned int bucket;
     QDictEntry *entry;
 
-    hash = tdb_hash(key) % QDICT_HASH_SIZE;
-    entry = qdict_find(qdict, key, hash);
+    bucket = tdb_hash(key) % QDICT_BUCKET_MAX;
+    entry = qdict_find(qdict, key, bucket);
     if (entry) {
         /* replace key's value */
         qobject_decref(entry->value);
@@ -122,7 +122,7 @@ void qdict_put_obj(QDict *qdict, const char *key, QObject *value)
     } else {
         /* allocate a new entry */
         entry = alloc_entry(key, value);
-        QLIST_INSERT_HEAD(&qdict->table[hash], entry, next);
+        QLIST_INSERT_HEAD(&qdict->table[bucket], entry, next);
         qdict->size++;
     }
 }
@@ -137,7 +137,7 @@ QObject *qdict_get(const QDict *qdict, const char *key)
 {
     QDictEntry *entry;
 
-    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_HASH_SIZE);
+    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_BUCKET_MAX);
     return (entry == NULL ? NULL : entry->value);
 }
 
@@ -148,8 +148,8 @@ QObject *qdict_get(const QDict *qdict, const char *key)
  */
 int qdict_haskey(const QDict *qdict, const char *key)
 {
-    unsigned int hash = tdb_hash(key) % QDICT_HASH_SIZE;
-    return (qdict_find(qdict, key, hash) == NULL ? 0 : 1);
+    unsigned int bucket = tdb_hash(key) % QDICT_BUCKET_MAX;
+    return (qdict_find(qdict, key, bucket) == NULL ? 0 : 1);
 }
 
 /**
@@ -318,7 +318,7 @@ void qdict_iter(const QDict *qdict,
     int i;
     QDictEntry *entry;
 
-    for (i = 0; i < QDICT_HASH_SIZE; i++) {
+    for (i = 0; i < QDICT_BUCKET_MAX; i++) {
         QLIST_FOREACH(entry, &qdict->table[i], next)
             iter(entry->key, entry->value, opaque);
     }
@@ -347,7 +347,7 @@ void qdict_del(QDict *qdict, const char *key)
 {
     QDictEntry *entry;
 
-    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_HASH_SIZE);
+    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_BUCKET_MAX);
     if (entry) {
         QLIST_REMOVE(entry, next);
         qentry_destroy(entry);
@@ -366,7 +366,7 @@ static void qdict_destroy_obj(QObject *obj)
     assert(obj != NULL);
     qdict = qobject_to_qdict(obj);
 
-    for (i = 0; i < QDICT_HASH_SIZE; i++) {
+    for (i = 0; i < QDICT_BUCKET_MAX; i++) {
         QDictEntry *entry = QLIST_FIRST(&qdict->table[i]);
         while (entry) {
             QDictEntry *tmp = QLIST_NEXT(entry, next);
diff --git a/qdict.h b/qdict.h
index 72ea563..dcd2b29 100644
--- a/qdict.h
+++ b/qdict.h
@@ -18,7 +18,7 @@
 #include "qemu-queue.h"
 #include <stdint.h>
 
-#define QDICT_HASH_SIZE 512
+#define QDICT_BUCKET_MAX 512
 
 typedef struct QDictEntry {
     char *key;
@@ -29,7 +29,7 @@ typedef struct QDictEntry {
 typedef struct QDict {
     QObject_HEAD;
     size_t size;
-    QLIST_HEAD(,QDictEntry) table[QDICT_HASH_SIZE];
+    QLIST_HEAD(,QDictEntry) table[QDICT_BUCKET_MAX];
 } QDict;
 
 /* Object API */
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 03/13] QDict: Introduce functions to retrieve QDictEntry values

[Luiz Capitulino]

Next commit will introduce a new QDict iteration API which
returns QDictEntry entries, but we don't want users to directly
access its members since QDictEntry should be private to QDict.

In the near future this kind of data type will be turned into a
forward reference.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qdict.c |   21 +++++++++++++++++++++
 qdict.h |    2 ++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/qdict.c b/qdict.c
index 71be2eb..c467763 100644
--- a/qdict.c
+++ b/qdict.c
@@ -83,6 +83,27 @@ static QDictEntry *alloc_entry(const char *key, QObject *value)
 }
 
 /**
+ * qdict_entry_value(): Return qdict entry value
+ *
+ * Return weak reference.
+ */
+QObject *qdict_entry_value(const QDictEntry *entry)
+{
+    return entry->value;
+}
+
+/**
+ * qdict_entry_key(): Return qdict entry key
+ *
+ * Return a *pointer* to the string, it has to be duplicated before being
+ * stored.
+ */
+const char *qdict_entry_key(const QDictEntry *entry)
+{
+    return entry->key;
+}
+
+/**
  * qdict_find(): List lookup function
  */
 static QDictEntry *qdict_find(const QDict *qdict,
diff --git a/qdict.h b/qdict.h
index dcd2b29..0c8de3c 100644
--- a/qdict.h
+++ b/qdict.h
@@ -34,6 +34,8 @@ typedef struct QDict {
 
 /* Object API */
 QDict *qdict_new(void);
+const char *qdict_entry_key(const QDictEntry *entry);
+QObject *qdict_entry_value(const QDictEntry *entry);
 size_t qdict_size(const QDict *qdict);
 void qdict_put_obj(QDict *qdict, const char *key, QObject *value);
 void qdict_del(QDict *qdict, const char *key);
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 04/13] QDict: Introduce new iteration API

[Luiz Capitulino]

It's composed of functions qdict_first() and qdict_next(), plus
functions to access QDictEntry values.

This API was suggested by Markus Armbruster <armbru@redhat.com> and
it offers full control over the iteration process.

The usage is simple, the following example prints all keys in 'qdict'
(it's hopefully better than any English description):

   QDict *qdict;
   const QDictEntry *ent;

   [...]

   for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
        printf("%s ", qdict_entry_key(ent));
    }

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qdict.c |   37 +++++++++++++++++++++++++++++++++++++
 qdict.h |    2 ++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/qdict.c b/qdict.c
index c467763..a28a0a9 100644
--- a/qdict.c
+++ b/qdict.c
@@ -345,6 +345,43 @@ void qdict_iter(const QDict *qdict,
     }
 }
 
+static QDictEntry *qdict_next_entry(const QDict *qdict, int first_bucket)
+{
+    int i;
+
+    for (i = first_bucket; i < QDICT_BUCKET_MAX; i++) {
+        if (!QLIST_EMPTY(&qdict->table[i])) {
+            return QLIST_FIRST(&qdict->table[i]);
+        }
+    }
+
+    return NULL;
+}
+
+/**
+ * qdict_first(): Return first qdict entry for iteration.
+ */
+const QDictEntry *qdict_first(const QDict *qdict)
+{
+    return qdict_next_entry(qdict, 0);
+}
+
+/**
+ * qdict_next(): Return next qdict entry in an iteration.
+ */
+const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry)
+{
+    QDictEntry *ret;
+
+    ret = QLIST_NEXT(entry, next);
+    if (!ret) {
+        unsigned int bucket = tdb_hash(entry->key) % QDICT_BUCKET_MAX;
+        ret = qdict_next_entry(qdict, bucket + 1);
+    }
+
+    return ret;
+}
+
 /**
  * qentry_destroy(): Free all the memory allocated by a QDictEntry
  */
diff --git a/qdict.h b/qdict.h
index 0c8de3c..0e7a43f 100644
--- a/qdict.h
+++ b/qdict.h
@@ -45,6 +45,8 @@ QDict *qobject_to_qdict(const QObject *obj);
 void qdict_iter(const QDict *qdict,
                 void (*iter)(const char *key, QObject *obj, void *opaque),
                 void *opaque);
+const QDictEntry *qdict_first(const QDict *qdict);
+const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry);
 
 /* Helper to qdict_put_obj(), accepts any object */
 #define qdict_put(qdict, key, obj) \
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 05/13] check-qdict: Introduce test for the new iteration API

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 check-qdict.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/check-qdict.c b/check-qdict.c
index 1b070f4..6afce5a 100644
--- a/check-qdict.c
+++ b/check-qdict.c
@@ -194,6 +194,36 @@ START_TEST(qobject_to_qdict_test)
 }
 END_TEST
 
+START_TEST(qdict_iterapi_test)
+{
+    int count;
+    const QDictEntry *ent;
+
+    fail_unless(qdict_first(tests_dict) == NULL);
+
+    qdict_put(tests_dict, "key1", qint_from_int(1));
+    qdict_put(tests_dict, "key2", qint_from_int(2));
+    qdict_put(tests_dict, "key3", qint_from_int(3));
+
+    count = 0;
+    for (ent = qdict_first(tests_dict); ent; ent = qdict_next(tests_dict, ent)){
+        fail_unless(qdict_haskey(tests_dict, qdict_entry_key(ent)) == 1);
+        count++;
+    }
+
+    fail_unless(count == qdict_size(tests_dict));
+
+    /* Do it again to test restarting */
+    count = 0;
+    for (ent = qdict_first(tests_dict); ent; ent = qdict_next(tests_dict, ent)){
+        fail_unless(qdict_haskey(tests_dict, qdict_entry_key(ent)) == 1);
+        count++;
+    }
+
+    fail_unless(count == qdict_size(tests_dict));
+}
+END_TEST
+
 /*
  * Errors test-cases
  */
@@ -338,6 +368,7 @@ static Suite *qdict_suite(void)
     tcase_add_test(qdict_public2_tcase, qdict_haskey_test);
     tcase_add_test(qdict_public2_tcase, qdict_del_test);
     tcase_add_test(qdict_public2_tcase, qobject_to_qdict_test);
+    tcase_add_test(qdict_public2_tcase, qdict_iterapi_test);
 
     qdict_errors_tcase = tcase_create("Errors");
     suite_add_tcase(s, qdict_errors_tcase);
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 06/13] QDict: Introduce qdict_get_try_bool()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qdict.c |   18 ++++++++++++++++++
 qdict.h |    1 +
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/qdict.c b/qdict.c
index a28a0a9..dee0fb4 100644
--- a/qdict.c
+++ b/qdict.c
@@ -308,6 +308,24 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
 }
 
 /**
+ * qdict_get_try_bool(): Try to get a bool mapped by 'key'
+ *
+ * Return bool mapped by 'key', if it is not present in the
+ * dictionary or if the stored object is not of QBool type
+ * 'def_value' will be returned.
+ */
+int qdict_get_try_bool(const QDict *qdict, const char *key, int def_value)
+{
+    QObject *obj;
+
+    obj = qdict_get(qdict, key);
+    if (!obj || qobject_type(obj) != QTYPE_QBOOL)
+        return def_value;
+
+    return qbool_get_int(qobject_to_qbool(obj));
+}
+
+/**
  * qdict_get_try_str(): Try to get a pointer to the stored string
  * mapped by 'key'
  *
diff --git a/qdict.h b/qdict.h
index 0e7a43f..929d8d2 100644
--- a/qdict.h
+++ b/qdict.h
@@ -61,6 +61,7 @@ QDict *qdict_get_qdict(const QDict *qdict, const char *key);
 const char *qdict_get_str(const QDict *qdict, const char *key);
 int64_t qdict_get_try_int(const QDict *qdict, const char *key,
                           int64_t def_value);
+int qdict_get_try_bool(const QDict *qdict, const char *key, int def_value);
 const char *qdict_get_try_str(const QDict *qdict, const char *key);
 
 #endif /* QDICT_H */
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 07/13] Monitor: handle optional '-' arg as a bool

[Luiz Capitulino]

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(-)

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:
-- 
1.7.1.359.gd0b8d

Re: [Qemu-devel] [PATCH 07/13] Monitor: handle optional '-' arg as a bool

[Markus Armbruster]

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".

Re: [Qemu-devel] [PATCH 07/13] Monitor: handle optional '-' arg as a bool

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH 07/13] Monitor: handle optional '-' arg as a bool

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 07/13] Monitor: handle optional '-' arg as a bool

[Luiz Capitulino]

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.

[Qemu-devel] [PATCH 08/13] QMP: New argument checker (first part)

[Luiz Capitulino]

Current QMP's argument checker is more complex than it should be
and has (at least) one serious bug: it ignores unknown arguments.

To solve both problems we introduce a new argument checker. It's
added on top of the existing one, so that there are no regressions
during the transition.

This commit introduces the first part of the new checker, which
is run by qmp_check_client_args() and does the following:

  1. Check if all mandatory arguments were provided
  2. Set flags for argument validation

In order to do that, we transform the args_type string (from
qemu-montor.hx) into a qdict and iterate over it.

Next commit adds the new checker's second part: type checking and
invalid argument detection.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 106 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3df8174..b4fe5ba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -177,6 +177,9 @@ static inline void mon_print_count_init(Monitor *mon) { }
 static inline int mon_print_count_get(const Monitor *mon) { return 0; }
 #endif /* CONFIG_DEBUG_MONITOR */
 
+/* QMP checker flags */
+#define QMP_CHECKER_OTYPE 1  /* O-type argument is present */
+
 static QLIST_HEAD(mon_list, Monitor) mon_list;
 
 static const mon_cmd_t mon_cmds[];
@@ -4135,6 +4138,104 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
     return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
 }
 
+/*
+ * - Check if the client has passed all mandatory args
+ * - Set special flags for argument validation
+ */
+static int check_mandatory_args(const QDict *cmd_args,
+                                const QDict *client_args, int *flags)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(cmd_args); ent; ent = qdict_next(cmd_args, ent)) {
+        const char *cmd_arg_name = qdict_entry_key(ent);
+        QString *type = qobject_to_qstring(qdict_entry_value(ent));
+        assert(type != NULL);
+
+        if (qstring_get_str(type)[0] == 'O') {
+            assert((*flags & QMP_CHECKER_OTYPE) == 0);
+            *flags |= QMP_CHECKER_OTYPE;
+        } else if (qstring_get_str(type)[0] != '-' &&
+                   qstring_get_str(type)[1] != '?' &&
+                   !qdict_haskey(client_args, cmd_arg_name)) {
+            qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+static QDict *qdict_from_args_type(const char *args_type)
+{
+    int i;
+    QDict *qdict;
+    QString *key, *type, *cur_qs;
+
+    assert(args_type != NULL);
+
+    qdict = qdict_new();
+
+    if (args_type == NULL || args_type[0] == '\0') {
+        /* no args, empty qdict */
+        goto out;
+    }
+
+    key = qstring_new();
+    type = qstring_new();
+
+    cur_qs = key;
+
+    for (i = 0;; i++) {
+        switch (args_type[i]) {
+            case ',':
+            case '\0':
+                qdict_put(qdict, qstring_get_str(key), type);
+                QDECREF(key);
+                if (args_type[i] == '\0') {
+                    goto out;
+                }
+                type = qstring_new(); /* qdict has ref */
+                cur_qs = key = qstring_new();
+                break;
+            case ':':
+                cur_qs = type;
+                break;
+            default:
+                qstring_append_chr(cur_qs, args_type[i]);
+                break;
+        }
+    }
+
+out:
+    return qdict;
+}
+
+/*
+ * Client argument checking rules:
+ *
+ * 1. Client must provide all mandatory arguments
+ */
+static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
+{
+    int flags, err;
+    QDict *cmd_args;
+
+    cmd_args = qdict_from_args_type(cmd->args_type);
+
+    flags = 0;
+    err = check_mandatory_args(cmd_args, client_args, &flags);
+    if (err) {
+        goto out;
+    }
+
+    /* TODO: Check client args type */
+
+out:
+    QDECREF(cmd_args);
+    return err;
+}
+
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
@@ -4210,6 +4311,11 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 
     QDECREF(input);
 
+    err = qmp_check_client_args(cmd, args);
+    if (err < 0) {
+        goto err_out;
+    }
+
     err = monitor_check_qmp_args(cmd, args);
     if (err < 0) {
         goto err_out;
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 09/13] QMP: New argument checker (second part)

[Luiz Capitulino]

This commit introduces the second (and last) part of QMP's new
argument checker.

The job is done by check_client_args_type(), it iterates over
the client's argument qdict and for for each argument it checks
if it exists and if its type is valid.

It's important to observe the following changes from the existing
argument checker:

  - If the handler accepts an O-type argument, unknown arguments
    are passed down to it. It's up to O-type handlers to validate
    their arguments

  - Boolean types (eg. 'b' and '-') don't accept integers anymore,
    only json-bool

  - Argument types '/' and '.' are currently unsupported under QMP,
    thus they're not handled

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 99 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index b4fe5ba..8d074c2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4139,6 +4139,101 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
 }
 
 /*
+ * Argument validation rules:
+ *
+ * 1. The argument must exist in cmd_args qdict
+ * 2. The argument type must be the expected one
+ *
+ * Special case: If the argument doesn't exist in cmd_args and
+ *               the QMP_CHECKER_OTYPE flag is set, then the
+ *               argument is considered an O-type one and the
+ *               checking is skipped for it.
+ */
+static int check_client_args_type(const QDict *client_args,
+                                  const QDict *cmd_args, int flags)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(client_args); ent;ent = qdict_next(client_args,ent)){
+        QObject *obj;
+        QString *arg_type;
+        const QObject *client_arg = qdict_entry_value(ent);
+        const char *client_arg_name = qdict_entry_key(ent);
+
+        obj = qdict_get(cmd_args, client_arg_name);
+        if (!obj) {
+            if (flags & QMP_CHECKER_OTYPE) {
+                /*
+                 * This handler accepts O-type arguments, it's up to it to
+                 * check for unknowns and validate its type.
+                 */
+                continue;
+            }
+            /* client arg doesn't exist */
+            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
+            return -1;
+        }
+
+        arg_type = qobject_to_qstring(obj);
+        assert(arg_type != NULL);
+
+        /* check if argument's type is correct */
+        switch (qstring_get_str(arg_type)[0]) {
+        case 'F':
+        case 'B':
+        case 's':
+            if (qobject_type(client_arg) != QTYPE_QSTRING) {
+                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+                              "string");
+                return -1;
+            }
+        break;
+        case 'i':
+        case 'l':
+        case 'M':
+            if (qobject_type(client_arg) != QTYPE_QINT) {
+                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+                              "int");
+                return -1; 
+            }
+            break;
+        case 'f':
+        case 'T':
+            if (qobject_type(client_arg) != QTYPE_QINT &&
+                qobject_type(client_arg) != QTYPE_QFLOAT) {
+                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+                              "number");
+               return -1; 
+            }
+            break;
+        case 'b':
+        case '-':
+            if (qobject_type(client_arg) != QTYPE_QBOOL) {
+                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+                              "bool");
+               return -1; 
+            }
+            break;
+        case 'O':
+            /* XXX: this argument has the same name of the O-type defined in
+                    in qemu-monitor.hx. This is not allowed, right? */
+            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
+            return -1;
+        case '/':
+        case '.':
+            /*
+             * These types are not supported by QMP and thus are not
+             * handled here. Fall through.
+             */
+        default:
+            abort();
+        }
+    }
+
+    return 0;
+}
+
+/*
  * - Check if the client has passed all mandatory args
  * - Set special flags for argument validation
  */
@@ -4215,6 +4310,9 @@ out:
  * Client argument checking rules:
  *
  * 1. Client must provide all mandatory arguments
+ * 2. Each argument provided by the client must be expected
+ * 3. Each argument provided by the client must have the type expected
+ *    by the command
  */
 static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
 {
@@ -4229,7 +4327,7 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
         goto out;
     }
 
-    /* TODO: Check client args type */
+    err = check_client_args_type(client_args, cmd_args, flags);
 
 out:
     QDECREF(cmd_args);
-- 
1.7.1.359.gd0b8d

Re: [Qemu-devel] [PATCH 09/13] QMP: New argument checker (second part)

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This commit introduces the second (and last) part of QMP's new
> argument checker.
>
> The job is done by check_client_args_type(), it iterates over
> the client's argument qdict and for for each argument it checks
> if it exists and if its type is valid.
>
> It's important to observe the following changes from the existing
> argument checker:
>
>   - If the handler accepts an O-type argument, unknown arguments
>     are passed down to it. It's up to O-type handlers to validate
>     their arguments
>
>   - Boolean types (eg. 'b' and '-') don't accept integers anymore,
>     only json-bool
>
>   - Argument types '/' and '.' are currently unsupported under QMP,
>     thus they're not handled
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 99 insertions(+), 1 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b4fe5ba..8d074c2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4139,6 +4139,101 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
>  }
>  
>  /*
> + * Argument validation rules:
> + *
> + * 1. The argument must exist in cmd_args qdict
> + * 2. The argument type must be the expected one
> + *
> + * Special case: If the argument doesn't exist in cmd_args and
> + *               the QMP_CHECKER_OTYPE flag is set, then the
> + *               argument is considered an O-type one and the
> + *               checking is skipped for it.
> + */
> +static int check_client_args_type(const QDict *client_args,
> +                                  const QDict *cmd_args, int flags)
> +{
> +    const QDictEntry *ent;
> +
> +    for (ent = qdict_first(client_args); ent;ent = qdict_next(client_args,ent)){
> +        QObject *obj;
> +        QString *arg_type;
> +        const QObject *client_arg = qdict_entry_value(ent);
> +        const char *client_arg_name = qdict_entry_key(ent);
> +
> +        obj = qdict_get(cmd_args, client_arg_name);
> +        if (!obj) {
> +            if (flags & QMP_CHECKER_OTYPE) {
> +                /*
> +                 * This handler accepts O-type arguments, it's up to it to
> +                 * check for unknowns and validate its type.
> +                 */
> +                continue;
> +            }
> +            /* client arg doesn't exist */
> +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
> +            return -1;
> +        }
> +
> +        arg_type = qobject_to_qstring(obj);
> +        assert(arg_type != NULL);
> +
> +        /* check if argument's type is correct */
> +        switch (qstring_get_str(arg_type)[0]) {
> +        case 'F':
> +        case 'B':
> +        case 's':
> +            if (qobject_type(client_arg) != QTYPE_QSTRING) {
> +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> +                              "string");
> +                return -1;
> +            }
> +        break;
> +        case 'i':
> +        case 'l':
> +        case 'M':
> +            if (qobject_type(client_arg) != QTYPE_QINT) {
> +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> +                              "int");
> +                return -1; 
> +            }
> +            break;
> +        case 'f':
> +        case 'T':
> +            if (qobject_type(client_arg) != QTYPE_QINT &&
> +                qobject_type(client_arg) != QTYPE_QFLOAT) {
> +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> +                              "number");
> +               return -1; 
> +            }
> +            break;
> +        case 'b':
> +        case '-':
> +            if (qobject_type(client_arg) != QTYPE_QBOOL) {
> +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> +                              "bool");
> +               return -1; 
> +            }
> +            break;
> +        case 'O':
> +            /* XXX: this argument has the same name of the O-type defined in
> +                    in qemu-monitor.hx. This is not allowed, right? */


No, it's actually fine.

Consider device_add.  Its args_type is "device:O".  Nevertheless, it's
pefectly okay for a qdev to have a property named "device".

> +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
> +            return -1;

Instead:

               assert(flags & QMP_CHECKER_OTYPE);
               continue;

Not sure I like the name QMP_CHECKER_OTYPE.  The way it's used, it means
"in addition to checking declared arguments, accept undeclared arguments
without checking them (somebody else will check)".  'O-type' is merely
something that triggers that flag.  Happens to be the only way right
now.

QMP_CHECKER_ACCEPT_MORE_ARGS?

> +        case '/':
> +        case '.':
> +            /*
> +             * These types are not supported by QMP and thus are not
> +             * handled here. Fall through.
> +             */
> +        default:
> +            abort();
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/*
>   * - Check if the client has passed all mandatory args
>   * - Set special flags for argument validation
>   */
> @@ -4215,6 +4310,9 @@ out:
>   * Client argument checking rules:
>   *
>   * 1. Client must provide all mandatory arguments
> + * 2. Each argument provided by the client must be expected
> + * 3. Each argument provided by the client must have the type expected
> + *    by the command
>   */
>  static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>  {
> @@ -4229,7 +4327,7 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>          goto out;
>      }
>  
> -    /* TODO: Check client args type */
> +    err = check_client_args_type(client_args, cmd_args, flags);
>  
>  out:
>      QDECREF(cmd_args);

Re: [Qemu-devel] [PATCH 09/13] QMP: New argument checker (second part)

[Luiz Capitulino]

On Wed, 23 Jun 2010 17:21:12 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This commit introduces the second (and last) part of QMP's new
> > argument checker.
> >
> > The job is done by check_client_args_type(), it iterates over
> > the client's argument qdict and for for each argument it checks
> > if it exists and if its type is valid.
> >
> > It's important to observe the following changes from the existing
> > argument checker:
> >
> >   - If the handler accepts an O-type argument, unknown arguments
> >     are passed down to it. It's up to O-type handlers to validate
> >     their arguments
> >
> >   - Boolean types (eg. 'b' and '-') don't accept integers anymore,
> >     only json-bool
> >
> >   - Argument types '/' and '.' are currently unsupported under QMP,
> >     thus they're not handled
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 99 insertions(+), 1 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index b4fe5ba..8d074c2 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4139,6 +4139,101 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
> >  }
> >  
> >  /*
> > + * Argument validation rules:
> > + *
> > + * 1. The argument must exist in cmd_args qdict
> > + * 2. The argument type must be the expected one
> > + *
> > + * Special case: If the argument doesn't exist in cmd_args and
> > + *               the QMP_CHECKER_OTYPE flag is set, then the
> > + *               argument is considered an O-type one and the
> > + *               checking is skipped for it.
> > + */
> > +static int check_client_args_type(const QDict *client_args,
> > +                                  const QDict *cmd_args, int flags)
> > +{
> > +    const QDictEntry *ent;
> > +
> > +    for (ent = qdict_first(client_args); ent;ent = qdict_next(client_args,ent)){
> > +        QObject *obj;
> > +        QString *arg_type;
> > +        const QObject *client_arg = qdict_entry_value(ent);
> > +        const char *client_arg_name = qdict_entry_key(ent);
> > +
> > +        obj = qdict_get(cmd_args, client_arg_name);
> > +        if (!obj) {
> > +            if (flags & QMP_CHECKER_OTYPE) {
> > +                /*
> > +                 * This handler accepts O-type arguments, it's up to it to
> > +                 * check for unknowns and validate its type.
> > +                 */
> > +                continue;
> > +            }
> > +            /* client arg doesn't exist */
> > +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
> > +            return -1;
> > +        }
> > +
> > +        arg_type = qobject_to_qstring(obj);
> > +        assert(arg_type != NULL);
> > +
> > +        /* check if argument's type is correct */
> > +        switch (qstring_get_str(arg_type)[0]) {
> > +        case 'F':
> > +        case 'B':
> > +        case 's':
> > +            if (qobject_type(client_arg) != QTYPE_QSTRING) {
> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> > +                              "string");
> > +                return -1;
> > +            }
> > +        break;
> > +        case 'i':
> > +        case 'l':
> > +        case 'M':
> > +            if (qobject_type(client_arg) != QTYPE_QINT) {
> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> > +                              "int");
> > +                return -1; 
> > +            }
> > +            break;
> > +        case 'f':
> > +        case 'T':
> > +            if (qobject_type(client_arg) != QTYPE_QINT &&
> > +                qobject_type(client_arg) != QTYPE_QFLOAT) {
> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> > +                              "number");
> > +               return -1; 
> > +            }
> > +            break;
> > +        case 'b':
> > +        case '-':
> > +            if (qobject_type(client_arg) != QTYPE_QBOOL) {
> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
> > +                              "bool");
> > +               return -1; 
> > +            }
> > +            break;
> > +        case 'O':
> > +            /* XXX: this argument has the same name of the O-type defined in
> > +                    in qemu-monitor.hx. This is not allowed, right? */
> 
> 
> No, it's actually fine.
> 
> Consider device_add.  Its args_type is "device:O".  Nevertheless, it's
> pefectly okay for a qdev to have a property named "device".
> 
> > +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
> > +            return -1;
> 
> Instead:
> 
>                assert(flags & QMP_CHECKER_OTYPE);
>                continue;

Ok, but isn't it better to choose a name which is unlikely to be a property?
Maybe something like "device_add_opts_list:O"?

> Not sure I like the name QMP_CHECKER_OTYPE.  The way it's used, it means
> "in addition to checking declared arguments, accept undeclared arguments
> without checking them (somebody else will check)".  'O-type' is merely
> something that triggers that flag.  Happens to be the only way right
> now.
> 
> QMP_CHECKER_ACCEPT_MORE_ARGS?

Or QMP_CHECKER_ACCEPT_UNKNOWNS, but it's too long..

> 
> > +        case '/':
> > +        case '.':
> > +            /*
> > +             * These types are not supported by QMP and thus are not
> > +             * handled here. Fall through.
> > +             */
> > +        default:
> > +            abort();
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> >   * - Check if the client has passed all mandatory args
> >   * - Set special flags for argument validation
> >   */
> > @@ -4215,6 +4310,9 @@ out:
> >   * Client argument checking rules:
> >   *
> >   * 1. Client must provide all mandatory arguments
> > + * 2. Each argument provided by the client must be expected
> > + * 3. Each argument provided by the client must have the type expected
> > + *    by the command
> >   */
> >  static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >  {
> > @@ -4229,7 +4327,7 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >          goto out;
> >      }
> >  
> > -    /* TODO: Check client args type */
> > +    err = check_client_args_type(client_args, cmd_args, flags);
> >  
> >  out:
> >      QDECREF(cmd_args);
> 

Re: [Qemu-devel] [PATCH 09/13] QMP: New argument checker (second part)

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 23 Jun 2010 17:21:12 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > This commit introduces the second (and last) part of QMP's new
>> > argument checker.
>> >
>> > The job is done by check_client_args_type(), it iterates over
>> > the client's argument qdict and for for each argument it checks
>> > if it exists and if its type is valid.
>> >
>> > It's important to observe the following changes from the existing
>> > argument checker:
>> >
>> >   - If the handler accepts an O-type argument, unknown arguments
>> >     are passed down to it. It's up to O-type handlers to validate
>> >     their arguments
>> >
>> >   - Boolean types (eg. 'b' and '-') don't accept integers anymore,
>> >     only json-bool
>> >
>> >   - Argument types '/' and '.' are currently unsupported under QMP,
>> >     thus they're not handled
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  monitor.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 files changed, 99 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index b4fe5ba..8d074c2 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -4139,6 +4139,101 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
>> >  }
>> >  
>> >  /*
>> > + * Argument validation rules:
>> > + *
>> > + * 1. The argument must exist in cmd_args qdict
>> > + * 2. The argument type must be the expected one
>> > + *
>> > + * Special case: If the argument doesn't exist in cmd_args and
>> > + *               the QMP_CHECKER_OTYPE flag is set, then the
>> > + *               argument is considered an O-type one and the
>> > + *               checking is skipped for it.
>> > + */
>> > +static int check_client_args_type(const QDict *client_args,
>> > +                                  const QDict *cmd_args, int flags)
>> > +{
>> > +    const QDictEntry *ent;
>> > +
>> > +    for (ent = qdict_first(client_args); ent;ent = qdict_next(client_args,ent)){
>> > +        QObject *obj;
>> > +        QString *arg_type;
>> > +        const QObject *client_arg = qdict_entry_value(ent);
>> > +        const char *client_arg_name = qdict_entry_key(ent);
>> > +
>> > +        obj = qdict_get(cmd_args, client_arg_name);
>> > +        if (!obj) {
>> > +            if (flags & QMP_CHECKER_OTYPE) {
>> > +                /*
>> > +                 * This handler accepts O-type arguments, it's up to it to
>> > +                 * check for unknowns and validate its type.
>> > +                 */
>> > +                continue;
>> > +            }
>> > +            /* client arg doesn't exist */
>> > +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
>> > +            return -1;
>> > +        }
>> > +
>> > +        arg_type = qobject_to_qstring(obj);
>> > +        assert(arg_type != NULL);
>> > +
>> > +        /* check if argument's type is correct */
>> > +        switch (qstring_get_str(arg_type)[0]) {
>> > +        case 'F':
>> > +        case 'B':
>> > +        case 's':
>> > +            if (qobject_type(client_arg) != QTYPE_QSTRING) {
>> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
>> > +                              "string");
>> > +                return -1;
>> > +            }
>> > +        break;
>> > +        case 'i':
>> > +        case 'l':
>> > +        case 'M':
>> > +            if (qobject_type(client_arg) != QTYPE_QINT) {
>> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
>> > +                              "int");
>> > +                return -1; 
>> > +            }
>> > +            break;
>> > +        case 'f':
>> > +        case 'T':
>> > +            if (qobject_type(client_arg) != QTYPE_QINT &&
>> > +                qobject_type(client_arg) != QTYPE_QFLOAT) {
>> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
>> > +                              "number");
>> > +               return -1; 
>> > +            }
>> > +            break;
>> > +        case 'b':
>> > +        case '-':
>> > +            if (qobject_type(client_arg) != QTYPE_QBOOL) {
>> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
>> > +                              "bool");
>> > +               return -1; 
>> > +            }
>> > +            break;
>> > +        case 'O':
>> > +            /* XXX: this argument has the same name of the O-type defined in
>> > +                    in qemu-monitor.hx. This is not allowed, right? */
>> 
>> 
>> No, it's actually fine.
>> 
>> Consider device_add.  Its args_type is "device:O".  Nevertheless, it's
>> pefectly okay for a qdev to have a property named "device".
>> 
>> > +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
>> > +            return -1;
>> 
>> Instead:
>> 
>>                assert(flags & QMP_CHECKER_OTYPE);
>>                continue;
>
> Ok, but isn't it better to choose a name which is unlikely to be a property?
> Maybe something like "device_add_opts_list:O"?

No.  The "name" of the O-type argument is not really an argument name.
When I created the O-type, I needed the name of a QemuOptsList, and I
had no use for the argument name, so I chose to simply (ab)use the
argument name.  Saved me the trouble of adding even more syntax to
args_type.

Since it's not an argument name, the argument checker must ignore it.

>> Not sure I like the name QMP_CHECKER_OTYPE.  The way it's used, it means
>> "in addition to checking declared arguments, accept undeclared arguments
>> without checking them (somebody else will check)".  'O-type' is merely
>> something that triggers that flag.  Happens to be the only way right
>> now.
>> 
>> QMP_CHECKER_ACCEPT_MORE_ARGS?
>
> Or QMP_CHECKER_ACCEPT_UNKNOWNS, but it's too long..

If you want it shorter, you could leave out CHECKER and/or ACCEPT.  Or
you could say VARARGS instead of MORE_ARGS.

[...]

[Qemu-devel] [PATCH 10/13] QMP: Drop old client argument checker

[Luiz Capitulino]

Previous two commits added qmp_check_client_args(), which
fully replaces this code and is way better.

It's important to note that the new checker doesn't support
the '/' arg type. As we don't have any of those handlers
converted to QMP, this is just dead code.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |  176 -------------------------------------------------------------
 1 files changed, 0 insertions(+), 176 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8d074c2..2a29095 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3961,177 +3961,6 @@ static int monitor_can_read(void *opaque)
     return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
-typedef struct CmdArgs {
-    QString *name;
-    int type;
-    int flag;
-    int optional;
-} CmdArgs;
-
-static int check_opt(const CmdArgs *cmd_args, const char *name, QDict *args)
-{
-    if (!cmd_args->optional) {
-        qerror_report(QERR_MISSING_PARAMETER, name);
-        return -1;
-    }
-
-    return 0;
-}
-
-static int check_arg(const CmdArgs *cmd_args, QDict *args)
-{
-    QObject *value;
-    const char *name;
-
-    name = qstring_get_str(cmd_args->name);
-
-    if (!args) {
-        return check_opt(cmd_args, name, args);
-    }
-
-    value = qdict_get(args, name);
-    if (!value) {
-        return check_opt(cmd_args, name, args);
-    }
-
-    switch (cmd_args->type) {
-        case 'F':
-        case 'B':
-        case 's':
-            if (qobject_type(value) != QTYPE_QSTRING) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "string");
-                return -1;
-            }
-            break;
-        case '/': {
-            int i;
-            const char *keys[] = { "count", "format", "size", NULL };
-
-            for (i = 0; keys[i]; i++) {
-                QObject *obj = qdict_get(args, keys[i]);
-                if (!obj) {
-                    qerror_report(QERR_MISSING_PARAMETER, name);
-                    return -1;
-                }
-                if (qobject_type(obj) != QTYPE_QINT) {
-                    qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "int");
-                    return -1;
-                }
-            }
-            break;
-        }
-        case 'i':
-        case 'l':
-        case 'M':
-            if (qobject_type(value) != QTYPE_QINT) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "int");
-                return -1;
-            }
-            break;
-        case 'f':
-        case 'T':
-            if (qobject_type(value) != QTYPE_QINT && qobject_type(value) != QTYPE_QFLOAT) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "number");
-                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) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "bool");
-                return -1;
-            }
-            break;
-        case 'O':
-        default:
-            /* impossible */
-            abort();
-    }
-
-    return 0;
-}
-
-static void cmd_args_init(CmdArgs *cmd_args)
-{
-    cmd_args->name = qstring_new();
-    cmd_args->type = cmd_args->flag = cmd_args->optional = 0;
-}
-
-static int check_opts(QemuOptsList *opts_list, QDict *args)
-{
-    assert(!opts_list->desc->name);
-    return 0;
-}
-
-/*
- * This is not trivial, we have to parse Monitor command's argument
- * type syntax to be able to check the arguments provided by clients.
- *
- * In the near future we will be using an array for that and will be
- * able to drop all this parsing...
- */
-static int monitor_check_qmp_args(const mon_cmd_t *cmd, QDict *args)
-{
-    int err;
-    const char *p;
-    CmdArgs cmd_args;
-    QemuOptsList *opts_list;
-
-    if (cmd->args_type == NULL) {
-        return (qdict_size(args) == 0 ? 0 : -1);
-    }
-
-    err = 0;
-    cmd_args_init(&cmd_args);
-    opts_list = NULL;
-
-    for (p = cmd->args_type;; p++) {
-        if (*p == ':') {
-            cmd_args.type = *++p;
-            p++;
-            if (cmd_args.type == '-') {
-                cmd_args.flag = *p++;
-                cmd_args.optional = 1;
-            } else if (cmd_args.type == 'O') {
-                opts_list = qemu_find_opts(qstring_get_str(cmd_args.name));
-                assert(opts_list);
-            } else if (*p == '?') {
-                cmd_args.optional = 1;
-                p++;
-            }
-
-            assert(*p == ',' || *p == '\0');
-            if (opts_list) {
-                err = check_opts(opts_list, args);
-                opts_list = NULL;
-            } else {
-                err = check_arg(&cmd_args, args);
-                QDECREF(cmd_args.name);
-                cmd_args_init(&cmd_args);
-            }
-
-            if (err < 0) {
-                break;
-            }
-        } else {
-            qstring_append_chr(cmd_args.name, *p);
-        }
-
-        if (*p == '\0') {
-            break;
-        }
-    }
-
-    QDECREF(cmd_args.name);
-    return err;
-}
-
 static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
 {
     int is_cap = compare_cmd(cmd_name, "qmp_capabilities");
@@ -4414,11 +4243,6 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    err = monitor_check_qmp_args(cmd, args);
-    if (err < 0) {
-        goto err_out;
-    }
-
     if (monitor_handler_is_async(cmd)) {
         qmp_async_cmd_handler(mon, cmd, args);
     } else {
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 11/13] QError: Introduce QERR_QMP_UNEXPECTED_INPUT_OBJECT_MEMBER

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 44d0bf8..8a10557 100644
--- a/qerror.c
+++ b/qerror.c
@@ -177,6 +177,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "QMP input object member '%(member)' expects '%(expected)'",
     },
     {
+        .error_fmt = QERR_QMP_UNEXPECTED_INPUT_OBJECT_MEMBER,
+        .desc      = "QMP input object member '%(member)' is unexpected",
+    },
+    {
         .error_fmt = QERR_SET_PASSWD_FAILED,
         .desc      = "Could not set password",
     },
diff --git a/qerror.h b/qerror.h
index 77ae574..ef59430 100644
--- a/qerror.h
+++ b/qerror.h
@@ -148,6 +148,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_QMP_BAD_INPUT_OBJECT_MEMBER \
     "{ 'class': 'QMPBadInputObjectMember', 'data': { 'member': %s, 'expected': %s } }"
 
+#define QERR_QMP_UNEXPECTED_INPUT_OBJECT_MEMBER \
+    "{ 'class': 'QMPUnexpectedInputObjectMember', 'data': { 'member': %s } }"
+
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
-- 
1.7.1.359.gd0b8d

[Qemu-devel] [PATCH 12/13] QMP: Introduce qmp_check_input_obj()

[Luiz Capitulino]

This is similar to qmp_check_client_args(), but it checks if
the input object follows the specification (QMP/qmp-spec.txt
section 2.3).

As we're limited to three keys, the work here is quite simple:
we iterate over the input object, checking each time if the
current argument complies to the specification.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2a29095..a24a152 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4163,6 +4163,46 @@ out:
     return err;
 }
 
+/*
+ * Input object checking rules
+ *
+ * 1. "execute" key must exist (not checked here)
+ * 2. "execute" key must be a string
+ * 3. "arguments" key must be a dict
+ * 4. "id" key can be anything (ie. json-value)
+ * 5. Any argument not listed above is considered unexpected
+ */
+static int qmp_check_input_obj(const QDict *input_obj)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(input_obj); ent; ent = qdict_next(input_obj, ent)) {
+        const char *arg_name = qdict_entry_key(ent);
+        const QObject *arg_obj = qdict_entry_value(ent);
+
+        if (!strcmp(arg_name, "execute")) {
+            if (qobject_type(arg_obj) != QTYPE_QSTRING) {
+                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
+                              "string");
+                return -1;
+            }
+        } else if (!strcmp(arg_name, "arguments")) {
+            if (qobject_type(arg_obj) != QTYPE_QDICT) {
+                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
+                              "object");
+                return -1;
+            }
+        } else if (!strcmp(arg_name, "id")) {
+            /* FIXME: check duplicated IDs for async commands */
+        } else {
+            qerror_report(QERR_QMP_UNEXPECTED_INPUT_OBJECT_MEMBER, arg_name);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
@@ -4187,6 +4227,11 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 
     input = qobject_to_qdict(obj);
 
+    err = qmp_check_input_obj(input);
+    if (err < 0) {
+        goto err_input;
+    }
+
     mon->mc->id = qdict_get(input, "id");
     qobject_incref(mon->mc->id);
 
-- 
1.7.1.359.gd0b8d

Re: [Qemu-devel] [PATCH 12/13] QMP: Introduce qmp_check_input_obj()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This is similar to qmp_check_client_args(), but it checks if
> the input object follows the specification (QMP/qmp-spec.txt
> section 2.3).
>
> As we're limited to three keys, the work here is quite simple:
> we iterate over the input object, checking each time if the
> current argument complies to the specification.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 45 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 2a29095..a24a152 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4163,6 +4163,46 @@ out:
>      return err;
>  }
>  
> +/*
> + * Input object checking rules
> + *
> + * 1. "execute" key must exist (not checked here)

Is it checked elsewhere?  If yes, make it (checked elsewhere already)

> + * 2. "execute" key must be a string
> + * 3. "arguments" key must be a dict
> + * 4. "id" key can be anything (ie. json-value)
> + * 5. Any argument not listed above is considered unexpected
> + */
> +static int qmp_check_input_obj(const QDict *input_obj)
> +{
> +    const QDictEntry *ent;
> +
> +    for (ent = qdict_first(input_obj); ent; ent = qdict_next(input_obj, ent)) {
> +        const char *arg_name = qdict_entry_key(ent);
> +        const QObject *arg_obj = qdict_entry_value(ent);
> +
> +        if (!strcmp(arg_name, "execute")) {
> +            if (qobject_type(arg_obj) != QTYPE_QSTRING) {
> +                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
> +                              "string");
> +                return -1;
> +            }
> +        } else if (!strcmp(arg_name, "arguments")) {
> +            if (qobject_type(arg_obj) != QTYPE_QDICT) {
> +                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
> +                              "object");
> +                return -1;
> +            }
> +        } else if (!strcmp(arg_name, "id")) {
> +            /* FIXME: check duplicated IDs for async commands */
> +        } else {
> +            qerror_report(QERR_QMP_UNEXPECTED_INPUT_OBJECT_MEMBER, arg_name);

Identifier's a bit on the long side, isn't it?

> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  {
>      int err;
> @@ -4187,6 +4227,11 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  
>      input = qobject_to_qdict(obj);
>  
> +    err = qmp_check_input_obj(input);
> +    if (err < 0) {
> +        goto err_input;
> +    }
> +
>      mon->mc->id = qdict_get(input, "id");
>      qobject_incref(mon->mc->id);

Re: [Qemu-devel] [PATCH 12/13] QMP: Introduce qmp_check_input_obj()

[Luiz Capitulino]

On Wed, 23 Jun 2010 17:23:54 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This is similar to qmp_check_client_args(), but it checks if
> > the input object follows the specification (QMP/qmp-spec.txt
> > section 2.3).
> >
> > As we're limited to three keys, the work here is quite simple:
> > we iterate over the input object, checking each time if the
> > current argument complies to the specification.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 45 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 2a29095..a24a152 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4163,6 +4163,46 @@ out:
> >      return err;
> >  }
> >  
> > +/*
> > + * Input object checking rules
> > + *
> > + * 1. "execute" key must exist (not checked here)
> 
> Is it checked elsewhere?  If yes, make it (checked elsewhere already)

Yes, will do.

> > + * 2. "execute" key must be a string
> > + * 3. "arguments" key must be a dict
> > + * 4. "id" key can be anything (ie. json-value)
> > + * 5. Any argument not listed above is considered unexpected
> > + */
> > +static int qmp_check_input_obj(const QDict *input_obj)
> > +{
> > +    const QDictEntry *ent;
> > +
> > +    for (ent = qdict_first(input_obj); ent; ent = qdict_next(input_obj, ent)) {
> > +        const char *arg_name = qdict_entry_key(ent);
> > +        const QObject *arg_obj = qdict_entry_value(ent);
> > +
> > +        if (!strcmp(arg_name, "execute")) {
> > +            if (qobject_type(arg_obj) != QTYPE_QSTRING) {
> > +                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
> > +                              "string");
> > +                return -1;
> > +            }
> > +        } else if (!strcmp(arg_name, "arguments")) {
> > +            if (qobject_type(arg_obj) != QTYPE_QDICT) {
> > +                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
> > +                              "object");
> > +                return -1;
> > +            }
> > +        } else if (!strcmp(arg_name, "id")) {
> > +            /* FIXME: check duplicated IDs for async commands */
> > +        } else {
> > +            qerror_report(QERR_QMP_UNEXPECTED_INPUT_OBJECT_MEMBER, arg_name);
> 
> Identifier's a bit on the long side, isn't it?

Yes, suggestions?

> 
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
> >  {
> >      int err;
> > @@ -4187,6 +4227,11 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
> >  
> >      input = qobject_to_qdict(obj);
> >  
> > +    err = qmp_check_input_obj(input);
> > +    if (err < 0) {
> > +        goto err_input;
> > +    }
> > +
> >      mon->mc->id = qdict_get(input, "id");
> >      qobject_incref(mon->mc->id);
> 

Re: [Qemu-devel] [PATCH 12/13] QMP: Introduce qmp_check_input_obj()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 23 Jun 2010 17:23:54 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
[...]
>> > +            qerror_report(QERR_QMP_UNEXPECTED_INPUT_OBJECT_MEMBER, arg_name);
>> 
>> Identifier's a bit on the long side, isn't it?
>
> Yes, suggestions?

What about omitting INPUT_OBJECT or at least OBJECT?

EXTRA instead of UNEXPECTED?

We could have

QERR_QMP_EXTRA_MEMBER
QERR_QMP_MISSING_MEMBER         now: QERR_QMP_BAD_INPUT_OBJECT (I think)      
QERR_QMP_BAD_MEMBER             now: QERR_QMP_BAD_INPUT_OBJECT_MEMBER

[...]

[Qemu-devel] [PATCH 13/13] QMP: Drop old input object checking

[Luiz Capitulino]

Previous commit added qmp_check_input_obj(), it does all the
checking we need.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index a24a152..c558cc7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4239,9 +4239,6 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     if (!obj) {
         qerror_report(QERR_QMP_BAD_INPUT_OBJECT, "execute");
         goto err_input;
-    } else if (qobject_type(obj) != QTYPE_QSTRING) {
-        qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute", "string");
-        goto err_input;
     }
 
     cmd_name = qstring_get_str(qobject_to_qstring(obj));
@@ -4273,9 +4270,6 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     obj = qdict_get(input, "arguments");
     if (!obj) {
         args = qdict_new();
-    } else if (qobject_type(obj) != QTYPE_QDICT) {
-        qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments", "object");
-        goto err_input;
     } else {
         args = qobject_to_qdict(obj);
         QINCREF(args);
-- 
1.7.1.359.gd0b8d