| Message ID | 1435782155-31412-41-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/01/2015 02:22 PM, Markus Armbruster wrote: > It's first class, because unlike '**', it actually works, i.e. doesn't > require 'gen': false. Generated code grows in order to accommodate visiting the new anyList type: qapi-types.c | 15 +++++++++++++++ qapi-types.h | 13 +++++++++++++ qapi-visit.c | 24 ++++++++++++++++++++++++ qapi-visit.h | 1 + qga/qapi-generated/qga-qapi-types.h | 13 +++++++++++++ qga/qapi-generated/qga-qapi-visit.h | 1 + 6 files changed, 67 insertions(+) But do we really need anyList as a representation of qapi ['any'] ? Or will a later pass that minimizes array creation only to places where it is needed help? > > '**' will go away next. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > docs/qapi-code-gen.txt | 1 + > include/qapi/visitor-impl.h | 2 ++ > include/qapi/visitor.h | 1 + > qapi/qapi-dealloc-visitor.c | 9 +++++++ > qapi/qapi-visit-core.c | 6 +++++ > qapi/qmp-input-visitor.c | 11 ++++++++ > qapi/qmp-output-visitor.c | 9 +++++++ > scripts/qapi-types.py | 1 + > scripts/qapi.py | 9 ++++--- > tests/qapi-schema/type-bypass.out | 4 +-- > tests/test-qmp-input-visitor.c | 45 +++++++++++++++++++++++++++++++++ > tests/test-qmp-output-visitor.c | 53 +++++++++++++++++++++++++++++++++++++++ > 12 files changed, 146 insertions(+), 5 deletions(-) Touches a bit more this time, but that's okay. > > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index cb0fe75..bf9e854 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -158,6 +158,7 @@ The following types are predefined, and map to C as follows: > size uint64_t like uint64_t, except StringInputVisitor > accepts size suffixes > bool bool JSON true or false > + any QObject * any JSON value And with this, an 'alternate' type is just a special subset of 'any' that limits the set of valid JSON values according to the qapi description. > +++ b/qapi/qapi-visit-core.c > @@ -260,6 +260,12 @@ void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp) > v->type_number(v, obj, name, errp); > } > > +void visit_type_any(Visitor *v, QObject **obj, const char *name, > + Error **errp) Indentation looks off. > @@ -1048,8 +1048,9 @@ class QAPISchema(object): > ('uint64', 'int', 'uint64_t', '0'), > ('size', 'int', 'uint64_t', '0'), > ('bool', 'boolean', 'bool', 'false'), > - ('**', 'value', None, None)]: > + ('any', 'value', 'QObject' + pointer_suffix , 'NULL')]: > self._def_builtin_type(*t) > + self.entity_dict['**'] = self.lookup_type('any') # TODO drop this alias > > def _make_implicit_enum_type(self, name, values): > name = name + 'Kind' > @@ -1190,6 +1191,8 @@ class QAPISchema(object): > def visit(self, visitor): > visitor.visit_begin() > for name in sorted(self.entity_dict.keys()): > + if self.entity_dict[name].name != name: > + continue # ignore alias TODO drop alias and remove Nice back-compat hacks while you transition into using it. > +++ b/tests/test-qmp-input-visitor.c > @@ -298,6 +298,49 @@ static void test_visitor_in_list(TestInputVisitorData *data, > qapi_free_UserDefOneList(head); > } > > +static void test_visitor_in_any(TestInputVisitorData *data, > + const void *unused) > +{ > + > + v = visitor_input_test_init(data, "-42"); So we prove it accepts ints,... > + v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }"); objects,... > + visit_type_any(v, &res, NULL, &err); > + g_assert(!err); > + qdict = qobject_to_qdict(res); > + g_assert(qdict); worth asserting the dictionary has 3 keys? > +} ...but no test of string, boolean, or array? > +++ b/tests/test-qmp-output-visitor.c > @@ -428,6 +428,57 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data, > qapi_free_UserDefTwoList(head); > } > > +static void test_visitor_out_any(TestOutputVisitorData *data, > + const void *unused) > +{ > +} Same tests of 'int' and 'object' but not of other JSON types. Incomplete tests are still better than no tests, so what you have is a good start. Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> It's first class, because unlike '**', it actually works, i.e. doesn't >> require 'gen': false. > > Generated code grows in order to accommodate visiting the new anyList type: > > qapi-types.c | 15 +++++++++++++++ > qapi-types.h | 13 +++++++++++++ > qapi-visit.c | 24 ++++++++++++++++++++++++ > qapi-visit.h | 1 + > qga/qapi-generated/qga-qapi-types.h | 13 +++++++++++++ > qga/qapi-generated/qga-qapi-visit.h | 1 + > 6 files changed, 67 insertions(+) > > But do we really need anyList as a representation of qapi ['any'] ? Or > will a later pass that minimizes array creation only to places where it > is needed help? Like most generated list types, anyList isn't used. I didn't feel like making it a special case. If the unused list types bother us, we can generate only the ones actually mentioned in the schema. May require mentioning a few we use internally just to have them generated. >> '**' will go away next. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> docs/qapi-code-gen.txt | 1 + >> include/qapi/visitor-impl.h | 2 ++ >> include/qapi/visitor.h | 1 + >> qapi/qapi-dealloc-visitor.c | 9 +++++++ >> qapi/qapi-visit-core.c | 6 +++++ >> qapi/qmp-input-visitor.c | 11 ++++++++ >> qapi/qmp-output-visitor.c | 9 +++++++ >> scripts/qapi-types.py | 1 + >> scripts/qapi.py | 9 ++++--- >> tests/qapi-schema/type-bypass.out | 4 +-- >> tests/test-qmp-input-visitor.c | 45 +++++++++++++++++++++++++++++++++ >> tests/test-qmp-output-visitor.c | 53 >> +++++++++++++++++++++++++++++++++++++++ >> 12 files changed, 146 insertions(+), 5 deletions(-) > > Touches a bit more this time, but that's okay. > >> >> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt >> index cb0fe75..bf9e854 100644 >> --- a/docs/qapi-code-gen.txt >> +++ b/docs/qapi-code-gen.txt >> @@ -158,6 +158,7 @@ The following types are predefined, and map to C as follows: >> size uint64_t like uint64_t, except StringInputVisitor >> accepts size suffixes >> bool bool JSON true or false >> + any QObject * any JSON value > > And with this, an 'alternate' type is just a special subset of 'any' > that limits the set of valid JSON values according to the qapi description. > >> +++ b/qapi/qapi-visit-core.c >> @@ -260,6 +260,12 @@ void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp) >> v->type_number(v, obj, name, errp); >> } >> >> +void visit_type_any(Visitor *v, QObject **obj, const char *name, >> + Error **errp) > > Indentation looks off. Will fix. >> @@ -1048,8 +1048,9 @@ class QAPISchema(object): >> ('uint64', 'int', 'uint64_t', '0'), >> ('size', 'int', 'uint64_t', '0'), >> ('bool', 'boolean', 'bool', 'false'), >> - ('**', 'value', None, None)]: >> + ('any', 'value', 'QObject' + pointer_suffix , 'NULL')]: >> self._def_builtin_type(*t) >> + self.entity_dict['**'] = self.lookup_type('any') # TODO drop this alias >> >> def _make_implicit_enum_type(self, name, values): >> name = name + 'Kind' >> @@ -1190,6 +1191,8 @@ class QAPISchema(object): >> def visit(self, visitor): >> visitor.visit_begin() >> for name in sorted(self.entity_dict.keys()): >> + if self.entity_dict[name].name != name: >> + continue # ignore alias TODO drop alias and remove > > Nice back-compat hacks while you transition into using it. > >> +++ b/tests/test-qmp-input-visitor.c >> @@ -298,6 +298,49 @@ static void test_visitor_in_list(TestInputVisitorData *data, >> qapi_free_UserDefOneList(head); >> } >> >> +static void test_visitor_in_any(TestInputVisitorData *data, >> + const void *unused) >> +{ > >> + >> + v = visitor_input_test_init(data, "-42"); > > So we prove it accepts ints,... > >> + v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': >> true, 'string': 'foo' }"); > > objects,... > >> + visit_type_any(v, &res, NULL, &err); >> + g_assert(!err); >> + qdict = qobject_to_qdict(res); >> + g_assert(qdict); > > worth asserting the dictionary has 3 keys? Can't hurt. >> +} > > ...but no test of string, boolean, or array? I cut corners to get the thing out: one scalar and one non-scalar type. We can always add more. >> +++ b/tests/test-qmp-output-visitor.c >> @@ -428,6 +428,57 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data, >> qapi_free_UserDefTwoList(head); >> } >> >> +static void test_visitor_out_any(TestOutputVisitorData *data, >> + const void *unused) >> +{ > >> +} > > Same tests of 'int' and 'object' but not of other JSON types. > > Incomplete tests are still better than no tests, so what you have is a > good start. > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index cb0fe75..bf9e854 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -158,6 +158,7 @@ The following types are predefined, and map to C as follows: size uint64_t like uint64_t, except StringInputVisitor accepts size suffixes bool bool JSON true or false + any QObject * any JSON value === Includes === diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index f4a2f74..8c0ba57 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -40,6 +40,8 @@ struct Visitor void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp); void (*type_number)(Visitor *v, double *obj, const char *name, Error **errp); + void (*type_any)(Visitor *v, QObject **obj, const char *name, + Error **errp); /* May be NULL */ void (*optional)(Visitor *v, bool *present, const char *name, diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 00ba104..cfc19a6 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -58,6 +58,7 @@ void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp); void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp); void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp); void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp); +void visit_type_any(Visitor *v, QObject **obj, const char *name, Error **errp); bool visit_start_union(Visitor *v, bool data_present, Error **errp); void visit_end_union(Visitor *v, bool data_present, Error **errp); diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index d7f92c5..737deab 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -151,6 +151,14 @@ static void qapi_dealloc_type_number(Visitor *v, double *obj, const char *name, { } +static void qapi_dealloc_type_anything(Visitor *v, QObject **obj, + const char *name, Error **errp) +{ + if (obj) { + qobject_decref(*obj); + } +} + static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp) { @@ -216,6 +224,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) v->visitor.type_bool = qapi_dealloc_type_bool; v->visitor.type_str = qapi_dealloc_type_str; v->visitor.type_number = qapi_dealloc_type_number; + v->visitor.type_any = qapi_dealloc_type_anything; v->visitor.type_size = qapi_dealloc_type_size; v->visitor.start_union = qapi_dealloc_start_union; diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 5a7c900..230d4b2 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -260,6 +260,12 @@ void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp) v->type_number(v, obj, name, errp); } +void visit_type_any(Visitor *v, QObject **obj, const char *name, + Error **errp) +{ + v->type_any(v, obj, name, errp); +} + void output_type_enum(Visitor *v, int *obj, const char * const strings[], const char *kind, const char *name, Error **errp) diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index e97b8a4..5dd9ed5 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -286,6 +286,16 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name, } } +static void qmp_input_type_any(Visitor *v, QObject **obj, const char *name, + Error **errp) +{ + QmpInputVisitor *qiv = to_qiv(v); + QObject *qobj = qmp_input_get_object(qiv, name, true); + + qobject_incref(qobj); + *obj = qobj; +} + static void qmp_input_optional(Visitor *v, bool *present, const char *name, Error **errp) { @@ -329,6 +339,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) v->visitor.type_bool = qmp_input_type_bool; v->visitor.type_str = qmp_input_type_str; v->visitor.type_number = qmp_input_type_number; + v->visitor.type_any = qmp_input_type_any; v->visitor.optional = qmp_input_optional; v->visitor.get_next_type = qmp_input_get_next_type; diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index efc19d5..1e36a7a 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -186,6 +186,14 @@ static void qmp_output_type_number(Visitor *v, double *obj, const char *name, qmp_output_add(qov, name, qfloat_from_double(*obj)); } +static void qmp_output_type_any(Visitor *v, QObject **obj, const char *name, + Error **errp) +{ + QmpOutputVisitor *qov = to_qov(v); + qobject_incref(*obj); + qmp_output_add_obj(qov, name, *obj); +} + QObject *qmp_output_get_qobject(QmpOutputVisitor *qov) { QObject *obj = qmp_output_first(qov); @@ -233,6 +241,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void) v->visitor.type_bool = qmp_output_type_bool; v->visitor.type_str = qmp_output_type_str; v->visitor.type_number = qmp_output_type_number; + v->visitor.type_any = qmp_output_type_any; QTAILQ_INIT(&v->stack); diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index b6990a5..7c403c0 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -289,6 +289,7 @@ fdef.write(mcgen(''' fdecl.write(mcgen(''' #include <stdbool.h> #include <stdint.h> +#include "qapi/qmp/qobject.h" ''')) schema = QAPISchema(input_file) diff --git a/scripts/qapi.py b/scripts/qapi.py index c6a5ddc..2ff087f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -33,6 +33,7 @@ builtin_types = { 'uint32': 'QTYPE_QINT', 'uint64': 'QTYPE_QINT', 'size': 'QTYPE_QINT', + 'any': None, # any qtype_code possible, actually } # Whitelist of commands allowed to return a non-dictionary @@ -1031,8 +1032,7 @@ class QAPISchema(object): def _def_builtin_type(self, name, json_type, c_type, c_null): self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type, c_null)) - if name != '**': - self._make_array_type(name) # TODO really needed? + self._make_array_type(name) # TODO really needed? def _def_predefineds(self): for t in [('str', 'string', 'char' + pointer_suffix, 'NULL'), @@ -1048,8 +1048,9 @@ class QAPISchema(object): ('uint64', 'int', 'uint64_t', '0'), ('size', 'int', 'uint64_t', '0'), ('bool', 'boolean', 'bool', 'false'), - ('**', 'value', None, None)]: + ('any', 'value', 'QObject' + pointer_suffix , 'NULL')]: self._def_builtin_type(*t) + self.entity_dict['**'] = self.lookup_type('any') # TODO drop this alias def _make_implicit_enum_type(self, name, values): name = name + 'Kind' @@ -1190,6 +1191,8 @@ class QAPISchema(object): def visit(self, visitor): visitor.visit_begin() for name in sorted(self.entity_dict.keys()): + if self.entity_dict[name].name != name: + continue # ignore alias TODO drop alias and remove self.entity_dict[name].visit(visitor) visitor.visit_end() diff --git a/tests/qapi-schema/type-bypass.out b/tests/qapi-schema/type-bypass.out index b147864..2dc0c09 100644 --- a/tests/qapi-schema/type-bypass.out +++ b/tests/qapi-schema/type-bypass.out @@ -1,4 +1,4 @@ object :obj-unsafe-args - member arg: ** optional=False -command unsafe :obj-unsafe-args -> ** + member arg: any optional=False +command unsafe :obj-unsafe-args -> any gen=False success_response=True diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 56da3c6..9c4bfdf 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -298,6 +298,49 @@ static void test_visitor_in_list(TestInputVisitorData *data, qapi_free_UserDefOneList(head); } +static void test_visitor_in_any(TestInputVisitorData *data, + const void *unused) +{ + QObject *res = NULL; + Error *err = NULL; + Visitor *v; + QInt *qint; + QBool *qbool; + QString *qstring; + QDict *qdict; + QObject *qobj; + + v = visitor_input_test_init(data, "-42"); + visit_type_any(v, &res, NULL, &err); + g_assert(!err); + qint = qobject_to_qint(res); + g_assert(qint); + g_assert_cmpint(qint_get_int(qint), ==, -42); + qobject_decref(res); + + v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }"); + visit_type_any(v, &res, NULL, &err); + g_assert(!err); + qdict = qobject_to_qdict(res); + g_assert(qdict); + qobj = qdict_get(qdict, "integer"); + g_assert(qobj); + qint = qobject_to_qint(qobj); + g_assert(qint); + g_assert_cmpint(qint_get_int(qint), ==, -42); + qobj = qdict_get(qdict, "boolean"); + g_assert(qobj); + qbool = qobject_to_qbool(qobj); + g_assert(qbool); + g_assert(qbool_get_bool(qbool) == true); + qobj = qdict_get(qdict, "string"); + g_assert(qobj); + qstring = qobject_to_qstring(qobj); + g_assert(qstring); + g_assert_cmpstr(qstring_get_str(qstring), ==, "foo"); + qobject_decref(res); +} + static void test_visitor_in_union_flat(TestInputVisitorData *data, const void *unused) { @@ -667,6 +710,8 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_struct_nested); input_visitor_test_add("/visitor/input/list", &in_visitor_data, test_visitor_in_list); + input_visitor_test_add("/visitor/input/any", + &in_visitor_data, test_visitor_in_any); input_visitor_test_add("/visitor/input/union-flat", &in_visitor_data, test_visitor_in_union_flat); input_visitor_test_add("/visitor/input/alternate", diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 338ada0..1a28dc2 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -428,6 +428,57 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data, qapi_free_UserDefTwoList(head); } +static void test_visitor_out_any(TestOutputVisitorData *data, + const void *unused) +{ + QObject *qobj; + Error *err = NULL; + QInt *qint; + QBool *qbool; + QString *qstring; + QDict *qdict; + QObject *obj; + + qobj = QOBJECT(qint_from_int(-42)); + visit_type_any(data->ov, &qobj, NULL, &err); + g_assert(!err); + obj = qmp_output_get_qobject(data->qov); + g_assert(obj != NULL); + g_assert(qobject_type(obj) == QTYPE_QINT); + g_assert_cmpint(qint_get_int(qobject_to_qint(obj)), ==, -42); + qobject_decref(obj); + qobject_decref(qobj); + + qdict = qdict_new(); + qdict_put(qdict, "integer", qint_from_int(-42)); + qdict_put(qdict, "boolean", qbool_from_bool(true)); + qdict_put(qdict, "string", qstring_from_str("foo")); + qobj = QOBJECT(qdict); + visit_type_any(data->ov, &qobj, NULL, &err); + g_assert(!err); + obj = qmp_output_get_qobject(data->qov); + g_assert(obj != NULL); + qdict = qobject_to_qdict(obj); + g_assert(qdict); + qobj = qdict_get(qdict, "integer"); + g_assert(qobj); + qint = qobject_to_qint(qobj); + g_assert(qint); + g_assert_cmpint(qint_get_int(qint), ==, -42); + qobj = qdict_get(qdict, "boolean"); + g_assert(qobj); + qbool = qobject_to_qbool(qobj); + g_assert(qbool); + g_assert(qbool_get_bool(qbool) == true); + qobj = qdict_get(qdict, "string"); + g_assert(qobj); + qstring = qobject_to_qstring(qobj); + g_assert(qstring); + g_assert_cmpstr(qstring_get_str(qstring), ==, "foo"); + qobject_decref(obj); + qobject_decref(qobj); +} + static void test_visitor_out_union_flat(TestOutputVisitorData *data, const void *unused) { @@ -832,6 +883,8 @@ int main(int argc, char **argv) &out_visitor_data, test_visitor_out_struct_errors); output_visitor_test_add("/visitor/output/list", &out_visitor_data, test_visitor_out_list); + output_visitor_test_add("/visitor/output/any", + &out_visitor_data, test_visitor_out_any); output_visitor_test_add("/visitor/output/list-qapi-free", &out_visitor_data, test_visitor_out_list_qapi_free); output_visitor_test_add("/visitor/output/union-flat",
It's first class, because unlike '**', it actually works, i.e. doesn't require 'gen': false. '**' will go away next. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/qapi-code-gen.txt | 1 + include/qapi/visitor-impl.h | 2 ++ include/qapi/visitor.h | 1 + qapi/qapi-dealloc-visitor.c | 9 +++++++ qapi/qapi-visit-core.c | 6 +++++ qapi/qmp-input-visitor.c | 11 ++++++++ qapi/qmp-output-visitor.c | 9 +++++++ scripts/qapi-types.py | 1 + scripts/qapi.py | 9 ++++--- tests/qapi-schema/type-bypass.out | 4 +-- tests/test-qmp-input-visitor.c | 45 +++++++++++++++++++++++++++++++++ tests/test-qmp-output-visitor.c | 53 +++++++++++++++++++++++++++++++++++++++ 12 files changed, 146 insertions(+), 5 deletions(-)