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

[Luiz Capitulino]

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"?

> 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..

> 
> > +        case '/':
> > +        case '.':
> > +            /*
> > +             * These types are not supported by QMP and thus are not
> > +             * handled here. Fall through.
> > +             */
> > +        default:
> > +            abort();
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> >   * - Check if the client has passed all mandatory args
> >   * - Set special flags for argument validation
> >   */
> > @@ -4215,6 +4310,9 @@ out:
> >   * Client argument checking rules:
> >   *
> >   * 1. Client must provide all mandatory arguments
> > + * 2. Each argument provided by the client must be expected
> > + * 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)
> >  {
> > @@ -4229,7 +4327,7 @@ static int qmp_check_client_args(const mon_cmd_t 
> > *cmd, QDict *client_args)
> >          goto out;
> >      }
> >  
> > -    /* TODO: Check client args type */
> > +    err = check_client_args_type(client_args, cmd_args, flags);
> >  
> >  out:
> >      QDECREF(cmd_args);
>