[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checki
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code |
Date: |
Wed, 2 Jun 2010 10:54:28 -0300 |
On Wed, 02 Jun 2010 09:31:24 +0200
Markus Armbruster <address@hidden> wrote:
> Luiz Capitulino <address@hidden> 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 <address@hidden>
> > ---
> > 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;
>
- Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code, (continued)
Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code, Markus Armbruster, 2010/06/02
[Qemu-devel] [PATCH 2/9] Monitor: handle optional '-' arg as a bool, Luiz Capitulino, 2010/06/01
[Qemu-devel] [PATCH 5/9] QMP: Drop old client argument checker, Luiz Capitulino, 2010/06/01
[Qemu-devel] [PATCH 6/9] QMP: check_opts(): Minor cleanup, Luiz Capitulino, 2010/06/01
[Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code, Luiz Capitulino, 2010/06/01
- Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code, Markus Armbruster, 2010/06/02
- [Qemu-devel] Handling the O-type, Luiz Capitulino, 2010/06/18
- Re: [Qemu-devel] Handling the O-type, Markus Armbruster, 2010/06/21
- Re: [Qemu-devel] Handling the O-type, Luiz Capitulino, 2010/06/21
- Re: [Qemu-devel] Handling the O-type, Markus Armbruster, 2010/06/21
[Qemu-devel] [PATCH 7/9] QError: Introduce QERR_QMP_BAD_INPUT_OBJECT_MEMBER, Luiz Capitulino, 2010/06/01
[Qemu-devel] [PATCH 8/9] QMP: Introduce qmp_check_input_obj(), Luiz Capitulino, 2010/06/01