[Qemu-devel] [PATCH 0/9]: QMP: Replace client argument checker

[Qemu-devel] [PATCH 0/9]: QMP: Replace client argument checker

[Luiz Capitulino]

Current QMP's client argument checker implementation 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.

[Qemu-devel] [PATCH 1/9] 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 175bc17..ca3c3b1 100644
--- a/qdict.c
+++ b/qdict.c
@@ -287,6 +287,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
+ * 'err_value' will be returned.
+ */
+int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value)
+{
+    QObject *obj;
+
+    obj = qdict_get(qdict, key);
+    if (!obj || qobject_type(obj) != QTYPE_QBOOL)
+        return err_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 5e5902c..5cca816 100644
--- a/qdict.h
+++ b/qdict.h
@@ -57,6 +57,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 err_value);
+int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value);
 const char *qdict_get_try_str(const QDict *qdict, const char *key);
 
 #endif /* QDICT_H */
-- 
1.7.1.231.gd0b16

[Qemu-devel] [PATCH 2/9] 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 the code that checks QMP
client's arguments.

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: keep requiring 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 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>
---
 migration.c |   16 +++++++---------
 monitor.c   |   19 ++++---------------
 2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/migration.c b/migration.c
index 706fe55..efecbdc 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 15b53b9..bc3cc18 100644
--- a/monitor.c
+++ b/monitor.c
@@ -971,7 +971,7 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
 static 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);
@@ -3684,7 +3684,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++;
@@ -3692,7 +3692,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) {
@@ -3708,11 +3707,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:
@@ -4097,11 +4096,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;
 }
 
@@ -4174,11 +4168,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.231.gd0b16

[Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Luiz Capitulino]

This commit introduces the first half of qmp_check_client_args(),
which is the new client argument checker.

It's introduced on top of the existing code, so that there are
no regressions during the transition.

It works this way: the command's args_type field (from
qemu-monitor.hx) is transformed into a qdict. Then we iterate
over it checking whether each mandatory argument has been
provided by the client.

All comunication between the iterator and its caller is done
via the new QMPArgCheckRes type.

Next commit adds the second half of this work: type checking
and invalid argument detection.

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

diff --git a/monitor.c b/monitor.c
index bc3cc18..47a0da8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4259,6 +4259,108 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
     return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
 }
 
+typedef struct QMPArgCheckRes {
+    int result;
+    int skip;
+    QDict *qdict;
+} QMPArgCheckRes;
+
+/*
+ * Check if client passed all mandatory args
+ */
+static void check_mandatory_args(const char *cmd_arg_name,
+                                 QObject *obj, void *opaque)
+{
+    QString *type;
+    QMPArgCheckRes *res = opaque;
+
+    if (res->result < 0) {
+        /* report only the first error */
+        return;
+    }
+
+    type = qobject_to_qstring(obj);
+    assert(type != NULL);
+
+    if (qstring_get_str(type)[0] == 'O') {
+        QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
+        assert(opts_list);
+        res->result = check_opts(opts_list, res->qdict);
+        res->skip = 1;
+    } else if (qstring_get_str(type)[0] != '-' &&
+               qstring_get_str(type)[1] != '?' &&
+               !qdict_haskey(res->qdict, cmd_arg_name)) {
+        res->result = -1;
+        qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name);
+    }
+}
+
+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)
+{
+    QDict *cmd_args;
+    QMPArgCheckRes res = { .result = 0, .skip = 0 };
+
+    cmd_args = qdict_from_args_type(cmd->args_type);
+
+    res.qdict = client_args;
+    qdict_iter(cmd_args, check_mandatory_args, &res);
+
+    /* TODO: Check client args type */
+
+    QDECREF(cmd_args);
+    return res.result;
+}
+
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
@@ -4334,6 +4436,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.231.gd0b16

[Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code

[Luiz Capitulino]

This commit introduces check_client_args_type(), which is
called by qmp_check_client_args() and complements the
previous commit.

Now the new client's argument checker code is capable of
doing type checking and detecting unknown arguments.

It works this way: we iterate over the client's arguments
qdict and for each argument we check if it exists and if
its type is correct.

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

diff --git a/monitor.c b/monitor.c
index 47a0da8..14790e6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes {
 } QMPArgCheckRes;
 
 /*
+ * Check if client's argument exists and type is correct
+ */
+static void check_client_args_type(const char *client_arg_name,
+                                   QObject *client_arg, void *opaque)
+{
+    QObject *obj;
+    QString *arg_type;
+    QMPArgCheckRes *res = opaque;
+
+    if (res->result < 0) {
+        /* report only the first error */
+        return;
+    }
+
+    obj = qdict_get(res->qdict, client_arg_name);
+    if (!obj) {
+        /* client arg doesn't exist */
+        res->result = -1;
+        qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
+        return;
+    }
+
+    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");
+            res->result = -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");
+            res->result = -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");
+            res->result = -1;
+        }
+        break;
+    case 'b':
+    case '-':
+        if (qobject_type(client_arg) != QTYPE_QBOOL) {
+            qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool");
+            res->result = -1;
+        }
+        break;
+    case 'O':
+        /* Not checked here */
+        break;
+    default:
+        abort();
+    }
+}
+
+/*
  * Check if client passed all mandatory args
  */
 static void check_mandatory_args(const char *cmd_arg_name,
@@ -4344,6 +4413,9 @@ out:
  * Client argument checking rules:
  *
  * 1. Client must provide all mandatory arguments
+ * 2. Each argument provided by the client must be valid
+ * 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)
 {
@@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
     res.qdict = client_args;
     qdict_iter(cmd_args, check_mandatory_args, &res);
 
-    /* TODO: Check client args type */
+    if (!res.result && !res.skip) {
+        res.qdict = cmd_args;
+        qdict_iter(client_args, check_client_args_type, &res);
+    }
 
     QDECREF(cmd_args);
     return res.result;
-- 
1.7.1.231.gd0b16

[Qemu-devel] [PATCH 5/9] 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 |  170 -------------------------------------------------------------
 1 files changed, 0 insertions(+), 170 deletions(-)

diff --git a/monitor.c b/monitor.c
index 14790e6..d2a510e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4082,177 +4082,12 @@ 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");
@@ -4516,11 +4351,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.231.gd0b16

[Qemu-devel] [PATCH 6/9] QMP: check_opts(): Minor cleanup

[Luiz Capitulino]

We couldn't do it before, otherwise we would break the intention
of the previous checker, which was to ensure that opts_list wasn't
a NULL before checking it.

Debug code, pretty minor, still I decided to maintain its original
behavior.

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

diff --git a/monitor.c b/monitor.c
index d2a510e..1875731 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4084,6 +4084,7 @@ static int monitor_can_read(void *opaque)
 
 static int check_opts(QemuOptsList *opts_list, QDict *args)
 {
+    assert(opts_list);
     assert(!opts_list->desc->name);
     return 0;
 }
@@ -4188,7 +4189,6 @@ static void check_mandatory_args(const char *cmd_arg_name,
 
     if (qstring_get_str(type)[0] == 'O') {
         QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
-        assert(opts_list);
         res->result = check_opts(opts_list, res->qdict);
         res->skip = 1;
     } else if (qstring_get_str(type)[0] != '-' &&
-- 
1.7.1.231.gd0b16

[Qemu-devel] [PATCH 7/9] QError: Introduce QERR_QMP_BAD_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..b26224e 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_INVALID_INPUT_OBJECT_MEMBER,
+        .desc      = "QMP input object member '%(member)' is invalid",
+    },
+    {
         .error_fmt = QERR_SET_PASSWD_FAILED,
         .desc      = "Could not set password",
     },
diff --git a/qerror.h b/qerror.h
index 77ae574..eec9949 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_INVALID_INPUT_OBJECT_MEMBER \
+    "{ 'class': 'QMPInvalidInputObjectMember', 'data': { 'member': %s } }"
+
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
-- 
1.7.1.231.gd0b16

[Qemu-devel] [PATCH 8/9] QMP: Introduce qmp_check_input_obj()

[Luiz Capitulino]

This is similar to qmp_check_client_args(), but 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, each time checking if the
given 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 1875731..654b193 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4271,6 +4271,45 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
     return res.result;
 }
 
+/*
+ * 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 invalid
+ */
+static void qmp_check_input_obj(const char *input_obj_arg_name,
+                                QObject *input_obj_arg, void *opaque)
+{
+    int *err = opaque;
+
+    if (*err < 0) {
+        /* report only the first error */
+        return;
+    }
+
+    if (!strcmp(input_obj_arg_name, "execute")) {
+        if (qobject_type(input_obj_arg) != QTYPE_QSTRING) {
+            qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
+                          "string");
+            *err = -1;
+        }
+    } else if (!strcmp(input_obj_arg_name, "arguments")) {
+        if (qobject_type(input_obj_arg) != QTYPE_QDICT) {
+            qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
+                          "object");
+            *err = -1;
+        }
+    } else if (!strcmp(input_obj_arg_name, "id")) {
+        /* nothing to do */
+    } else {
+        qerror_report(QERR_QMP_INVALID_INPUT_OBJECT_MEMBER, input_obj_arg_name);
+        *err = -1;
+    }
+}
+
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
@@ -4295,6 +4334,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 
     input = qobject_to_qdict(obj);
 
+    err = 0;
+    qdict_iter(input, qmp_check_input_obj, &err);
+    if (err < 0) {
+        goto err_out;
+    }
+
     mon->mc->id = qdict_get(input, "id");
     qobject_incref(mon->mc->id);
 
-- 
1.7.1.231.gd0b16

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

[Luiz Capitulino]

Previous commit added qmp_check_input_obj(), it does this
checking for us.

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 654b193..f849456 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4347,9 +4347,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));
@@ -4381,9 +4378,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.231.gd0b16

Re: [Qemu-devel] [PATCH 1/9] QDict: Introduce qdict_get_try_bool()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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 175bc17..ca3c3b1 100644
> --- a/qdict.c
> +++ b/qdict.c
> @@ -287,6 +287,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
> + * 'err_value' will be returned.
> + */
> +int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value)
> +{
> +    QObject *obj;
> +
> +    obj = qdict_get(qdict, key);
> +    if (!obj || qobject_type(obj) != QTYPE_QBOOL)
> +        return err_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 5e5902c..5cca816 100644
> --- a/qdict.h
> +++ b/qdict.h
> @@ -57,6 +57,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 err_value);
> +int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value);
>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>  
>  #endif /* QDICT_H */

Nitpick: there's precedence for parameter name "err_value" in qdict.c,
but it's a misleading name all the same.  The parameter is a default
value.  Missing key is *not* an error.

Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This commit introduces the first half of qmp_check_client_args(),
> which is the new client argument checker.
>
> It's introduced on top of the existing code, so that there are
> no regressions during the transition.
>
> It works this way: the command's args_type field (from
> qemu-monitor.hx) is transformed into a qdict. Then we iterate
> over it checking whether each mandatory argument has been
> provided by the client.
>
> All comunication between the iterator and its caller is done
> via the new QMPArgCheckRes type.
>
> Next commit adds the second half of this work: type checking
> and invalid argument detection.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 107 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index bc3cc18..47a0da8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4259,6 +4259,108 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
>      return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
>  }
>  
> +typedef struct QMPArgCheckRes {
> +    int result;
> +    int skip;

skip is write-only in this patch.

> +    QDict *qdict;
> +} QMPArgCheckRes;
> +
> +/*
> + * Check if client passed all mandatory args
> + */
> +static void check_mandatory_args(const char *cmd_arg_name,
> +                                 QObject *obj, void *opaque)
> +{
> +    QString *type;
> +    QMPArgCheckRes *res = opaque;
> +
> +    if (res->result < 0) {
> +        /* report only the first error */
> +        return;
> +    }

This is a sign that the iterator needs a way to break the loop.

> +
> +    type = qobject_to_qstring(obj);
> +    assert(type != NULL);
> +
> +    if (qstring_get_str(type)[0] == 'O') {
> +        QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
> +        assert(opts_list);
> +        res->result = check_opts(opts_list, res->qdict);

I doubt this is the right place for calling check_opts.

Right now, it's only implemented for QemuOptsList with empty desc.  A
more complete version is in my tree (blockdev needs it).  Looks like
this:

static int check_opts(QemuOptsList *opts_list, QDict *args)
{
    QemuOptDesc *desc;
    CmdArgs cmd_args;

    for (desc = opts_list->desc; desc->name; desc++) {
        cmd_args_init(&cmd_args);
        cmd_args.optional = 1;
        switch (desc->type) {
        case QEMU_OPT_STRING:
            cmd_args.type = 's';
            break;
        case QEMU_OPT_BOOL:
            cmd_args.type = '-';
            break;
        case QEMU_OPT_NUMBER:
        case QEMU_OPT_SIZE:
            cmd_args.type = 'l';
            break;
        }
        qstring_append(cmd_args.name, desc->name);
        if (check_arg(&cmd_args, args) < 0) {
            QDECREF(cmd_args.name);
            return -1;
        }
        QDECREF(cmd_args.name);
    }
    return 0;
}

> +        res->skip = 1;
> +    } else if (qstring_get_str(type)[0] != '-' &&
> +               qstring_get_str(type)[1] != '?' &&
> +               !qdict_haskey(res->qdict, cmd_arg_name)) {
> +        res->result = -1;
> +        qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name);
> +    }
> +}
[...]

Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Markus Armbruster]

