| Message ID | 1435782155-31412-42-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/01/2015 02:22 PM, Markus Armbruster wrote: > With the previous commit, the generated marshalers just work, and save > us a bit of handwritten code. > Generated code grows, because you are now generating the hook :) qmp-commands.h | 6 ++ qmp-marshal.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > include/monitor/monitor.h | 3 --- > qapi-schema.json | 9 +++------ > qmp-commands.hx | 6 +++--- > qmp.c | 20 +++++++------------- > scripts/qapi.py | 1 + > 5 files changed, 14 insertions(+), 25 deletions(-) > > +++ b/qmp.c > @@ -229,11 +229,9 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp) > } > > /* FIXME: teach qapi about how to pass through Visitors */ > -void qmp_qom_set(QDict *qdict, QObject **ret, Error **errp) > +void qmp_qom_set(const char *path, const char *property, QObject *value, > + Error **errp) The FIXME seems stale now. > > -void qmp_object_add(QDict *qdict, QObject **ret, Error **errp) > +void qmp_object_add(const char *type, const char *id, > + bool has_props, QObject *props, Error **errp) > { > - const char *type = qdict_get_str(qdict, "qom-type"); > - const char *id = qdict_get_str(qdict, "id"); > - QObject *props = qdict_get(qdict, "props"); > const QDict *pdict = NULL; > QmpInputVisitor *qiv; > Continuing: if (props) { pdict = qobject_to_qdict(props); if (!pdict) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); return; } } I know that we guarantee that all pointers are initialized to NULL in our marshaler, so this is correct; but wouldn't it be more idiomatic to write 'if (has_props)' as the condition? (And it would make a nice followup project for someone to figure out how to get rid of have_FOO arguments for strings and objects where NULL is a nice witness; it is only integers and booleans that require them - but doing that will touch a lot of the tree, and is a series of its own) What I pointed out is minor, so you can fix it and add: Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> With the previous commit, the generated marshalers just work, and save >> us a bit of handwritten code. >> > > Generated code grows, because you are now generating the hook :) > > qmp-commands.h | 6 ++ > qmp-marshal.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 150 insertions(+) > > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> include/monitor/monitor.h | 3 --- >> qapi-schema.json | 9 +++------ >> qmp-commands.hx | 6 +++--- >> qmp.c | 20 +++++++------------- >> scripts/qapi.py | 1 + >> 5 files changed, 14 insertions(+), 25 deletions(-) >> > >> +++ b/qmp.c >> @@ -229,11 +229,9 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp) >> } >> >> /* FIXME: teach qapi about how to pass through Visitors */ >> -void qmp_qom_set(QDict *qdict, QObject **ret, Error **errp) >> +void qmp_qom_set(const char *path, const char *property, QObject *value, >> + Error **errp) > > The FIXME seems stale now. I'm not 100% sure I understand the intent of the FIXME, but my best guess is "do what this patch does". I'll drop it. >> -void qmp_object_add(QDict *qdict, QObject **ret, Error **errp) >> +void qmp_object_add(const char *type, const char *id, >> + bool has_props, QObject *props, Error **errp) >> { >> - const char *type = qdict_get_str(qdict, "qom-type"); >> - const char *id = qdict_get_str(qdict, "id"); >> - QObject *props = qdict_get(qdict, "props"); >> const QDict *pdict = NULL; >> QmpInputVisitor *qiv; >> > > Continuing: > > if (props) { > pdict = qobject_to_qdict(props); > if (!pdict) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); > return; > } > } > > I know that we guarantee that all pointers are initialized to NULL in > our marshaler, so this is correct; but wouldn't it be more idiomatic to > write 'if (has_props)' as the condition? We do similar things elsewhere. Moreover: > (And it would make a nice followup project for someone to figure out how > to get rid of have_FOO arguments for strings and objects where NULL is a > nice witness; it is only integers and booleans that require them - but > doing that will touch a lot of the tree, and is a series of its own) Shouldn't be hard, and I really want it done. Will change all the places that test has_PTR to just PTR. More reason to simply test PTR now. > What I pointed out is minor, so you can fix it and add: > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 9aff47e..bc6cb6d 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -42,9 +42,6 @@ void monitor_read_command(Monitor *mon, int show_prompt); int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, void *opaque); -void qmp_qom_set(QDict *qdict, QObject **ret, Error **errp); -void qmp_qom_get(QDict *qdict, QObject **ret, Error **errp); -void qmp_object_add(QDict *qdict, QObject **ret, Error **errp); void object_add(const char *type, const char *id, const QDict *qdict, Visitor *v, Error **errp); diff --git a/qapi-schema.json b/qapi-schema.json index 106008c..815689a 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1695,8 +1695,7 @@ ## { 'command': 'qom-get', 'data': { 'path': 'str', 'property': 'str' }, - 'returns': '**', - 'gen': false } + 'returns': 'any' } ## # @qom-set: @@ -1713,8 +1712,7 @@ # Since: 1.2 ## { 'command': 'qom-set', - 'data': { 'path': 'str', 'property': 'str', 'value': '**' }, - 'gen': false } + 'data': { 'path': 'str', 'property': 'str', 'value': 'any' } } ## # @set_password: @@ -2110,8 +2108,7 @@ # Since: 2.0 ## { 'command': 'object-add', - 'data': {'qom-type': 'str', 'id': 'str', '*props': '**'}, - 'gen': false } + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} } ## # @object-del: diff --git a/qmp-commands.hx b/qmp-commands.hx index 6760ddb..874da31 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -928,7 +928,7 @@ EQMP { .name = "object-add", .args_type = "qom-type:s,id:s,props:q?", - .mhandler.cmd_new = qmp_object_add, + .mhandler.cmd_new = qmp_marshal_object_add, }, SQMP @@ -3546,13 +3546,13 @@ EQMP { .name = "qom-set", .args_type = "path:s,property:s,value:q", - .mhandler.cmd_new = qmp_qom_set, + .mhandler.cmd_new = qmp_marshal_qom_set, }, { .name = "qom-get", .args_type = "path:s,property:s", - .mhandler.cmd_new = qmp_qom_get, + .mhandler.cmd_new = qmp_marshal_qom_get, }, { diff --git a/qmp.c b/qmp.c index 403805a..564a414 100644 --- a/qmp.c +++ b/qmp.c @@ -229,11 +229,9 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp) } /* FIXME: teach qapi about how to pass through Visitors */ -void qmp_qom_set(QDict *qdict, QObject **ret, Error **errp) +void qmp_qom_set(const char *path, const char *property, QObject *value, + Error **errp) { - const char *path = qdict_get_str(qdict, "path"); - const char *property = qdict_get_str(qdict, "property"); - QObject *value = qdict_get(qdict, "value"); Object *obj; obj = object_resolve_path(path, NULL); @@ -246,20 +244,18 @@ void qmp_qom_set(QDict *qdict, QObject **ret, Error **errp) object_property_set_qobject(obj, value, property, errp); } -void qmp_qom_get(QDict *qdict, QObject **ret, Error **errp) +QObject *qmp_qom_get(const char *path, const char *property, Error **errp) { - const char *path = qdict_get_str(qdict, "path"); - const char *property = qdict_get_str(qdict, "property"); Object *obj; obj = object_resolve_path(path, NULL); if (!obj) { error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", path); - return; + return NULL; } - *ret = object_property_get_qobject(obj, property, errp); + return object_property_get_qobject(obj, property, errp); } void qmp_set_password(const char *protocol, const char *password, @@ -655,11 +651,9 @@ out: object_unref(obj); } -void qmp_object_add(QDict *qdict, QObject **ret, Error **errp) +void qmp_object_add(const char *type, const char *id, + bool has_props, QObject *props, Error **errp) { - const char *type = qdict_get_str(qdict, "qom-type"); - const char *id = qdict_get_str(qdict, "id"); - QObject *props = qdict_get(qdict, "props"); const QDict *pdict = NULL; QmpInputVisitor *qiv; diff --git a/scripts/qapi.py b/scripts/qapi.py index 2ff087f..df63130 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -40,6 +40,7 @@ builtin_types = { returns_whitelist = [ # From QMP: 'human-monitor-command', + 'qom-get', 'query-migrate-cache-size', 'query-tpm-models', 'query-tpm-types',
With the previous commit, the generated marshalers just work, and save us a bit of handwritten code. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/monitor/monitor.h | 3 --- qapi-schema.json | 9 +++------ qmp-commands.hx | 6 +++--- qmp.c | 20 +++++++------------- scripts/qapi.py | 1 + 5 files changed, 14 insertions(+), 25 deletions(-)