From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52523 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OJoOy-0004H1-SS for qemu-devel@nongnu.org; Wed, 02 Jun 2010 09:54:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OJoOw-0006Ol-Vh for qemu-devel@nongnu.org; Wed, 02 Jun 2010 09:54:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31058) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OJoOw-0006Od-OT for qemu-devel@nongnu.org; Wed, 02 Jun 2010 09:54:34 -0400 Date: Wed, 2 Jun 2010 10:54:28 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code Message-ID: <20100602105428.4bf54815@redhat.com> In-Reply-To: References: <1275424897-32253-1-git-send-email-lcapitulino@redhat.com> <1275424897-32253-5-git-send-email-lcapitulino@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: jan.kiszka@siemens.com, qemu-devel@nongnu.org On Wed, 02 Jun 2010 09:31:24 +0200 Markus Armbruster wrote: > Luiz Capitulino 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 > > --- > > 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; >