There's more...

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This commit introduces the first half of qmp_check_client_args(),
> which is the new client argument checker.
>
> It's introduced on top of the existing code, so that there are
> no regressions during the transition.
>
> It works this way: the command's args_type field (from
> qemu-monitor.hx) is transformed into a qdict. Then we iterate
> over it checking whether each mandatory argument has been
> provided by the client.
>
> All comunication between the iterator and its caller is done
> via the new QMPArgCheckRes type.
>
> Next commit adds the second half of this work: type checking
> and invalid argument detection.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 107 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index bc3cc18..47a0da8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4259,6 +4259,108 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
>      return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
>  }
>  
> +typedef struct QMPArgCheckRes {
> +    int result;
> +    int skip;
> +    QDict *qdict;
> +} QMPArgCheckRes;
> +
> +/*
> + * Check if client passed all mandatory args
> + */
> +static void check_mandatory_args(const char *cmd_arg_name,
> +                                 QObject *obj, void *opaque)
> +{
> +    QString *type;
> +    QMPArgCheckRes *res = opaque;
> +
> +    if (res->result < 0) {
> +        /* report only the first error */
> +        return;
> +    }
> +
> +    type = qobject_to_qstring(obj);
> +    assert(type != NULL);
> +
> +    if (qstring_get_str(type)[0] == 'O') {
> +        QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
> +        assert(opts_list);
> +        res->result = check_opts(opts_list, res->qdict);
> +        res->skip = 1;
> +    } else if (qstring_get_str(type)[0] != '-' &&
> +               qstring_get_str(type)[1] != '?' &&
> +               !qdict_haskey(res->qdict, cmd_arg_name)) {
> +        res->result = -1;

This is a sign that the iterator needs a way to return a value.

Check out qemu_opts_foreach(), it can break and return a value.

> +        qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name);
> +    }
> +}
> +
> +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)
> +{
> +    QDict *cmd_args;
> +    QMPArgCheckRes res = { .result = 0, .skip = 0 };
> +
> +    cmd_args = qdict_from_args_type(cmd->args_type);
> +
> +    res.qdict = client_args;
> +    qdict_iter(cmd_args, check_mandatory_args, &res);
> +
> +    /* TODO: Check client args type */
> +
> +    QDECREF(cmd_args);
> +    return res.result;
> +}

Higher order functions rock.  But C is too static and limited for
elegant use of higher order functions.  Means to construct loops are
usually more convenient to use, and yield more readable code.

I find the use of qdict_iter() here quite tortuous: you define a
separate iterator function, which you can't put next to its use.  You
need to jump back and forth between the two places to understand what
the loop does.  You define a special data structure just to pass
arguments and results through qdict_iter().

Let me try to sketch the alternative:

