From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57371) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWwxy-0007UN-PT for qemu-devel@nongnu.org; Wed, 15 Jun 2011 16:45:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QWwxw-0001tk-LH for qemu-devel@nongnu.org; Wed, 15 Jun 2011 16:45:34 -0400 Received: from e4.ny.us.ibm.com ([32.97.182.144]:51240) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWwxw-0001tZ-8D for qemu-devel@nongnu.org; Wed, 15 Jun 2011 16:45:32 -0400 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by e4.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p5FKNqPN010225 for ; Wed, 15 Jun 2011 16:23:52 -0400 Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p5FKjMuu536810 for ; Wed, 15 Jun 2011 16:45:22 -0400 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p5FGj9bb005788 for ; Wed, 15 Jun 2011 13:45:10 -0300 Message-ID: <4DF919E0.9030508@linux.vnet.ibm.com> Date: Wed, 15 Jun 2011 15:45:20 -0500 From: Michael Roth MIME-Version: 1.0 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> In-Reply-To: <20110615171248.191abf2a@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Luiz Capitulino Cc: Jes.Sorensen@redhat.com, Anthony Liguori , agl@linux.vnet.ibm.com, qemu-devel@nongnu.org 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. >> >> Regards, >> >> Anthony Liguori >> >>> >>>> + } >>>> + >>>> + return QOBJECT(rsp); >>>> +} >>> >> >