From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41377 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OJp8o-0005tk-PA for qemu-devel@nongnu.org; Wed, 02 Jun 2010 10:41:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OJp8e-0006hv-Ei for qemu-devel@nongnu.org; Wed, 02 Jun 2010 10:41:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26743) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OJp8e-0006hi-7X for qemu-devel@nongnu.org; Wed, 02 Jun 2010 10:41:48 -0400 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code References: <1275424897-32253-1-git-send-email-lcapitulino@redhat.com> <1275424897-32253-5-git-send-email-lcapitulino@redhat.com> <20100602105428.4bf54815@redhat.com> Date: Wed, 02 Jun 2010 16:41:43 +0200 In-Reply-To: <20100602105428.4bf54815@redhat.com> (Luiz Capitulino's message of "Wed, 2 Jun 2010 10:54:28 -0300") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: jan.kiszka@siemens.com, qemu-devel@nongnu.org Luiz Capitulino writes: > 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. I read that commit message after I sent my comment :) The commit message is fine; I'd like a comment in the code in addition, not instead of the commit message. [...]