From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:45523) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWx1G-0008KF-GU for qemu-devel@nongnu.org; Wed, 15 Jun 2011 16:48:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QWx1E-0002dR-Kv for qemu-devel@nongnu.org; Wed, 15 Jun 2011 16:48:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46180) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWx1D-0002dG-Vj for qemu-devel@nongnu.org; Wed, 15 Jun 2011 16:48:56 -0400 Date: Wed, 15 Jun 2011 17:48:51 -0300 From: Luiz Capitulino Message-ID: <20110615174851.6e335c43@doriath> In-Reply-To: <4DF919E0.9030508@linux.vnet.ibm.com> References: <1308018686-8235-1-git-send-email-mdroth@linux.vnet.ibm.com> <1308018686-8235-10-git-send-email-mdroth@linux.vnet.ibm.com> <20110615163314.2db81c17@doriath> <4DF90BDA.2050201@us.ibm.com> <20110615171248.191abf2a@doriath> <4DF919E0.9030508@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 09/21] qapi: add QMP dispatch functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: Jes.Sorensen@redhat.com, Anthony Liguori , agl@linux.vnet.ibm.com, qemu-devel@nongnu.org On Wed, 15 Jun 2011 15:45:20 -0500 Michael Roth wrote: > On 06/15/2011 03:12 PM, Luiz Capitulino wrote: > > On Wed, 15 Jun 2011 14:45:30 -0500 > > Anthony Liguori wrote: > > > >> On 06/15/2011 02:33 PM, Luiz Capitulino wrote: > >>> On Mon, 13 Jun 2011 21:31:14 -0500 > >>> Michael Roth wrote: > >>> > >>> > >>>> +{ > >>>> + const char *command; > >>>> + QDict *args, *dict; > >>>> + QmpCommand *cmd; > >>>> + QObject *ret = NULL; > >>>> + > >>>> + if (qobject_type(request) != QTYPE_QDICT) { > >>>> + error_set(errp, QERR_JSON_PARSE_ERROR, "request is not a dictionary"); > >>>> + goto out; > >>>> + } > >>>> + > >>>> + dict = qobject_to_qdict(request); > >>>> + if (!qdict_haskey(dict, "execute")) { > >>>> + error_set(errp, QERR_JSON_PARSE_ERROR, "no execute key"); > >>>> + goto out; > >>>> + } > >>>> + > >>>> + command = qdict_get_str(dict, "execute"); > >>>> + cmd = qmp_find_command(command); > >>>> + if (cmd == NULL) { > >>>> + error_set(errp, QERR_COMMAND_NOT_FOUND, command); > >>>> + goto out; > >>>> + } > >>>> + > >>>> + if (!qdict_haskey(dict, "arguments")) { > >>>> + args = qdict_new(); > >>>> + } else { > >>>> + args = qdict_get_qdict(dict, "arguments"); > >>>> + QINCREF(args); > >>>> + } > >>> > >>> This function doesn't seem to handle extra keys in the command dict, like: > >>> > >>> { "execute": "query-block", "foo": "bar" } > >>> > >>> You probably want to use qmp_check_input_obj() here. > >> > >> That's a feature, no? > >> > >> "Be liberal in what you accept, conservative in what you send." > > > > I'm not sure the principle applies in this case, as this is an invalid > > argument. This is the kind of thing that could give a hard time to clients, > > like using a new argument on an old command and wonder why it doesn't work. > > > > Libvirt did something like this in the past when we weren't doing the check, > > they were passing an additional key for some command and expecting it would > > have the desired functionality. > > > >>>> + > >>>> + switch (cmd->type) { > >>>> + case QCT_NORMAL: > >>>> + cmd->fn(args,&ret, errp); > >>>> + if (!error_is_set(errp)&& ret == NULL) { > >>>> + ret = QOBJECT(qdict_new()); > >>>> + } > >>>> + break; > >>>> + } > >>>> + > >>>> + QDECREF(args); > >>>> + > >>>> +out: > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +QObject *qmp_dispatch(QObject *request) > >>>> +{ > >>>> + Error *err = NULL; > >>>> + QObject *ret; > >>>> + QDict *rsp; > >>>> + > >>>> + ret = qmp_dispatch_err(request,&err); > >>>> + > >>>> + rsp = qdict_new(); > >>>> + if (err) { > >>>> + qdict_put_obj(rsp, "error", error_get_qobject(err)); > >>>> + error_free(err); > >>>> + } else if (ret) { > >>>> + qdict_put_obj(rsp, "return", ret); > >>>> + } else { > >>>> + QDECREF(rsp); > >>>> + return NULL; > >>> > >>> When does the 'else' condition happens? > >> > >> Signals which aren't in this patch series. > > > > It can be dropped then. > > > > I think it's still a good safeguard in the meantime. Whether it's > reachable or not is hard to know without looking over a lot of code > outside the function, and things can change over time. This way the user > can expect a NULL for an undefined error, as opposed to an empty > dictionary they need to free, which isn't very intuitive. Makes sense. Although I think I prefer an assert(). But I'm not strong about it. > > >> > >> Regards, > >> > >> Anthony Liguori > >> > >>> > >>>> + } > >>>> + > >>>> + return QOBJECT(rsp); > >>>> +} > >>> > >> > > >