| Message ID | b1cb4492a8688f295a7c8eaddd06a53dd3cae664.1329758006.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Regards, Anthony Liguori > > 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 {
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 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 { >
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 > > > >>> >>> 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 { >> >
On 02/22/2012 10:35 AM, Anthony Liguori wrote: >> [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"....}] } } Does that mean we want it to look more like: { 'command': 'blockdev-group-snapshot-sync', 'data': { 'devlist': [ 'SnapshotDev' ] } } and used like: { "execute": "blockdev-group-snapshot-sync", "arguments": { "devlist": [{ "device": "ide-hd0" ...}, { "device": "ide-hd1" ...}] } } I don't quite care what syntax we use, as long as we settle on something soon, because I'm trying to code up libvirt to generate this new monitor command, and need to know what syntax it will use.
On 02/22/2012 11:47 AM, Eric Blake wrote: > On 02/22/2012 10:35 AM, Anthony Liguori wrote: > >>> [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"....}] } } > > Does that mean we want it to look more like: > > { 'command': 'blockdev-group-snapshot-sync', > 'data': { 'devlist': [ 'SnapshotDev' ] } } > > and used like: > > { "execute": "blockdev-group-snapshot-sync", > "arguments": { "devlist": [{ "device": "ide-hd0" ...}, > { "device": "ide-hd1" ...}] } } Yes, devlist is a better variable name. > > I don't quite care what syntax we use, as long as we settle on something > soon, because I'm trying to code up libvirt to generate this new monitor > command, and need to know what syntax it will use. Just FYI, I've looked at the QAPI bits so far. I would suggest getting an Ack from Kevin at least that this API is reasonable inline with what we're looking for here before you start doing libvirt support. Personally, I loathe the idea of extending an already broken interface... I would rather we fix the original interface first before tacking on additional features. Regards, Anthony Liguori
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 { >>> >> > >
On 02/22/2012 12:56 PM, Anthony Liguori wrote: > On 02/22/2012 11:47 AM, Eric Blake wrote: >> On 02/22/2012 10:35 AM, Anthony Liguori wrote: >> >>>> [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"....}] } } >> >> Does that mean we want it to look more like: >> >> { 'command': 'blockdev-group-snapshot-sync', >> 'data': { 'devlist': [ 'SnapshotDev' ] } } >> >> and used like: >> >> { "execute": "blockdev-group-snapshot-sync", >> "arguments": { "devlist": [{ "device": "ide-hd0" ...}, >> { "device": "ide-hd1" ...}] } } > > > Yes, devlist is a better variable name. > I will change it to devlist, for the v2 patch submission. >> >> I don't quite care what syntax we use, as long as we settle on something >> soon, because I'm trying to code up libvirt to generate this new monitor >> command, and need to know what syntax it will use. > > > Just FYI, I've looked at the QAPI bits so far. I would suggest getting > an Ack from Kevin at least that this API is reasonable inline with what > we're looking for here before you start doing libvirt support. > > Personally, I loathe the idea of extending an already broken > interface... I would rather we fix the original interface first before > tacking on additional features. > > Regards, > > Anthony Liguori > >
On Wed, 22 Feb 2012 11:35:26 -0600 Anthony Liguori <anthony@codemonkey.ws> 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? I don't think the code can just be dropped (if that's what you meant), as there are some features on top of it, like auto-completion, suffix support (G, M, k, etc), but it certainly can be improved. Also note that QMP does argument validation too, which is unfortunate enough to be based on HMP's, and it probably has to be adapted too (actually, maybe you can use the O type for lists...).
On 02/22/2012 02:25 PM, Luiz Capitulino wrote: > On Wed, 22 Feb 2012 11:35:26 -0600 > Anthony Liguori<anthony@codemonkey.ws> 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? > > I don't think the code can just be dropped (if that's what you meant), as there > are some features on top of it, like auto-completion, suffix support (G, M, k, etc), > but it certainly can be improved. > > Also note that QMP does argument validation too, which is unfortunate enough to > be based on HMP's, Once everything is converted to middle mode, this validation can be dropped for QMP. The generated code should do thorough checking of arguments and can handle complex types. Regards, Anthony Liguori
On Wed, 22 Feb 2012 14:31:17 -0600 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 02/22/2012 02:25 PM, Luiz Capitulino wrote: > > On Wed, 22 Feb 2012 11:35:26 -0600 > > Anthony Liguori<anthony@codemonkey.ws> 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? > > > > I don't think the code can just be dropped (if that's what you meant), as there > > are some features on top of it, like auto-completion, suffix support (G, M, k, etc), > > but it certainly can be improved. > > > > Also note that QMP does argument validation too, which is unfortunate enough to > > be based on HMP's, > > Once everything is converted to middle mode, this validation can be dropped for > QMP. The generated code should do thorough checking of arguments and can handle > complex types. That's cool, although we'll be switching to the new server too (hence the whole current server will be dropped).
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 {
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. 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(-)