Re: [Qemu-devel] [PATCH 09/13] QMP: New argument checker (second part)

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 23 Jun 2010 17:21:12 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > This commit introduces the second (and last) part of QMP's new
>> > argument checker.
>> >
>> > The job is done by check_client_args_type(), it iterates over
>> > the client's argument qdict and for for each argument it checks
>> > if it exists and if its type is valid.
>> >
>> > It's important to observe the following changes from the existing
>> > argument checker:
>> >
>> >   - If the handler accepts an O-type argument, unknown arguments
>> >     are passed down to it. It's up to O-type handlers to validate
>> >     their arguments
>> >
>> >   - Boolean types (eg. 'b' and '-') don't accept integers anymore,
>> >     only json-bool
>> >
>> >   - Argument types '/' and '.' are currently unsupported under QMP,
>> >     thus they're not handled
>> >
>> > Signed-off-by: Luiz Capitulino <address@hidden>
>> > ---
>> >  monitor.c |  100 
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 files changed, 99 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index b4fe5ba..8d074c2 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -4139,6 +4139,101 @@ static int invalid_qmp_mode(const Monitor *mon, 
>> > const char *cmd_name)
>> >  }
>> >  
>> >  /*
>> > + * Argument validation rules:
>> > + *
>> > + * 1. The argument must exist in cmd_args qdict
>> > + * 2. The argument type must be the expected one
>> > + *
>> > + * Special case: If the argument doesn't exist in cmd_args and
>> > + *               the QMP_CHECKER_OTYPE flag is set, then the
>> > + *               argument is considered an O-type one and the
>> > + *               checking is skipped for it.
>> > + */
>> > +static int check_client_args_type(const QDict *client_args,
>> > +                                  const QDict *cmd_args, int flags)
>> > +{
>> > +    const QDictEntry *ent;
>> > +
>> > +    for (ent = qdict_first(client_args); ent;ent = 
>> > qdict_next(client_args,ent)){
>> > +        QObject *obj;
>> > +        QString *arg_type;
>> > +        const QObject *client_arg = qdict_entry_value(ent);
>> > +        const char *client_arg_name = qdict_entry_key(ent);
>> > +
>> > +        obj = qdict_get(cmd_args, client_arg_name);
>> > +        if (!obj) {
>> > +            if (flags & QMP_CHECKER_OTYPE) {
>> > +                /*
>> > +                 * This handler accepts O-type arguments, it's up to it to
>> > +                 * check for unknowns and validate its type.
>> > +                 */
>> > +                continue;
>> > +            }
>> > +            /* client arg doesn't exist */
>> > +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
>> > +            return -1;
>> > +        }
>> > +
>> > +        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");
>> > +                return -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");
>> > +                return -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");
>> > +               return -1; 
>> > +            }
>> > +            break;
>> > +        case 'b':
>> > +        case '-':
>> > +            if (qobject_type(client_arg) != QTYPE_QBOOL) {
>> > +                qerror_report(QERR_INVALID_PARAMETER_TYPE, 
>> > client_arg_name,
>> > +                              "bool");
>> > +               return -1; 
>> > +            }
>> > +            break;
>> > +        case 'O':
>> > +            /* XXX: this argument has the same name of the O-type defined 
>> > in
>> > +                    in qemu-monitor.hx. This is not allowed, right? */
>> 
>> 
>> No, it's actually fine.
>> 
>> Consider device_add.  Its args_type is "device:O".  Nevertheless, it's
>> pefectly okay for a qdev to have a property named "device".
>> 
>> > +            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
>> > +            return -1;
>> 
>> Instead:
>> 
>>                assert(flags & QMP_CHECKER_OTYPE);
>>                continue;
>
> Ok, but isn't it better to choose a name which is unlikely to be a property?
> Maybe something like "device_add_opts_list:O"?

No.  The "name" of the O-type argument is not really an argument name.
When I created the O-type, I needed the name of a QemuOptsList, and I
had no use for the argument name, so I chose to simply (ab)use the
argument name.  Saved me the trouble of adding even more syntax to
args_type.

Since it's not an argument name, the argument checker must ignore it.

>> Not sure I like the name QMP_CHECKER_OTYPE.  The way it's used, it means
>> "in addition to checking declared arguments, accept undeclared arguments
>> without checking them (somebody else will check)".  'O-type' is merely
>> something that triggers that flag.  Happens to be the only way right
>> now.
>> 
>> QMP_CHECKER_ACCEPT_MORE_ARGS?
>
> Or QMP_CHECKER_ACCEPT_UNKNOWNS, but it's too long..

If you want it shorter, you could leave out CHECKER and/or ACCEPT.  Or
you could say VARARGS instead of MORE_ARGS.

[...]