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

[Michael Roth]

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.

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