Patchwork [09/13] QMP: New argument checker (second part)

login
register
mail settings
Submitter Luiz Capitulino
Date June 22, 2010, 5:40 p.m.
Message ID <1277228451-7741-10-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/56553/
State New
Headers show

Comments

Luiz Capitulino - June 22, 2010, 5:40 p.m.
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 <lcapitulino@redhat.com>
---
 monitor.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 99 insertions(+), 1 deletions(-)
Markus Armbruster - June 23, 2010, 3:21 p.m.
Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> ---
>  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;

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?

> +        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);
Luiz Capitulino - June 23, 2010, 4:28 p.m.
On Wed, 23 Jun 2010 17:21:12 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> > ---
> >  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);
>
Markus Armbruster - June 23, 2010, 5:16 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 23 Jun 2010 17:21:12 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
>> > ---
>> >  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.

[...]

Patch

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? */
+            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
+            return -1;
+        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);