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

[Markus Armbruster <armbru@redhat.com>]

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;