On Wed, 02 Jun 2010 09:31:24 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@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 <lcapitul...@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. Yes, there're two args_type we don't handle in QMP because we don't have any of those handlers converted: '/' and '.'. I think it's unlikely to get them converted in this form, so the current implementation contains dead-code. I explained this in one commit log, but maybe it's the wrong place. > > > + 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. Oh, good catch. I thought that that was what the current code does, but looks like I misread it. > Again, the iterator makes for tortuous code. > > > > > QDECREF(cmd_args); > > return res.result; >