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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 06/15/2011 03:12 PM, Luiz Capitulino wrote:
> On Wed, 15 Jun 2011 14:45:30 -0500
> Anthony Liguori<aliguori@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<mdroth@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.

>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>>> +    }
>>>> +
>>>> +    return QOBJECT(rsp);
>>>> +}
>>>
>>
>