static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
{
    QDict *cmd_args;
    int res = 0, skip = 0;
    QDictEntry *ent;

    cmd_args = qdict_from_args_type(cmd->args_type);

    for (ent = qdict_first_entry(cmd_args); ent; ent = qdict_next_entry(ent) {
        type = qobject_to_qstring(ent->value);
        assert(type != NULL);
    
        if (qstring_get_str(type)[0] == 'O') {
            QemuOptsList *opts_list = qemu_find_opts(ent->key);
            assert(opts_list);
            res = check_opts(opts_list, cmd_args);
            skip = 1;
        } else if (qstring_get_str(type)[0] != '-' &&
                   qstring_get_str(type)[1] != '?' &&
                   !qdict_haskey(cmd_args, ent->key)) {
            qerror_report(QERR_MISSING_PARAMETER, ent->key);
            res = -1;
            break;
        }
    
    }

    /* TODO: Check client args type */

    QDECREF(cmd_args);
    return res;
}

> +
>  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  {
>      int err;
> @@ -4334,6 +4436,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;

Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This commit introduces check_client_args_type(), which is
> called by qmp_check_client_args() and complements the
> previous commit.
>
> Now the new client's argument checker code is capable of
> doing type checking and detecting unknown arguments.
>
> It works this way: we iterate over the client's arguments
> qdict and for each argument we check if it exists and if
> its type is correct.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 76 insertions(+), 1 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 47a0da8..14790e6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes {
>  } QMPArgCheckRes;
>  
>  /*
> + * Check if client's argument exists and type is correct
> + */
> +static void check_client_args_type(const char *client_arg_name,
> +                                   QObject *client_arg, void *opaque)
> +{
> +    QObject *obj;
> +    QString *arg_type;
> +    QMPArgCheckRes *res = opaque;
> +
> +    if (res->result < 0) {
> +        /* report only the first error */
> +        return;
> +    }
> +
> +    obj = qdict_get(res->qdict, client_arg_name);
> +    if (!obj) {
> +        /* client arg doesn't exist */
> +        res->result = -1;
> +        qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
> +        return;
> +    }
> +
> +    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");
> +            res->result = -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");
> +            res->result = -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");
> +            res->result = -1;
> +        }
> +        break;
> +    case 'b':
> +    case '-':
> +        if (qobject_type(client_arg) != QTYPE_QBOOL) {
> +            qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool");
> +            res->result = -1;
> +        }
> +        break;
> +    case 'O':
> +        /* Not checked here */
> +        break;

What about case '/'?  I guess it doesn't make much sense for QMP, but
the old checker handles it.  If we drop it from QMP, we should document
the restriction in the source.

> +    default:
> +        abort();
> +    }
> +}
> +
> +/*
>   * Check if client passed all mandatory args
>   */
>  static void check_mandatory_args(const char *cmd_arg_name,
> @@ -4344,6 +4413,9 @@ out:
>   * Client argument checking rules:
>   *
>   * 1. Client must provide all mandatory arguments
> + * 2. Each argument provided by the client must be valid
> + * 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)
>  {
> @@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>      res.qdict = client_args;
>      qdict_iter(cmd_args, check_mandatory_args, &res);
>  
> -    /* TODO: Check client args type */
> +    if (!res.result && !res.skip) {
> +        res.qdict = cmd_args;
> +        qdict_iter(client_args, check_client_args_type, &res);
> +    }

What if we have both an O-type argument and other arguments?  Then the
'O' makes check_client_args_type() set res.skip, and we duly skip
checking the other arguments here.

Again, the iterator makes for tortuous code.

>  
>      QDECREF(cmd_args);
>      return res.result;

Re: [Qemu-devel] [PATCH 7/9] QError: Introduce QERR_QMP_BAD_INPUT_OBJECT_MEMBER

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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..b26224e 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_INVALID_INPUT_OBJECT_MEMBER,
> +        .desc      = "QMP input object member '%(member)' is invalid",
> +    },
> +    {

"is invalid" can mean pretty much anything.  Let's call it "is
unexpected".  Update item 2. in qmp_check_client_args()'s function
comment, too.

>          .error_fmt = QERR_SET_PASSWD_FAILED,
>          .desc      = "Could not set password",
>      },
> diff --git a/qerror.h b/qerror.h
> index 77ae574..eec9949 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_INVALID_INPUT_OBJECT_MEMBER \
> +    "{ 'class': 'QMPInvalidInputObjectMember', 'data': { 'member': %s } }"
> +
>  #define QERR_SET_PASSWD_FAILED \
>      "{ 'class': 'SetPasswdFailed', 'data': {} }"

Re: [Qemu-devel] [PATCH 8/9] QMP: Introduce qmp_check_input_obj()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This is similar to qmp_check_client_args(), but 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, each time checking if the
> given 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 1875731..654b193 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4271,6 +4271,45 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>      return res.result;
>  }
>  
> +/*
> + * 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)

Really?  Checking qmp-spec.txt... yes, really.  Is it a good idea to
permit objects and arrays?

> + * 5. Any argument not listed above is invalid
> + */
> +static void qmp_check_input_obj(const char *input_obj_arg_name,
> +                                QObject *input_obj_arg, void *opaque)
> +{
> +    int *err = opaque;
> +
> +    if (*err < 0) {
> +        /* report only the first error */
> +        return;
> +    }
> +
> +    if (!strcmp(input_obj_arg_name, "execute")) {
> +        if (qobject_type(input_obj_arg) != QTYPE_QSTRING) {
> +            qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
> +                          "string");
> +            *err = -1;
> +        }
> +    } else if (!strcmp(input_obj_arg_name, "arguments")) {
> +        if (qobject_type(input_obj_arg) != QTYPE_QDICT) {
> +            qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
> +                          "object");
> +            *err = -1;
> +        }
> +    } else if (!strcmp(input_obj_arg_name, "id")) {
> +        /* nothing to do */
> +    } else {
> +        qerror_report(QERR_QMP_INVALID_INPUT_OBJECT_MEMBER, input_obj_arg_name);
> +        *err = -1;
> +    }
> +}
> +
>  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  {
>      int err;
> @@ -4295,6 +4334,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  
>      input = qobject_to_qdict(obj);
>  
> +    err = 0;
> +    qdict_iter(input, qmp_check_input_obj, &err);
> +    if (err < 0) {
> +        goto err_out;
> +    }
> +
>      mon->mc->id = qdict_get(input, "id");
>      qobject_incref(mon->mc->id);

More contortions caused by iterator use...

Re: [Qemu-devel] [PATCH 0/9]: QMP: Replace client argument checker

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Current QMP's client argument checker implementation 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.

Good work!

Re: [Qemu-devel] [PATCH 1/9] QDict: Introduce qdict_get_try_bool()

[Luiz Capitulino]

On Wed, 02 Jun 2010 08:35:16 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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 175bc17..ca3c3b1 100644
> > --- a/qdict.c
> > +++ b/qdict.c
> > @@ -287,6 +287,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
> > + * 'err_value' will be returned.
> > + */
> > +int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value)
> > +{
> > +    QObject *obj;
> > +
> > +    obj = qdict_get(qdict, key);
> > +    if (!obj || qobject_type(obj) != QTYPE_QBOOL)
> > +        return err_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 5e5902c..5cca816 100644
> > --- a/qdict.h
> > +++ b/qdict.h
> > @@ -57,6 +57,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 err_value);
> > +int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value);
> >  const char *qdict_get_try_str(const QDict *qdict, const char *key);
> >  
> >  #endif /* QDICT_H */
> 
> Nitpick: there's precedence for parameter name "err_value" in qdict.c,
> but it's a misleading name all the same.  The parameter is a default
> value.  Missing key is *not* an error.

 Good point.

Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Luiz Capitulino]

On Wed, 02 Jun 2010 08:59:11 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:

[...]

> > +
> > +    type = qobject_to_qstring(obj);
> > +    assert(type != NULL);
> > +
> > +    if (qstring_get_str(type)[0] == 'O') {
> > +        QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
> > +        assert(opts_list);
> > +        res->result = check_opts(opts_list, res->qdict);
> 
> I doubt this is the right place for calling check_opts.

 Can you suggest the right place?

Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Luiz Capitulino]

On Wed, 02 Jun 2010 09:22:40 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> There's more...

 Good!

> Luiz Capitulino <lcapitulino@redhat.com> writes:

[...]

