Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Jeff Cody]

On 02/22/2012 12:35 PM, Anthony Liguori wrote:
> On 02/22/2012 10:12 AM, Jeff Cody wrote:
> > On 02/22/2012 09:53 AM, Anthony Liguori wrote:
> > > On 02/20/2012 11:31 AM, Jeff Cody wrote:
> > > > The QAPI scripts allow for generating commands that receive parameter
> > > > input consisting of a list of custom structs, but the QMP input
> > > > paramter
> > > > checking did not support receiving a qlist as opposed to a qdict for
> > > > input.
> > >
> > > What are you trying to send on the wire? Something like
> > >
> > > {"execute": "foo", "arguments": [ "a", "b", "c" ]}
> > >
> > > ?
> > >
> > > That's not valid per the QMP protocol. "arguments" must always be a QMP
> > > dictionary.
> >
> > Not a bare list like that (perhaps my wording was poor), but rather an
> > array of
> > custom structs, that contain QMP dicts. In this particular case:
> >
> > {"execute": "blockdev-group-snapshot-sync", "arguments": [{ "device":
> > "ide-hd0","snapshot-file":"/some/place/my-image","format": "qcow2" }]}
> >
> > When specifying this in the schema JSON file as shown below[1], the
> > QAPI scripts
> > generate the marshaller & visitor functions that handle everything
> > correctly.
> > The monitor code, however, could not deal with the QList container for
> > the input
> > parameters, prior to passing the data to the generated functions.
> >
> >
> > [1] JSON schema definition:
> >
> > { 'type': 'SnapshotDev',
> > 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
> >
> > { 'command': 'blockdev-group-snapshot-sync',
> > 'data': { 'dev': [ 'SnapshotDev' ] } }
>
>
> This will end up looking like:
>
> { "execute": "blockdev-group-snapshot-sync",
> "arguments": { "dev": [{"device": "ide-hd0"....}] } }
>
> Which should work as expected in QMP. HMP argument validation doesn't
> handle arguments of lists but IMHO, it's time to kill that code.
>
> Luiz, what do you think?
>
> Regards,
>
> Anthony Liguori

You are right, I just tried it the way you suggested; that works fine. 
I will drop patch 1, and I will also be dropping patch 3.  Please ignore 
patch 1/3, thanks.




> > > > This patch allows for array input parameters, although no argument
> > > > validation
> > > > is performed for commands that accept a list input.
> > > >
> > > > Signed-off-by: Jeff Cody<address@hidden>
> > > > ---
> > > > monitor.c | 72
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++----------
> > > > monitor.h | 1 +
> > > > 2 files changed, 61 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/monitor.c b/monitor.c
> > > > index a7df995..98e6017 100644
> > > > --- a/monitor.c
> > > > +++ b/monitor.c
> > > > @@ -125,8 +125,12 @@ typedef struct mon_cmd_t {
> > > > void (*info)(Monitor *mon);
> > > > void (*cmd)(Monitor *mon, const QDict *qdict);
> > > > int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
> > > > + int (*cmd_new_list)(Monitor *mon, const QList *params,
> > > > + QObject **ret_data);
> > > > int (*cmd_async)(Monitor *mon, const QDict *params,
> > > > MonitorCompletion *cb, void *opaque);
> > > > + int (*cmd_async_list)(Monitor *mon, const QList *params,
> > > > + MonitorCompletion *cb, void *opaque);
> > > > } mhandler;
> > > > bool qapi;
> > > > int flags;
> > > > @@ -353,6 +357,11 @@ static inline bool handler_is_async(const
> > > > mon_cmd_t *cmd)
> > > > return cmd->flags& MONITOR_CMD_ASYNC;
> > > > }
> > > >
> > > > +static inline bool handler_accepts_array(const mon_cmd_t *cmd)
> > > > +{
> > > > + return cmd->flags& MONITOR_CMD_ARRAY_INPUT;
> > > > +}
> > > > +
> > > > static inline int monitor_has_error(const Monitor *mon)
> > > > {
> > > > return mon->error != NULL;
> > > > @@ -671,6 +680,12 @@ static int qmp_async_cmd_handler(Monitor *mon,
> > > > const mon_cmd_t *cmd,
> > > > return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
> > > > }
> > > >
> > > > +static int qmp_async_cmd_handler_array(Monitor *mon, const mon_cmd_t
> > > > *cmd,
> > > > + const QList *params)
> > > > +{
> > > > + return cmd->mhandler.cmd_async_list(mon, params,
> > > > qmp_monitor_complete, mon);
> > > > +}
> > > > +
> > > > static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
> > > > const QDict *params)
> > > > {
> > > > @@ -4310,7 +4325,8 @@ static QDict *qmp_check_input_obj(QObject
> > > > *input_obj)
> > > > }
> > > > has_exec_key = 1;
> > > > } else if (!strcmp(arg_name, "arguments")) {
> > > > - if (qobject_type(arg_obj) != QTYPE_QDICT) {
> > > > + if ((qobject_type(arg_obj) != QTYPE_QDICT)&&
> > > > + (qobject_type(arg_obj) != QTYPE_QLIST)) {
> > > > qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
> > > > "object");
> > > > return NULL;
> > > > @@ -4345,11 +4361,26 @@ static void qmp_call_cmd(Monitor *mon, const
> > > > mon_cmd_t *cmd,
> > > > qobject_decref(data);
> > > > }
> > > >
> > > > +static void qmp_call_cmd_array(Monitor *mon, const mon_cmd_t *cmd,
> > > > + const QList *params)
> > > > +{
> > > > + int ret;
> > > > + QObject *data = NULL;
> > > > +
> > > > + mon_print_count_init(mon);
> > > > +
> > > > + ret = cmd->mhandler.cmd_new_list(mon, params,&data);
> > > > + handler_audit(mon, cmd, ret);
> > > > + monitor_protocol_emitter(mon, data);
> > > > + qobject_decref(data);
> > > > +}
> > > > +
> > > > static void handle_qmp_command(JSONMessageParser *parser, QList
> > > > *tokens)
> > > > {
> > > > int err;
> > > > QObject *obj;
> > > > QDict *input, *args;
> > > > + QList *args_list = NULL;
> > > > const mon_cmd_t *cmd;
> > > > const char *cmd_name;
> > > > Monitor *mon = cur_mon;
> > > > @@ -4386,26 +4417,42 @@ static void
> > > > handle_qmp_command(JSONMessageParser *parser, QList *tokens)
> > > > }
> > > >
> > > > obj = qdict_get(input, "arguments");
> > > > - if (!obj) {
> > > > - args = qdict_new();
> > > > + if (handler_accepts_array(cmd)) {
> > > > + if (!obj || (qobject_type(obj) != QTYPE_QLIST)) {
> > > > + args_list = qlist_new();
> > > > + } else {
> > > > + args_list = qobject_to_qlist(obj);
> > > > + QINCREF(args_list);
> > > > + }
> > > > } else {
> > > > - args = qobject_to_qdict(obj);
> > > > - QINCREF(args);
> > > > - }
> > > > -
> > > > - err = qmp_check_client_args(cmd, args);
> > > > - if (err< 0) {
> > > > - goto err_out;
> > > > + if (!obj || (qobject_type(obj) != QTYPE_QDICT)) {
> > > > + args = qdict_new();
> > > > + } else {
> > > > + args = qobject_to_qdict(obj);
> > > > + QINCREF(args);
> > > > + }
> > > > + err = qmp_check_client_args(cmd, args);
> > > > + if (err< 0) {
> > > > + goto err_out;
> > > > + }
> > > > }
> > > >
> > > > if (handler_is_async(cmd)) {
> > > > - err = qmp_async_cmd_handler(mon, cmd, args);
> > > > + if (handler_accepts_array(cmd)) {
> > > > + err = qmp_async_cmd_handler_array(mon, cmd, args_list);
> > > > + } else {
> > > > + err = qmp_async_cmd_handler(mon, cmd, args);
> > > > + }
> > > > if (err) {
> > > > /* emit the error response */
> > > > goto err_out;
> > > > }
> > > > } else {
> > > > - qmp_call_cmd(mon, cmd, args);
> > > > + if (handler_accepts_array(cmd)) {
> > > > + qmp_call_cmd_array(mon, cmd, args_list);
> > > > + } else {
> > > > + qmp_call_cmd(mon, cmd, args);
> > > > + }
> > > > }
> > > >
> > > > goto out;
> > > > @@ -4415,6 +4462,7 @@ err_out:
> > > > out:
> > > > QDECREF(input);
> > > > QDECREF(args);
> > > > + QDECREF(args_list);
> > > > }
> > > >
> > > > /**
> > > > diff --git a/monitor.h b/monitor.h
> > > > index b72ea07..499fc1f 100644
> > > > --- a/monitor.h
> > > > +++ b/monitor.h
> > > > @@ -19,6 +19,7 @@ extern Monitor *default_mon;
> > > >
> > > > /* flags for monitor commands */
> > > > #define MONITOR_CMD_ASYNC 0x0001
> > > > +#define MONITOR_CMD_ARRAY_INPUT 0x0002
> > > >
> > > > /* QMP events */
> > > > typedef enum MonitorEvent {