Patchwork [v3,09/21] qapi: add QMP dispatch functions

login
register
mail settings
Submitter Michael Roth
Date June 14, 2011, 2:31 a.m.
Message ID <1308018686-8235-10-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/100242/
State New
Headers show

Comments

Michael Roth - June 14, 2011, 2:31 a.m.
Given an object recieved via QMP, this code uses the dispatch table
provided by qmp_registry.c to call the corresponding marshalling/dispatch
function and format return values/errors for delivery to the QMP.
Currently only synchronous QMP functions are supported, but this will
also be used for async QMP functions and QMP guest proxy dispatch as
well.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 Makefile.objs       |    2 +-
 qapi/qmp-core.h     |    1 +
 qapi/qmp-dispatch.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 1 deletions(-)
 create mode 100644 qapi/qmp-dispatch.c
Luiz Capitulino - June 15, 2011, 7:33 p.m.
On Mon, 13 Jun 2011 21:31:14 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Given an object recieved via QMP, this code uses the dispatch table
> provided by qmp_registry.c to call the corresponding marshalling/dispatch
> function and format return values/errors for delivery to the QMP.
> Currently only synchronous QMP functions are supported, but this will
> also be used for async QMP functions and QMP guest proxy dispatch as
> well.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  Makefile.objs       |    2 +-
>  qapi/qmp-core.h     |    1 +
>  qapi/qmp-dispatch.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 1 deletions(-)
>  create mode 100644 qapi/qmp-dispatch.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 092f314..7e82587 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -368,7 +368,7 @@ libcacard-y = cac.o event.o vcard.o vreader.o vcard_emul_nss.o vcard_emul_type.o
>  # qapi
>  
>  qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o qmp-output-visitor.o qapi-dealloc-visitor.o
> -qapi-nested-y += qmp-registry.o
> +qapi-nested-y += qmp-registry.o qmp-dispatch.o
>  qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>  
>  vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
> diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h
> index 99e929f..f1c26e4 100644
> --- a/qapi/qmp-core.h
> +++ b/qapi/qmp-core.h
> @@ -35,6 +35,7 @@ typedef struct QmpCommand
>  
>  void qmp_register_command(const char *name, QmpCommandFunc *fn);
>  QmpCommand *qmp_find_command(const char *name);
> +QObject *qmp_dispatch(QObject *request);
>  
>  #endif
>  
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> new file mode 100644
> index 0000000..5bf41ea
> --- /dev/null
> +++ b/qapi/qmp-dispatch.c
> @@ -0,0 +1,76 @@
> +#include "qemu-objects.h"
> +#include "qapi/qmp-core.h"
> +#include "json-parser.h"
> +#include "error.h"
> +#include "error_int.h"
> +#include "qerror.h"
> +
> +static QObject *qmp_dispatch_err(QObject *request, Error **errp)

Please, call this do_qmp_dispatch() instead. Every time I turn to this
function I tend to think this is only called when dispatch fails :)

> +{
> +    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.

> +
> +    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?

> +    }
> +
> +    return QOBJECT(rsp);
> +}
Anthony Liguori - June 15, 2011, 7:45 p.m.
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."

>
>> +
>> +    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.

Regards,

Anthony Liguori

>
>> +    }
>> +
>> +    return QOBJECT(rsp);
>> +}
>
Luiz Capitulino - June 15, 2011, 8:12 p.m.
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.

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >> +    }
> >> +
> >> +    return QOBJECT(rsp);
> >> +}
> >
>
Michael Roth - June 15, 2011, 8:45 p.m.
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);
>>>> +}
>>>
>>
>
Luiz Capitulino - June 15, 2011, 8:48 p.m.
On Wed, 15 Jun 2011 15:45:20 -0500
Michael Roth <mdroth@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<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.

Makes sense. Although I think I prefer an assert(). But I'm not strong
about it.

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

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 092f314..7e82587 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -368,7 +368,7 @@  libcacard-y = cac.o event.o vcard.o vreader.o vcard_emul_nss.o vcard_emul_type.o
 # qapi
 
 qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o qmp-output-visitor.o qapi-dealloc-visitor.o
-qapi-nested-y += qmp-registry.o
+qapi-nested-y += qmp-registry.o qmp-dispatch.o
 qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
 
 vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h
index 99e929f..f1c26e4 100644
--- a/qapi/qmp-core.h
+++ b/qapi/qmp-core.h
@@ -35,6 +35,7 @@  typedef struct QmpCommand
 
 void qmp_register_command(const char *name, QmpCommandFunc *fn);
 QmpCommand *qmp_find_command(const char *name);
+QObject *qmp_dispatch(QObject *request);
 
 #endif
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
new file mode 100644
index 0000000..5bf41ea
--- /dev/null
+++ b/qapi/qmp-dispatch.c
@@ -0,0 +1,76 @@ 
+#include "qemu-objects.h"
+#include "qapi/qmp-core.h"
+#include "json-parser.h"
+#include "error.h"
+#include "error_int.h"
+#include "qerror.h"
+
+static QObject *qmp_dispatch_err(QObject *request, Error **errp)
+{
+    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);
+    }
+
+    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;
+    }
+
+    return QOBJECT(rsp);
+}