Patchwork [3/9] QMP: First half of the new argument checking code

login
register
mail settings
Submitter Luiz Capitulino
Date June 1, 2010, 8:41 p.m.
Message ID <1275424897-32253-4-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/54306/
State New
Headers show

Comments

Luiz Capitulino - June 1, 2010, 8:41 p.m.
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(-)
Markus Armbruster - June 2, 2010, 6:59 a.m.
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);
> +    }
> +}
[...]
Markus Armbruster - June 2, 2010, 7:22 a.m.
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;
Luiz Capitulino - June 2, 2010, 1:53 p.m.
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?
Luiz Capitulino - June 2, 2010, 1:53 p.m.
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.
Markus Armbruster - June 2, 2010, 2:52 p.m.
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.
Markus Armbruster - June 3, 2010, 7:35 a.m.
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.

Patch

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;