From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52357 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OJoOM-0003qx-L9 for qemu-devel@nongnu.org; Wed, 02 Jun 2010 09:54:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OJoOL-0006IN-8e for qemu-devel@nongnu.org; Wed, 02 Jun 2010 09:53:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48411) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OJoOK-0006IC-Un for qemu-devel@nongnu.org; Wed, 02 Jun 2010 09:53:57 -0400 Date: Wed, 2 Jun 2010 10:53:51 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code Message-ID: <20100602105351.5a197e75@redhat.com> In-Reply-To: References: <1275424897-32253-1-git-send-email-lcapitulino@redhat.com> <1275424897-32253-4-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:22:40 +0200 Markus Armbruster wrote: > There's more... Good! > Luiz Capitulino writes: [...] > > +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. Ah, that's good, I was wondering how I could do that but couldn't find a good way. [...] > 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) { I thought about doing something similar a while ago, but I dislike it for two reasons: 1. I don't think the notion of 'first' and 'next' apply for dicts. One may argue that the iterator has the same issue, but it's implicit 2. QDictEntry shouldn't be part of the public interface, we should be using forward declaration there (although I'm not sure whether this is possible with a typedef) I think having qdict_foreach() will improve things already.