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

[Markus Armbruster <armbru@redhat.com>]

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);
> +    }
> +}
[...]