Re: [Qemu-devel] [PATCH v3 09/21] qapi: add QMP dispatch functions

[Luiz Capitulino]

On Wed, 15 Jun 2011 15:45:20 -0500
Michael Roth <address@hidden> wrote:

> On 06/15/2011 03:12 PM, Luiz Capitulino wrote:
> > On Wed, 15 Jun 2011 14:45:30 -0500
> > Anthony Liguori<address@hidden>  wrote:
> >
> >> On 06/15/2011 02:33 PM, Luiz Capitulino wrote:
> >>> On Mon, 13 Jun 2011 21:31:14 -0500
> >>> Michael Roth<address@hidden>   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);
> >>>> +}
> >>>
> >>
> >
>