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

[Jeff Cody <jcody@redhat.com>]

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<jcody@redhat.com>
>>>> ---
>>>> 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 {
>>>
>>
>
>