| Message ID | 1308018686-8235-10-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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); > +}
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); >> +} >
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); > >> +} > > >
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); >>>> +} >>> >> >
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); > >>>> +} > >>> > >> > > >
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); +}
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