On Wed, 15 Jun 2011 15:45:20 -0500
Michael Roth <mdr...@linux.vnet.ibm.com> wrote:
> On 06/15/2011 03:12 PM, Luiz Capitulino wrote:
> > On Wed, 15 Jun 2011 14:45:30 -0500
> > Anthony Liguori<aligu...@us.ibm.com> wrote:
> >
> >> On 06/15/2011 02:33 PM, Luiz Capitulino wrote:
> >>> On Mon, 13 Jun 2011 21:31:14 -0500
> >>> Michael Roth<mdr...@linux.vnet.ibm.com> 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);
> >>>> +}
> >>>
> >>
> >
>
