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

[Markus Armbruster <armbru@redhat.com>]

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;