> > +static void check_mandatory_args(const char *cmd_arg_name,
> > +                                 QObject *obj, void *opaque)
> > +{
> > +    QString *type;
> > +    QMPArgCheckRes *res = opaque;
> > +
> > +    if (res->result < 0) {
> > +        /* report only the first error */
> > +        return;
> > +    }
> > +
> > +    type = qobject_to_qstring(obj);
> > +    assert(type != NULL);
> > +
> > +    if (qstring_get_str(type)[0] == 'O') {
> > +        QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
> > +        assert(opts_list);
> > +        res->result = check_opts(opts_list, res->qdict);
> > +        res->skip = 1;
> > +    } else if (qstring_get_str(type)[0] != '-' &&
> > +               qstring_get_str(type)[1] != '?' &&
> > +               !qdict_haskey(res->qdict, cmd_arg_name)) {
> > +        res->result = -1;
> 
> This is a sign that the iterator needs a way to return a value.
> 
> Check out qemu_opts_foreach(), it can break and return a value.

 Ah, that's good, I was wondering how I could do that but couldn't
find a good way.

[...]

> Higher order functions rock.  But C is too static and limited for
> elegant use of higher order functions.  Means to construct loops are
> usually more convenient to use, and yield more readable code.
> 
> I find the use of qdict_iter() here quite tortuous: you define a
> separate iterator function, which you can't put next to its use.  You
> need to jump back and forth between the two places to understand what
> the loop does.  You define a special data structure just to pass
> arguments and results through qdict_iter().
> 
> Let me try to sketch the alternative:
> 
> static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> {
>     QDict *cmd_args;
>     int res = 0, skip = 0;
>     QDictEntry *ent;
> 
>     cmd_args = qdict_from_args_type(cmd->args_type);
> 
>     for (ent = qdict_first_entry(cmd_args); ent; ent = qdict_next_entry(ent) {

 I thought about doing something similar a while ago, but I dislike it for
two reasons:

  1. I don't think the notion of 'first' and 'next' apply for dicts. One may
     argue that the iterator has the same issue, but it's implicit

  2. QDictEntry shouldn't be part of the public interface, we should be
     using forward declaration there (although I'm not sure whether this is
     possible with a typedef)

 I think having qdict_foreach() will improve things already.

Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code

[Luiz Capitulino]

On Wed, 02 Jun 2010 09:31:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This commit introduces check_client_args_type(), which is
> > called by qmp_check_client_args() and complements the
> > previous commit.
> >
> > Now the new client's argument checker code is capable of
> > doing type checking and detecting unknown arguments.
> >
> > It works this way: we iterate over the client's arguments
> > qdict and for each argument we check if it exists and if
> > its type is correct.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 76 insertions(+), 1 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 47a0da8..14790e6 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes {
> >  } QMPArgCheckRes;
> >  
> >  /*
> > + * Check if client's argument exists and type is correct
> > + */
> > +static void check_client_args_type(const char *client_arg_name,
> > +                                   QObject *client_arg, void *opaque)
> > +{
> > +    QObject *obj;
> > +    QString *arg_type;
> > +    QMPArgCheckRes *res = opaque;
> > +
> > +    if (res->result < 0) {
> > +        /* report only the first error */
> > +        return;
> > +    }
> > +
> > +    obj = qdict_get(res->qdict, client_arg_name);
> > +    if (!obj) {
> > +        /* client arg doesn't exist */
> > +        res->result = -1;
> > +        qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
> > +        return;
> > +    }
> > +
> > +    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");
> > +            res->result = -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");
> > +            res->result = -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");
> > +            res->result = -1;
> > +        }
> > +        break;
> > +    case 'b':
> > +    case '-':
> > +        if (qobject_type(client_arg) != QTYPE_QBOOL) {
> > +            qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool");
> > +            res->result = -1;
> > +        }
> > +        break;
> > +    case 'O':
> > +        /* Not checked here */
> > +        break;
> 
> What about case '/'?  I guess it doesn't make much sense for QMP, but
> the old checker handles it.  If we drop it from QMP, we should document
> the restriction in the source.

 Yes, there're two args_type we don't handle in QMP because we don't have
any of those handlers converted: '/' and '.'.

 I think it's unlikely to get them converted in this form, so the current
implementation contains dead-code. I explained this in one commit log, but
maybe it's the wrong place.

> 
> > +    default:
> > +        abort();
> > +    }
> > +}
> > +
> > +/*
> >   * Check if client passed all mandatory args
> >   */
> >  static void check_mandatory_args(const char *cmd_arg_name,
> > @@ -4344,6 +4413,9 @@ out:
> >   * Client argument checking rules:
> >   *
> >   * 1. Client must provide all mandatory arguments
> > + * 2. Each argument provided by the client must be valid
> > + * 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)
> >  {
> > @@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >      res.qdict = client_args;
> >      qdict_iter(cmd_args, check_mandatory_args, &res);
> >  
> > -    /* TODO: Check client args type */
> > +    if (!res.result && !res.skip) {
> > +        res.qdict = cmd_args;
> > +        qdict_iter(client_args, check_client_args_type, &res);
> > +    }
> 
> What if we have both an O-type argument and other arguments?  Then the
> 'O' makes check_client_args_type() set res.skip, and we duly skip
> checking the other arguments here.

 Oh, good catch. I thought that that was what the current code does,
but looks like I misread it.

> Again, the iterator makes for tortuous code.
> 
> >  
> >      QDECREF(cmd_args);
> >      return res.result;
> 

Re: [Qemu-devel] [PATCH 8/9] QMP: Introduce qmp_check_input_obj()

[Luiz Capitulino]

On Wed, 02 Jun 2010 09:39:26 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This is similar to qmp_check_client_args(), but 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, each time checking if the
> > given 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 1875731..654b193 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4271,6 +4271,45 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >      return res.result;
> >  }
> >  
> > +/*
> > + * 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)
> 
> Really?  Checking qmp-spec.txt... yes, really.  Is it a good idea to
> permit objects and arrays?

 It was Avi's suggestion to allow anything, maybe arrays don't make sense
but objects do.

Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 02 Jun 2010 09:31:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > This commit introduces check_client_args_type(), which is
>> > called by qmp_check_client_args() and complements the
>> > previous commit.
>> >
>> > Now the new client's argument checker code is capable of
>> > doing type checking and detecting unknown arguments.
>> >
>> > It works this way: we iterate over the client's arguments
>> > qdict and for each argument we check if it exists and if
>> > its type is correct.
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  monitor.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 files changed, 76 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index 47a0da8..14790e6 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes {
>> >  } QMPArgCheckRes;
>> >  
>> >  /*
>> > + * Check if client's argument exists and type is correct
>> > + */
>> > +static void check_client_args_type(const char *client_arg_name,
>> > +                                   QObject *client_arg, void *opaque)
>> > +{
>> > +    QObject *obj;
>> > +    QString *arg_type;
>> > +    QMPArgCheckRes *res = opaque;
>> > +
>> > +    if (res->result < 0) {
>> > +        /* report only the first error */
>> > +        return;
>> > +    }
>> > +
>> > +    obj = qdict_get(res->qdict, client_arg_name);
>> > +    if (!obj) {
>> > +        /* client arg doesn't exist */
>> > +        res->result = -1;
>> > +        qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
>> > +        return;
>> > +    }
>> > +
>> > +    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");
>> > +            res->result = -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");
>> > +            res->result = -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");
>> > +            res->result = -1;
>> > +        }
>> > +        break;
>> > +    case 'b':
>> > +    case '-':
>> > +        if (qobject_type(client_arg) != QTYPE_QBOOL) {
>> > +            qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, "bool");
>> > +            res->result = -1;
>> > +        }
>> > +        break;
>> > +    case 'O':
>> > +        /* Not checked here */
>> > +        break;
>> 
>> What about case '/'?  I guess it doesn't make much sense for QMP, but
>> the old checker handles it.  If we drop it from QMP, we should document
>> the restriction in the source.
>
>  Yes, there're two args_type we don't handle in QMP because we don't have
> any of those handlers converted: '/' and '.'.
>
>  I think it's unlikely to get them converted in this form, so the current
> implementation contains dead-code. I explained this in one commit log, but
> maybe it's the wrong place.

I read that commit message after I sent my comment :)

The commit message is fine; I'd like a comment in the code in addition,
not instead of the commit message.

[...]

Re: [Qemu-devel] [PATCH 8/9] QMP: Introduce qmp_check_input_obj()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 02 Jun 2010 09:39:26 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > This is similar to qmp_check_client_args(), but 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, each time checking if the
>> > given 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 1875731..654b193 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -4271,6 +4271,45 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>> >      return res.result;
>> >  }
>> >  
>> > +/*
>> > + * 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)
>> 
>> Really?  Checking qmp-spec.txt... yes, really.  Is it a good idea to
>> permit objects and arrays?
>
>  It was Avi's suggestion to allow anything, maybe arrays don't make sense
> but objects do.

If we permit objects, we can just as well permit anything.

Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 02 Jun 2010 09:22:40 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> Higher order functions rock.  But C is too static and limited for
>> elegant use of higher order functions.  Means to construct loops are
>> usually more convenient to use, and yield more readable code.
>> 
>> I find the use of qdict_iter() here quite tortuous: you define a
>> separate iterator function, which you can't put next to its use.  You
>> need to jump back and forth between the two places to understand what
>> the loop does.  You define a special data structure just to pass
>> arguments and results through qdict_iter().
>> 
>> Let me try to sketch the alternative:
>> 
>> static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>> {
>>     QDict *cmd_args;
>>     int res = 0, skip = 0;
>>     QDictEntry *ent;
>> 
>>     cmd_args = qdict_from_args_type(cmd->args_type);
>> 
>>     for (ent = qdict_first_entry(cmd_args); ent; ent = qdict_next_entry(ent) {
>
>  I thought about doing something similar a while ago, but I dislike it for
> two reasons:
>
>   1. I don't think the notion of 'first' and 'next' apply for dicts. One may
>      argue that the iterator has the same issue, but it's implicit

Does the dirt under the carpet exist when nobody looks?

Iterating over an unordered collection necessarily creates an order
where none was before.  It's the nature of iteration.  Dressing it up as
iterator + function argument doesn't change the basic fact[*].

>   2. QDictEntry shouldn't be part of the public interface, we should be
>      using forward declaration there

No problem, just add qdict_ent_key() and qdict_ent_value(), and use them
instead of operator ->.

>                                      (although I'm not sure whether this is
>      possible with a typedef)

In qdict.h: typedef struct QDictEntry QDictEntry;

In qdict.c: struct QDictEntry { ... };

>  I think having qdict_foreach() will improve things already.

I doubt it, but try and see :)


[*] Unless the iterator gets fancy and calls the function argument
concurrently.  Hardly an option in primitive old C.

Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 02 Jun 2010 08:59:11 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
> [...]
>
>> > +
>> > +    type = qobject_to_qstring(obj);
>> > +    assert(type != NULL);
>> > +
>> > +    if (qstring_get_str(type)[0] == 'O') {
>> > +        QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
>> > +        assert(opts_list);
>> > +        res->result = check_opts(opts_list, res->qdict);
>> 
>> I doubt this is the right place for calling check_opts.
>
>  Can you suggest the right place?

It's related to check_arg().  I figure it should be dropped along with
it in 5/9.

The new checker needs to cope with 'O':

1. Client must provide all mandatory arguments

   'O' options are optional, so there's nothing to do for
   check_mandatory_args().

2. Each argument provided by the client must be valid
3. Each argument provided by the client must have the type expected
   by the command

   'O' comes in two flavours, like the underlying QemuOptsList: desc
   present (not yet implemented) and empty.

   Empty desc means we accept any option.  check_client_args_type()
   needs to accept unknown arguments.  Is this broken in your patch?

   Non-empty desc probably should be exploded by qdict_from_args_type(),
   i.e. instead of putting (NAME, "O") into the dictionary, put the
   contents of QemuOptsList's desc.  Problem: key is obvious
   (desc->name), but value is not.  Maybe you need to represent
   "expected type" differently, so you can cover both arg_type codes and
   the QemuOptType values.  What its user really needs to know is the
   set of expected types.  Why not put that, in a suitable encoding.

   I recommend to implement only empty desc here for now, and non-empty
   when we implement it.  Would be nice if you could keep it in mind, so
   we don't have to dig up too much of the argument checker for it.

[Qemu-devel] Handling the O-type

[Luiz Capitulino]

On Wed, 02 Jun 2010 09:31:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:

[...]

> >  static void check_mandatory_args(const char *cmd_arg_name,
> > @@ -4344,6 +4413,9 @@ out:
> >   * Client argument checking rules:
> >   *
> >   * 1. Client must provide all mandatory arguments
> > + * 2. Each argument provided by the client must be valid
> > + * 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)
> >  {
> > @@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >      res.qdict = client_args;
> >      qdict_iter(cmd_args, check_mandatory_args, &res);
> >  
> > -    /* TODO: Check client args type */
> > +    if (!res.result && !res.skip) {
> > +        res.qdict = cmd_args;
> > +        qdict_iter(client_args, check_client_args_type, &res);
> > +    }
> 
> What if we have both an O-type argument and other arguments?  Then the
> 'O' makes check_client_args_type() set res.skip, and we duly skip
> checking the other arguments here.

