From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0GtP-0000iA-7N for qemu-devel@nongnu.org; Wed, 22 Feb 2012 13:26:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S0GtN-0005XF-P3 for qemu-devel@nongnu.org; Wed, 22 Feb 2012 13:26:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:6029) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0GtN-0005X9-Er for qemu-devel@nongnu.org; Wed, 22 Feb 2012 13:26:17 -0500 Message-ID: <4F453344.7080903@redhat.com> Date: Wed, 22 Feb 2012 13:26:12 -0500 From: Jeff Cody MIME-Version: 1.0 References: <4F450170.5070806@codemonkey.ws> <4F4513FE.4070100@redhat.com> <4F45275E.4000304@codemonkey.ws> In-Reply-To: <4F45275E.4000304@codemonkey.ws> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Kevin Wolf , Markus Armbruster , qemu-devel@nongnu.org, Luiz Capitulino 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 >>>> --- >>>> 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 { >>> >> > >