From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39132 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OJiHl-0005LO-9r for qemu-devel@nongnu.org; Wed, 02 Jun 2010 03:22:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OJiHk-0003IB-76 for qemu-devel@nongnu.org; Wed, 02 Jun 2010 03:22:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45210) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OJiHk-0003I1-0j for qemu-devel@nongnu.org; Wed, 02 Jun 2010 03:22:44 -0400 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code References: <1275424897-32253-1-git-send-email-lcapitulino@redhat.com> <1275424897-32253-4-git-send-email-lcapitulino@redhat.com> Date: Wed, 02 Jun 2010 09:22:40 +0200 In-Reply-To: <1275424897-32253-4-git-send-email-lcapitulino@redhat.com> (Luiz Capitulino's message of "Tue, 1 Jun 2010 17:41:31 -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 There's more... Luiz Capitulino writes: > This commit introduces the first half of qmp_check_client_args(), > which is the new client argument checker. > > It's introduced on top of the existing code, so that there are > no regressions during the transition. > > It works this way: the command's args_type field (from > qemu-monitor.hx) is transformed into a qdict. Then we iterate > over it checking whether each mandatory argument has been > provided by the client. > > All comunication between the iterator and its caller is done > via the new QMPArgCheckRes type. > > Next commit adds the second half of this work: type checking > and invalid argument detection. > > Signed-off-by: Luiz Capitulino > --- > monitor.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 107 insertions(+), 0 deletions(-) > > diff --git a/monitor.c b/monitor.c > index bc3cc18..47a0da8 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4259,6 +4259,108 @@ static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name) > return (qmp_cmd_mode(mon) ? is_cap : !is_cap); > } > > +typedef struct QMPArgCheckRes { > + int result; > + int skip; > + QDict *qdict; > +} QMPArgCheckRes; > + > +/* > + * Check if client passed all mandatory args > + */ > +static void check_mandatory_args(const char *cmd_arg_name, > + QObject *obj, void *opaque) > +{ > + QString *type; > + QMPArgCheckRes *res = opaque; > + > + if (res->result < 0) { > + /* report only the first error */ > + return; > + } > + > + type = qobject_to_qstring(obj); > + assert(type != NULL); > + > + if (qstring_get_str(type)[0] == 'O') { > + QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name); > + assert(opts_list); > + res->result = check_opts(opts_list, res->qdict); > + res->skip = 1; > + } else if (qstring_get_str(type)[0] != '-' && > + qstring_get_str(type)[1] != '?' && > + !qdict_haskey(res->qdict, cmd_arg_name)) { > + res->result = -1; This is a sign that the iterator needs a way to return a value. Check out qemu_opts_foreach(), it can break and return a value. > + qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name); > + } > +} > + > +static QDict *qdict_from_args_type(const char *args_type) > +{ > + int i; > + QDict *qdict; > + QString *key, *type, *cur_qs; > + > + assert(args_type != NULL); > + > + qdict = qdict_new(); > + > + if (args_type == NULL || args_type[0] == '\0') { > + /* no args, empty qdict */ > + goto out; > + } > + > + key = qstring_new(); > + type = qstring_new(); > + > + cur_qs = key; > + > + for (i = 0;; i++) { > + switch (args_type[i]) { > + case ',': > + case '\0': > + qdict_put(qdict, qstring_get_str(key), type); > + QDECREF(key); > + if (args_type[i] == '\0') { > + goto out; > + } > + type = qstring_new(); /* qdict has ref */ > + cur_qs = key = qstring_new(); > + break; > + case ':': > + cur_qs = type; > + break; > + default: > + qstring_append_chr(cur_qs, args_type[i]); > + break; > + } > + } > + > +out: > + return qdict; > +} > + > +/* > + * Client argument checking rules: > + * > + * 1. Client must provide all mandatory arguments > + */ > +static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args) > +{ > + QDict *cmd_args; > + QMPArgCheckRes res = { .result = 0, .skip = 0 }; > + > + cmd_args = qdict_from_args_type(cmd->args_type); > + > + res.qdict = client_args; > + qdict_iter(cmd_args, check_mandatory_args, &res); > + > + /* TODO: Check client args type */ > + > + QDECREF(cmd_args); > + return res.result; > +} Higher order functions rock. But C is too static and limited for elegant use of higher order functions. Means to construct loops are usually more convenient to use, and yield more readable code. I find the use of qdict_iter() here quite tortuous: you define a separate iterator function, which you can't put next to its use. You need to jump back and forth between the two places to understand what the loop does. You define a special data structure just to pass arguments and results through qdict_iter(). Let me try to sketch the alternative: static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args) { QDict *cmd_args; int res = 0, skip = 0; QDictEntry *ent; cmd_args = qdict_from_args_type(cmd->args_type); for (ent = qdict_first_entry(cmd_args); ent; ent = qdict_next_entry(ent) { type = qobject_to_qstring(ent->value); assert(type != NULL); if (qstring_get_str(type)[0] == 'O') { QemuOptsList *opts_list = qemu_find_opts(ent->key); assert(opts_list); res = check_opts(opts_list, cmd_args); skip = 1; } else if (qstring_get_str(type)[0] != '-' && qstring_get_str(type)[1] != '?' && !qdict_haskey(cmd_args, ent->key)) { qerror_report(QERR_MISSING_PARAMETER, ent->key); res = -1; break; } } /* TODO: Check client args type */ QDECREF(cmd_args); return res; } > + > static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) > { > int err; > @@ -4334,6 +4436,11 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) > > QDECREF(input); > > + err = qmp_check_client_args(cmd, args); > + if (err < 0) { > + goto err_out; > + } > + > err = monitor_check_qmp_args(cmd, args); > if (err < 0) { > goto err_out;