I was working on this and it seems a bad idea to allow mixing O-type and
other monitor types*.

The reason is that you can't easily tell if an argument passed by the client
is part of the O-type or the monitor type. We could workaround this by trying to
ensure that an argument exists only in one of them, but I really feel this will
get messy.

I think we should disallow mixing O-type with other argument types and maintain
the skip trick, ie. skip any checking in the top level if the argument is an
O-type one.

 * Does this work with the current argument checker?

Re: [Qemu-devel] Handling the O-type

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 02 Jun 2010 09:31:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
> [...]
>
>> >  static void check_mandatory_args(const char *cmd_arg_name,
>> > @@ -4344,6 +4413,9 @@ out:
>> >   * Client argument checking rules:
>> >   *
>> >   * 1. Client must provide all mandatory arguments
>> > + * 2. Each argument provided by the client must be valid
>> > + * 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)
>> >  {
>> > @@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>> >      res.qdict = client_args;
>> >      qdict_iter(cmd_args, check_mandatory_args, &res);
>> >  
>> > -    /* TODO: Check client args type */
>> > +    if (!res.result && !res.skip) {
>> > +        res.qdict = cmd_args;
>> > +        qdict_iter(client_args, check_client_args_type, &res);
>> > +    }
>> 
>> What if we have both an O-type argument and other arguments?  Then the
>> 'O' makes check_client_args_type() set res.skip, and we duly skip
>> checking the other arguments here.
>
> I was working on this and it seems a bad idea to allow mixing O-type and
> other monitor types*.
>
> The reason is that you can't easily tell if an argument passed by the client
> is part of the O-type or the monitor type. We could workaround this by trying to
> ensure that an argument exists only in one of them, but I really feel this will
> get messy.
>
> I think we should disallow mixing O-type with other argument types and maintain
> the skip trick, ie. skip any checking in the top level if the argument is an
> O-type one.

If you're proposing "if you have an O-type parameter, then you can have
any other parameters", then I disagree.  That's too big a hammer.

The problem is to match actual arguments to formal parameters.

In HMP, the matching is positional.  No ambiguity as long as positions
are clearly delimited.  A positional argument maybe an O-type, and
within that argument, matching is by option name.

The big hammer restriction would make it impossible for a command to
take both positional arguments and named arguments, unless you do the
named arguments ad hoc instead of with an O-type.  Some commands already
take both positional and named arguments: pci_add, drive_add,
host_net_add.  Okay, those examples aren't exactly pinnacles of human
interface design.  Still, it's an ugly restriction.

Multiple O-types in the same command are probably a bad idea, because
the user would have to remember which option goes into what positional
argument.

In QMP, the matching is by parameter name.  No ambiguity as long as the
names are unique.  Therefore, all we need to disallow is non-unique
parameter names.

Having an args_type define the same parameter name twice is a
programming error.  It doesn't matter whether the name is right in the
string, or buried in an O-type.

Complication: you can have a QemuOptsList accept arbitrary arguments,
regardless of their name.  Checking them is left to its consumer.

The way it's currently done (empty desc[] member), such an O-type can't
declare *any* names when it accepts arbitrary arguments.

Obviously, we can't have more than one O-type parameter accept arbitrary
arguments, because we wouldn't know which O-type should get them.

An O-type accepting arbitrary arguments should receive the "left-overs",
i.e. any arguments that can't be matched to named parameters.

Summary:

* Parameter names must be unique, whether they are defined directly in
  args_type or included by an O-type.

* We can't have multiple O-types accepting arbitrary arguments.

* If we have an O-type accepting arbitrary arguments, it receives any
  arguments that can't be matched to named parameters.

* I'd be fine with the restriction to a single O-type.

>  * Does this work with the current argument checker?

I added O-types in the device_add series.  I implemented only what
device_add needs, because that series was already awkwardly long.

Sooner or later we'll want to switch to a more structured encoding of
parameters than the args_type string.  We might want to revise or ditch
the use of QemuOptsList then.

Re: [Qemu-devel] Handling the O-type

[Luiz Capitulino]

On Mon, 21 Jun 2010 10:12:06 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 02 Jun 2010 09:31:24 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> > [...]
> >
> >> >  static void check_mandatory_args(const char *cmd_arg_name,
> >> > @@ -4344,6 +4413,9 @@ out:
> >> >   * Client argument checking rules:
> >> >   *
> >> >   * 1. Client must provide all mandatory arguments
> >> > + * 2. Each argument provided by the client must be valid
> >> > + * 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)
> >> >  {
> >> > @@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
> >> >      res.qdict = client_args;
> >> >      qdict_iter(cmd_args, check_mandatory_args, &res);
> >> >  
> >> > -    /* TODO: Check client args type */
> >> > +    if (!res.result && !res.skip) {
> >> > +        res.qdict = cmd_args;
> >> > +        qdict_iter(client_args, check_client_args_type, &res);
> >> > +    }
> >> 
> >> What if we have both an O-type argument and other arguments?  Then the
> >> 'O' makes check_client_args_type() set res.skip, and we duly skip
> >> checking the other arguments here.
> >
> > I was working on this and it seems a bad idea to allow mixing O-type and
> > other monitor types*.
> >
> > The reason is that you can't easily tell if an argument passed by the client
> > is part of the O-type or the monitor type. We could workaround this by trying to
> > ensure that an argument exists only in one of them, but I really feel this will
> > get messy.
> >
> > I think we should disallow mixing O-type with other argument types and maintain
> > the skip trick, ie. skip any checking in the top level if the argument is an
> > O-type one.
> 
> If you're proposing "if you have an O-type parameter, then you can have
> any other parameters", then I disagree.  That's too big a hammer.

Not sure if this changes what you're trying to say here, but actually what
I'm saying is "if you have an O-type parameter, then argument checking is
up to you".

The best way to fix that is to do the other way around, ie. O-type should
also be checked by the new checker.

> The problem is to match actual arguments to formal parameters.
> 
> In HMP, the matching is positional.  No ambiguity as long as positions
> are clearly delimited.  A positional argument maybe an O-type, and
> within that argument, matching is by option name.

Ok, so the HMP parser can tell when an O-type sequence beings and ends,
right? By looking at the code, I have the impression it does.

In this case, the new checker should do the same. Should be possible, right?

> The big hammer restriction would make it impossible for a command to
> take both positional arguments and named arguments, unless you do the
> named arguments ad hoc instead of with an O-type.  Some commands already
> take both positional and named arguments: pci_add, drive_add,
> host_net_add.  Okay, those examples aren't exactly pinnacles of human
> interface design.  Still, it's an ugly restriction.
> 
> Multiple O-types in the same command are probably a bad idea, because
> the user would have to remember which option goes into what positional
> argument.
> 
> In QMP, the matching is by parameter name.  No ambiguity as long as the
> names are unique.  Therefore, all we need to disallow is non-unique
> parameter names.

Yes, if there's an easy way to do that I will do.

> Having an args_type define the same parameter name twice is a
> programming error.  It doesn't matter whether the name is right in the
> string, or buried in an O-type.

Sure, but it's error prone.

[...]

> Sooner or later we'll want to switch to a more structured encoding of
> parameters than the args_type string.  We might want to revise or ditch
> the use of QemuOptsList then.

Yes, and we have to decide what to do before we get there.

My suggestion is: if it's easy to do the O-type checking in the new checker,
then let's do it. Otherwise let's live with the limitation until we can
properly fix it.

Re: [Qemu-devel] Handling the O-type

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 21 Jun 2010 10:12:06 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Wed, 02 Jun 2010 09:31:24 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >
>> > [...]
>> >
>> >> >  static void check_mandatory_args(const char *cmd_arg_name,
>> >> > @@ -4344,6 +4413,9 @@ out:
>> >> >   * Client argument checking rules:
>> >> >   *
>> >> >   * 1. Client must provide all mandatory arguments
>> >> > + * 2. Each argument provided by the client must be valid
>> >> > + * 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)
>> >> >  {
>> >> > @@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>> >> >      res.qdict = client_args;
>> >> >      qdict_iter(cmd_args, check_mandatory_args, &res);
>> >> >  
>> >> > -    /* TODO: Check client args type */
>> >> > +    if (!res.result && !res.skip) {
>> >> > +        res.qdict = cmd_args;
>> >> > +        qdict_iter(client_args, check_client_args_type, &res);
>> >> > +    }
>> >> 
>> >> What if we have both an O-type argument and other arguments?  Then the
>> >> 'O' makes check_client_args_type() set res.skip, and we duly skip
>> >> checking the other arguments here.
>> >
>> > I was working on this and it seems a bad idea to allow mixing O-type and
>> > other monitor types*.
>> >
>> > The reason is that you can't easily tell if an argument passed by the client
>> > is part of the O-type or the monitor type. We could workaround this by trying to
>> > ensure that an argument exists only in one of them, but I really feel this will
>> > get messy.
>> >
>> > I think we should disallow mixing O-type with other argument types and maintain
>> > the skip trick, ie. skip any checking in the top level if the argument is an
>> > O-type one.
>> 
>> If you're proposing "if you have an O-type parameter, then you can have

"can't have" for crying out loud!

>> any other parameters", then I disagree.  That's too big a hammer.
>
> Not sure if this changes what you're trying to say here, but actually what
> I'm saying is "if you have an O-type parameter, then argument checking is
> up to you".

Workable, I think.

> The best way to fix that is to do the other way around, ie. O-type should
> also be checked by the new checker.
>
>> The problem is to match actual arguments to formal parameters.
>> 
>> In HMP, the matching is positional.  No ambiguity as long as positions
>> are clearly delimited.  A positional argument maybe an O-type, and
>> within that argument, matching is by option name.
>
> Ok, so the HMP parser can tell when an O-type sequence beings and ends,
> right? By looking at the code, I have the impression it does.

Just like any other type, the O-type tells us how to parse a single
positional argument.

> In this case, the new checker should do the same. Should be possible, right?
>
>> The big hammer restriction would make it impossible for a command to
>> take both positional arguments and named arguments, unless you do the
>> named arguments ad hoc instead of with an O-type.  Some commands already
>> take both positional and named arguments: pci_add, drive_add,
>> host_net_add.  Okay, those examples aren't exactly pinnacles of human
>> interface design.  Still, it's an ugly restriction.
>> 
>> Multiple O-types in the same command are probably a bad idea, because
>> the user would have to remember which option goes into what positional
>> argument.
>> 
>> In QMP, the matching is by parameter name.  No ambiguity as long as the
>> names are unique.  Therefore, all we need to disallow is non-unique
>> parameter names.
>
> Yes, if there's an easy way to do that I will do.
>
>> Having an args_type define the same parameter name twice is a
>> programming error.  It doesn't matter whether the name is right in the
>> string, or buried in an O-type.
>
> Sure, but it's error prone.
>
> [...]
>
>> Sooner or later we'll want to switch to a more structured encoding of
>> parameters than the args_type string.  We might want to revise or ditch
>> the use of QemuOptsList then.
>
> Yes, and we have to decide what to do before we get there.
>
> My suggestion is: if it's easy to do the O-type checking in the new checker,
> then let's do it. Otherwise let's live with the limitation until we can
> properly fix it.

Should be good enough for the initial commit.