* [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types
@ 2013-05-10 2:20 Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support Michael Roth
` (10 more replies)
0 siblings, 11 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
These patches apply on top of qemu.git master, and can also be obtained from:
git://github.com/mdroth/qemu.git qapi-native-lists
Sending this now since a number of series have popped up in the past that
wanted this, and Amos has some pending patches (query-mac-tables) that rely
on this as well.
These patches add support for specifying lists of native qapi types
(int/bool/str/number) like so:
{ 'type': 'foo',
'data': { 'bar': ['int'] }}
for a 'bar' field that is a list of type 'int',
{ 'type': 'foo2',
'data': { 'bar2': ['str'] }}
for a 'bar2' field that is a list of type 'str', and so on.
This uses linked list types for the native C representations, just as we do
for complex schema-defined types. In the future we may add schema annotations
of some sort to specify a more natural/efficient array type for the C
representations, but this should serve the majority of uses-cases for now.
v1->v2:
* fixed do-nothing float tests in pre-existing code and updated new
unit tests accordingly (Laszlo)
* added a fix for a bug in json parser that was exposed by above change
* fixed misuse of string format parameters for float testing (Laszlo)
* fixed extra characters in comment (Laszlo)
* removed unused variant from UserDefNativeListUnion QAPI type
Makefile | 6 +-
qapi-schema-test.json | 7 +
qobject/json-parser.c | 26 +++-
scripts/qapi-types.py | 43 +++++-
scripts/qapi-visit.py | 36 ++++-
scripts/qapi.py | 21 +++
tests/test-qmp-input-visitor.c | 186 ++++++++++++++++++++++++
tests/test-qmp-output-visitor.c | 176 ++++++++++++++++++++++
tests/test-visitor-serialization.c | 282 +++++++++++++++++++++++++++++++-----
9 files changed, 731 insertions(+), 52 deletions(-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
@ 2013-05-10 2:20 ` Michael Roth
2013-05-10 3:04 ` Amos Kong
2013-05-10 14:07 ` Luiz Capitulino
2013-05-10 2:20 ` [Qemu-devel] [PATCH 02/10] qapi: qapi-visit.py, fix list handling for union types Michael Roth
` (9 subsequent siblings)
10 siblings, 2 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
Teach type generators about native types so they can generate the
appropriate linked list types.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
scripts/qapi-types.py | 43 ++++++++++++++++++++++++++++++++++++++++---
scripts/qapi.py | 21 +++++++++++++++++++++
2 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9e19920..96cb26d 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -16,7 +16,18 @@ import os
import getopt
import errno
-def generate_fwd_struct(name, members):
+def generate_fwd_struct(name, members, builtin_type=False):
+ if builtin_type:
+ return mcgen('''
+typedef struct %(name)sList
+{
+ %(type)s value;
+ struct %(name)sList *next;
+} %(name)sList;
+''',
+ type=c_type(name),
+ name=name)
+
return mcgen('''
typedef struct %(name)s %(name)s;
@@ -164,6 +175,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
def generate_type_cleanup(name):
ret = mcgen('''
+
void qapi_free_%(type)s(%(c_type)s obj)
{
QapiDeallocVisitor *md;
@@ -184,8 +196,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
try:
- opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
- ["source", "header", "prefix=", "output-dir="])
+ opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
+ ["source", "header", "builtins",
+ "prefix=", "output-dir="])
except getopt.GetoptError, err:
print str(err)
sys.exit(1)
@@ -197,6 +210,7 @@ h_file = 'qapi-types.h'
do_c = False
do_h = False
+do_builtins = False
for o, a in opts:
if o in ("-p", "--prefix"):
@@ -207,6 +221,8 @@ for o, a in opts:
do_c = True
elif o in ("-h", "--header"):
do_h = True
+ elif o in ("-b", "--builtins"):
+ do_builtins = True
if not do_c and not do_h:
do_c = True
@@ -282,6 +298,11 @@ fdecl.write(mcgen('''
exprs = parse_schema(sys.stdin)
exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
+fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
+for typename in builtin_types:
+ fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
+fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
+
for expr in exprs:
ret = "\n"
if expr.has_key('type'):
@@ -298,6 +319,22 @@ for expr in exprs:
continue
fdecl.write(ret)
+# to avoid header dependency hell, we always generate declarations
+# for built-in types in our header files and simply guard them
+fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
+for typename in builtin_types:
+ fdecl.write(generate_type_cleanup_decl(typename + "List"))
+fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
+
+# ...this doesn't work for cases where we link in multiple objects that
+# have the functions defined, so we use -b option to provide control
+# over these cases
+if do_builtins:
+ fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
+ for typename in builtin_types:
+ fdef.write(generate_type_cleanup(typename + "List"))
+ fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
+
for expr in exprs:
ret = "\n"
if expr.has_key('type'):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index afc5f32..0ac8c2b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -11,6 +11,10 @@
from ordereddict import OrderedDict
+builtin_types = [
+ 'str', 'int', 'number', 'bool'
+]
+
def tokenize(data):
while len(data):
ch = data[0]
@@ -242,3 +246,20 @@ def guardname(filename):
for substr in [".", " ", "-"]:
guard = guard.replace(substr, "_")
return guard.upper() + '_H'
+
+def guardstart(name):
+ return mcgen('''
+
+#ifndef %(name)s
+#define %(name)s
+
+''',
+ name=guardname(name))
+
+def guardend(name):
+ return mcgen('''
+
+#endif /* %(name)s */
+
+''',
+ name=guardname(name))
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 02/10] qapi: qapi-visit.py, fix list handling for union types
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support Michael Roth
@ 2013-05-10 2:20 ` Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 03/10] qapi: qapi-visit.py, native list support Michael Roth
` (8 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
Currently we assume non-list types when generating visitor routines for
union types. This is broken, since values like ['Type'] need to mapped
to 'TypeList'.
We already have a type_name() function to handle this that we use for
generating struct visitors, so use that here as well.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
scripts/qapi-visit.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a276540..4c4de4b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -174,7 +174,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
''',
abbrev = de_camel_case(name).upper(),
enum = c_fun(de_camel_case(key),False).upper(),
- c_type=members[key],
+ c_type=type_name(members[key]),
c_name=c_fun(key))
ret += mcgen('''
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 03/10] qapi: qapi-visit.py, native list support
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 02/10] qapi: qapi-visit.py, fix list handling for union types Michael Roth
@ 2013-05-10 2:20 ` Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code Michael Roth
` (7 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
Teach visitor generators about native types so they can generate the
appropriate visitor routines.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
scripts/qapi-visit.py | 34 +++++++++++++++++++++++++++++-----
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4c4de4b..6cac05a 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -202,12 +202,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
return ret
-def generate_declaration(name, members, genlist=True):
- ret = mcgen('''
+def generate_declaration(name, members, genlist=True, builtin_type=False):
+ ret = ""
+ if not builtin_type:
+ ret += mcgen('''
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
''',
- name=name)
+ name=name)
if genlist:
ret += mcgen('''
@@ -235,8 +237,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e
name=name)
try:
- opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
- ["source", "header", "prefix=", "output-dir="])
+ opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
+ ["source", "header", "builtins", "prefix=",
+ "output-dir="])
except getopt.GetoptError, err:
print str(err)
sys.exit(1)
@@ -248,6 +251,7 @@ h_file = 'qapi-visit.h'
do_c = False
do_h = False
+do_builtins = False
for o, a in opts:
if o in ("-p", "--prefix"):
@@ -258,6 +262,8 @@ for o, a in opts:
do_c = True
elif o in ("-h", "--header"):
do_h = True
+ elif o in ("-b", "--builtins"):
+ do_builtins = True
if not do_c and not do_h:
do_c = True
@@ -324,11 +330,29 @@ fdecl.write(mcgen('''
#include "qapi/visitor.h"
#include "%(prefix)sqapi-types.h"
+
''',
prefix=prefix, guard=guardname(h_file)))
exprs = parse_schema(sys.stdin)
+# to avoid header dependency hell, we always generate declarations
+# for built-in types in our header files and simply guard them
+fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
+for typename in builtin_types:
+ fdecl.write(generate_declaration(typename, None, genlist=True,
+ builtin_type=True))
+fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
+
+# ...this doesn't work for cases where we link in multiple objects that
+# have the functions defined, so we use -b option to provide control
+# over these cases
+if do_builtins:
+ fdef.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
+ for typename in builtin_types:
+ fdef.write(generate_visit_list(typename, None))
+ fdef.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
+
for expr in exprs:
if expr.has_key('type'):
ret = generate_visit_struct(expr['type'], expr['data'])
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (2 preceding siblings ...)
2013-05-10 2:20 ` [Qemu-devel] [PATCH 03/10] qapi: qapi-visit.py, native list support Michael Roth
@ 2013-05-10 2:20 ` Michael Roth
2013-05-10 14:10 ` Luiz Capitulino
2013-05-10 2:20 ` [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests Michael Roth
` (6 subsequent siblings)
10 siblings, 1 reply; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
qapi-types.c/qapi-visit.c
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 7dc0204..9695c9d 100644
--- a/Makefile
+++ b/Makefile
@@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
# Build libraries
libqemustub.a: $(stub-obj-y)
-libqemuutil.a: $(util-obj-y)
+libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
######################################################################
@@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
qapi-types.c qapi-types.h :\
$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
- $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." < $<, " GEN $@")
+ $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -b < $<, " GEN $@")
qapi-visit.c qapi-visit.h :\
$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
- $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." < $<, " GEN $@")
+ $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@")
qmp-commands.h qmp-marshal.c :\
$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@")
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (3 preceding siblings ...)
2013-05-10 2:20 ` [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code Michael Roth
@ 2013-05-10 2:20 ` Michael Roth
2013-05-10 15:14 ` Luiz Capitulino
2013-05-10 2:20 ` [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values Michael Roth
` (5 subsequent siblings)
10 siblings, 1 reply; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
qmp_output_get_qobject() increments the qobject's reference count. Since
we currently pass this straight into qobject_to_json() so we can feed
the data into a QMP input visitor, we never actually free the underlying
qobject when qmp_output_visitor_cleanup() is called. This causes leaks
on all of the QMP serialization tests.
Fix this by holding a pointer to the qobject and decref'ing it before
returning from qmp_deserialize().
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
tests/test-visitor-serialization.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index e84926f..8c8adac 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -657,11 +657,16 @@ static void qmp_deserialize(void **native_out, void *datap,
VisitorFunc visit, Error **errp)
{
QmpSerializeData *d = datap;
- QString *output_json = qobject_to_json(qmp_output_get_qobject(d->qov));
- QObject *obj = qobject_from_json(qstring_get_str(output_json));
+ QString *output_json;
+ QObject *obj_orig, *obj;
+
+ obj_orig = qmp_output_get_qobject(d->qov);
+ output_json = qobject_to_json(obj_orig);
+ obj = qobject_from_json(qstring_get_str(output_json));
QDECREF(output_json);
d->qiv = qmp_input_visitor_new(obj);
+ qobject_decref(obj_orig);
qobject_decref(obj);
visit(qmp_input_get_visitor(d->qiv), native_out, errp);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (4 preceding siblings ...)
2013-05-10 2:20 ` [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests Michael Roth
@ 2013-05-10 2:20 ` Michael Roth
2013-05-10 11:55 ` Laszlo Ersek
` (2 more replies)
2013-05-10 2:20 ` [Qemu-devel] [PATCH 07/10] qapi: fix visitor serialization tests for numbers/doubles Michael Roth
` (4 subsequent siblings)
10 siblings, 3 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
Currently our JSON parser assumes that numbers lacking a mantissa are
integers and attempts to store them as QInt/int64 values. This breaks in
the case where the number overflows/underflows int64 values (which is
still valid JSON)
Fix this by detecting such cases and using a QFloat to store the value
instead.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qobject/json-parser.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 05279c1..4d14e71 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
case JSON_STRING:
obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
break;
- case JSON_INTEGER:
- obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
- break;
+ case JSON_INTEGER: {
+ /* A possibility exists that this is a whole-valued float where the
+ * mantissa was left out due to being 0 (.0). It's not a big deal to
+ * treat these as ints in the parser, so long as users of the
+ * resulting QObject know to expect a QInt in place of a QFloat in
+ * cases like these.
+ *
+ * However, in some cases these values will overflow/underflow a
+ * QInt/int64 container, thus we should assume these are to be handled
+ * as QFloats/doubles rather than silently changing their values.
+ *
+ * strtoll() indicates these instances by setting errno to ERANGE
+ */
+ int64_t value;
+
+ errno = 0; /* strtoll doesn't set errno on success */
+ value = strtoll(token_get_value(token), NULL, 10);
+ if (errno != ERANGE) {
+ obj = QOBJECT(qint_from_int(value));
+ break;
+ }
+ /* fall through to JSON_FLOAT */
+ }
case JSON_FLOAT:
/* FIXME dependent on locale */
obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), NULL)));
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 07/10] qapi: fix visitor serialization tests for numbers/doubles
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (5 preceding siblings ...)
2013-05-10 2:20 ` [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values Michael Roth
@ 2013-05-10 2:20 ` Michael Roth
2013-05-10 2:21 ` [Qemu-devel] [PATCH 08/10] qapi: add native list coverage for visitor serialization tests Michael Roth
` (3 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
We never actually stored the stringified double values into the strings
before we did the comparisons. This left number/double values completely
uncovered in test-visitor-serialization tests.
Fixing this exposed a bug in our handling of large whole number values
in QEMU's JSON parser which is now fixed.
Simplify the code while we're at it by dropping the
calc_float_string_storage() craziness in favor of GStrings.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
tests/test-visitor-serialization.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 8c8adac..fed6810 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -229,17 +229,6 @@ typedef struct TestArgs {
void *test_data;
} TestArgs;
-#define FLOAT_STRING_PRECISION 6 /* corresponding to n in %.nf formatting */
-static gsize calc_float_string_storage(double value)
-{
- int whole_value = value;
- gsize i = 0;
- do {
- i++;
- } while (whole_value /= 10);
- return i + 2 + FLOAT_STRING_PRECISION;
-}
-
static void test_primitives(gconstpointer opaque)
{
TestArgs *args = (TestArgs *) opaque;
@@ -248,7 +237,6 @@ static void test_primitives(gconstpointer opaque)
PrimitiveType *pt_copy = g_malloc0(sizeof(*pt_copy));
Error *err = NULL;
void *serialize_data;
- char *double1, *double2;
pt_copy->type = pt->type;
ops->serialize(pt, &serialize_data, visit_primitive_type, &err);
@@ -260,14 +248,17 @@ static void test_primitives(gconstpointer opaque)
g_assert_cmpstr(pt->value.string, ==, pt_copy->value.string);
g_free((char *)pt_copy->value.string);
} else if (pt->type == PTYPE_NUMBER) {
+ GString *double_expected = g_string_new("");
+ GString *double_actual = g_string_new("");
/* we serialize with %f for our reference visitors, so rather than fuzzy
* floating math to test "equality", just compare the formatted values
*/
- double1 = g_malloc0(calc_float_string_storage(pt->value.number));
- double2 = g_malloc0(calc_float_string_storage(pt_copy->value.number));
- g_assert_cmpstr(double1, ==, double2);
- g_free(double1);
- g_free(double2);
+ g_string_printf(double_expected, "%.6f", pt->value.number);
+ g_string_printf(double_actual, "%.6f", pt_copy->value.number);
+ g_assert_cmpstr(double_actual->str, ==, double_expected->str);
+
+ g_string_free(double_expected, true);
+ g_string_free(double_actual, true);
} else if (pt->type == PTYPE_BOOLEAN) {
g_assert_cmpint(!!pt->value.max, ==, !!pt->value.max);
} else {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 08/10] qapi: add native list coverage for visitor serialization tests
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (6 preceding siblings ...)
2013-05-10 2:20 ` [Qemu-devel] [PATCH 07/10] qapi: fix visitor serialization tests for numbers/doubles Michael Roth
@ 2013-05-10 2:21 ` Michael Roth
2013-05-10 2:21 ` [Qemu-devel] [PATCH 09/10] qapi: add native list coverage for QMP output visitor tests Michael Roth
` (2 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:21 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
tests/test-visitor-serialization.c | 248 +++++++++++++++++++++++++++++++++---
1 file changed, 230 insertions(+), 18 deletions(-)
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index fed6810..b31911f 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -23,6 +23,25 @@
#include "qapi/qmp-output-visitor.h"
#include "qapi/string-input-visitor.h"
#include "qapi/string-output-visitor.h"
+#include "qapi-types.h"
+#include "qapi-visit.h"
+#include "qapi/dealloc-visitor.h"
+
+enum PrimitiveTypeKind {
+ PTYPE_STRING = 0,
+ PTYPE_BOOLEAN,
+ PTYPE_NUMBER,
+ PTYPE_INTEGER,
+ PTYPE_U8,
+ PTYPE_U16,
+ PTYPE_U32,
+ PTYPE_U64,
+ PTYPE_S8,
+ PTYPE_S16,
+ PTYPE_S32,
+ PTYPE_S64,
+ PTYPE_EOL,
+};
typedef struct PrimitiveType {
union {
@@ -40,26 +59,34 @@ typedef struct PrimitiveType {
int64_t s64;
intmax_t max;
} value;
- enum {
- PTYPE_STRING = 0,
- PTYPE_BOOLEAN,
- PTYPE_NUMBER,
- PTYPE_INTEGER,
- PTYPE_U8,
- PTYPE_U16,
- PTYPE_U32,
- PTYPE_U64,
- PTYPE_S8,
- PTYPE_S16,
- PTYPE_S32,
- PTYPE_S64,
- PTYPE_EOL,
- } type;
+ enum PrimitiveTypeKind type;
const char *description;
} PrimitiveType;
+typedef struct PrimitiveList {
+ union {
+ strList *strings;
+ boolList *booleans;
+ numberList *numbers;
+ intList *integers;
+ } value;
+ enum PrimitiveTypeKind type;
+ const char *description;
+} PrimitiveList;
+
/* test helpers */
+typedef void (*VisitorFunc)(Visitor *v, void **native, Error **errp);
+
+static void dealloc_helper(void *native_in, VisitorFunc visit, Error **errp)
+{
+ QapiDeallocVisitor *qdv = qapi_dealloc_visitor_new();
+
+ visit(qapi_dealloc_get_visitor(qdv), &native_in, errp);
+
+ qapi_dealloc_visitor_cleanup(qdv);
+}
+
static void visit_primitive_type(Visitor *v, void **native, Error **errp)
{
PrimitiveType *pt = *native;
@@ -105,6 +132,27 @@ static void visit_primitive_type(Visitor *v, void **native, Error **errp)
}
}
+static void visit_primitive_list(Visitor *v, void **native, Error **errp)
+{
+ PrimitiveList *pl = *native;
+ switch (pl->type) {
+ case PTYPE_STRING:
+ visit_type_strList(v, &pl->value.strings, NULL, errp);
+ break;
+ case PTYPE_BOOLEAN:
+ visit_type_boolList(v, &pl->value.booleans, NULL, errp);
+ break;
+ case PTYPE_NUMBER:
+ visit_type_numberList(v, &pl->value.numbers, NULL, errp);
+ break;
+ case PTYPE_INTEGER:
+ visit_type_intList(v, &pl->value.integers, NULL, errp);
+ break;
+ default:
+ g_assert(false);
+ }
+}
+
typedef struct TestStruct
{
int64_t integer;
@@ -206,12 +254,11 @@ static void visit_nested_struct_list(Visitor *v, void **native, Error **errp)
/* test cases */
-typedef void (*VisitorFunc)(Visitor *v, void **native, Error **errp);
-
typedef enum VisitorCapabilities {
VCAP_PRIMITIVES = 1,
VCAP_STRUCTURES = 2,
VCAP_LISTS = 4,
+ VCAP_PRIMITIVE_LISTS = 8,
} VisitorCapabilities;
typedef struct SerializeOps {
@@ -270,6 +317,152 @@ static void test_primitives(gconstpointer opaque)
g_free(pt_copy);
}
+static void test_primitive_lists(gconstpointer opaque)
+{
+ TestArgs *args = (TestArgs *) opaque;
+ const SerializeOps *ops = args->ops;
+ PrimitiveType *pt = args->test_data;
+ PrimitiveList pl = { .value = { 0 } };
+ PrimitiveList pl_copy = { .value = { 0 } };
+ PrimitiveList *pl_copy_ptr = &pl_copy;
+ Error *err = NULL;
+ void *serialize_data;
+ void *cur_head = NULL;
+ int i;
+
+ pl.type = pl_copy.type = pt->type;
+
+ /* build up our list of primitive types */
+ for (i = 0; i < 32; i++) {
+ switch (pl.type) {
+ case PTYPE_STRING: {
+ strList *tmp = g_new0(strList, 1);
+ tmp->value = g_strdup(pt->value.string);
+ if (pl.value.strings == NULL) {
+ pl.value.strings = tmp;
+ } else {
+ tmp->next = pl.value.strings;
+ pl.value.strings = tmp;
+ }
+ break;
+ }
+ case PTYPE_INTEGER: {
+ intList *tmp = g_new0(intList, 1);
+ tmp->value = pt->value.integer;
+ if (pl.value.integers == NULL) {
+ pl.value.integers = tmp;
+ } else {
+ tmp->next = pl.value.integers;
+ pl.value.integers = tmp;
+ }
+ break;
+ }
+ case PTYPE_NUMBER: {
+ numberList *tmp = g_new0(numberList, 1);
+ tmp->value = pt->value.number;
+ if (pl.value.numbers == NULL) {
+ pl.value.numbers = tmp;
+ } else {
+ tmp->next = pl.value.numbers;
+ pl.value.numbers = tmp;
+ }
+ break;
+ }
+ case PTYPE_BOOLEAN: {
+ boolList *tmp = g_new0(boolList, 1);
+ tmp->value = pt->value.boolean;
+ if (pl.value.booleans == NULL) {
+ pl.value.booleans = tmp;
+ } else {
+ tmp->next = pl.value.booleans;
+ pl.value.booleans = tmp;
+ }
+ break;
+ }
+ default:
+ g_assert(0);
+ }
+ }
+
+ ops->serialize((void **)&pl, &serialize_data, visit_primitive_list, &err);
+ ops->deserialize((void **)&pl_copy_ptr, serialize_data, visit_primitive_list, &err);
+
+ g_assert(err == NULL);
+ i = 0;
+
+ /* compare our deserialized list of primitives to the original */
+ do {
+ switch (pl_copy.type) {
+ case PTYPE_STRING: {
+ strList *ptr;
+ if (cur_head) {
+ ptr = cur_head;
+ cur_head = ptr->next;
+ } else {
+ cur_head = ptr = pl_copy.value.strings;
+ }
+ g_assert_cmpstr(pt->value.string, ==, ptr->value);
+ break;
+ }
+ case PTYPE_INTEGER: {
+ intList *ptr;
+ if (cur_head) {
+ ptr = cur_head;
+ cur_head = ptr->next;
+ } else {
+ cur_head = ptr = pl_copy.value.integers;
+ }
+ g_assert_cmpint(pt->value.integer, ==, ptr->value);
+ break;
+ }
+ case PTYPE_NUMBER: {
+ numberList *ptr;
+ GString *double_expected = g_string_new("");
+ GString *double_actual = g_string_new("");
+ if (cur_head) {
+ ptr = cur_head;
+ cur_head = ptr->next;
+ } else {
+ cur_head = ptr = pl_copy.value.numbers;
+ }
+ /* we serialize with %f for our reference visitors, so rather than
+ * fuzzy floating math to test "equality", just compare the
+ * formatted values
+ */
+ g_string_printf(double_expected, "%.6f", pt->value.number);
+ g_string_printf(double_actual, "%.6f", ptr->value);
+ g_assert_cmpstr(double_actual->str, ==, double_expected->str);
+ g_string_free(double_expected, true);
+ g_string_free(double_actual, true);
+ break;
+ }
+ case PTYPE_BOOLEAN: {
+ boolList *ptr;
+ if (cur_head) {
+ ptr = cur_head;
+ cur_head = ptr->next;
+ } else {
+ cur_head = ptr = pl_copy.value.booleans;
+ }
+ g_assert_cmpint(!!pt->value.boolean, ==, !!ptr->value);
+ break;
+ }
+ default:
+ g_assert(0);
+ }
+ i++;
+ } while (cur_head);
+
+ g_assert_cmpint(i, ==, 33);
+
+ ops->cleanup(serialize_data);
+ dealloc_helper(&pl, visit_primitive_list, &err);
+ g_assert(!err);
+ dealloc_helper(&pl_copy, visit_primitive_list, &err);
+ g_assert(!err);
+ g_free(args);
+}
+
static void test_struct(gconstpointer opaque)
{
TestArgs *args = (TestArgs *) opaque;
@@ -719,7 +912,8 @@ static const SerializeOps visitors[] = {
.serialize = qmp_serialize,
.deserialize = qmp_deserialize,
.cleanup = qmp_cleanup,
- .caps = VCAP_PRIMITIVES | VCAP_STRUCTURES | VCAP_LISTS
+ .caps = VCAP_PRIMITIVES | VCAP_STRUCTURES | VCAP_LISTS |
+ VCAP_PRIMITIVE_LISTS
},
{
.type = "String",
@@ -773,6 +967,24 @@ static void add_visitor_type(const SerializeOps *ops)
args->test_data = NULL;
g_test_add_data_func(testname, args, test_nested_struct_list);
}
+
+ if (ops->caps & VCAP_PRIMITIVE_LISTS) {
+ i = 0;
+ while (pt_values[i].type != PTYPE_EOL) {
+ if (pt_values[i].type == PTYPE_STRING
+ || pt_values[i].type == PTYPE_NUMBER
+ || pt_values[i].type == PTYPE_BOOLEAN
+ || pt_values[i].type == PTYPE_INTEGER) {
+ sprintf(testname, "%s/primitive_list/%s", testname_prefix,
+ pt_values[i].description);
+ args = g_malloc0(sizeof(*args));
+ args->ops = ops;
+ args->test_data = &pt_values[i];
+ g_test_add_data_func(testname, args, test_primitive_lists);
+ }
+ i++;
+ }
+ }
}
int main(int argc, char **argv)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 09/10] qapi: add native list coverage for QMP output visitor tests
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (7 preceding siblings ...)
2013-05-10 2:21 ` [Qemu-devel] [PATCH 08/10] qapi: add native list coverage for visitor serialization tests Michael Roth
@ 2013-05-10 2:21 ` Michael Roth
2013-05-10 2:21 ` [Qemu-devel] [PATCH 10/10] qapi: add native list coverage for QMP input " Michael Roth
2013-05-10 15:30 ` [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Luiz Capitulino
10 siblings, 0 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:21 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
This exercises schema-generated visitors for native list types and does
some sanity checking on validity of serialized data.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qapi-schema-test.json | 7 ++
tests/test-qmp-output-visitor.c | 176 +++++++++++++++++++++++++++++++++++++++
2 files changed, 183 insertions(+)
diff --git a/qapi-schema-test.json b/qapi-schema-test.json
index 9eae350..d49cdb3 100644
--- a/qapi-schema-test.json
+++ b/qapi-schema-test.json
@@ -32,6 +32,13 @@
{ 'union': 'UserDefUnion',
'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+# for testing native lists
+{ 'union': 'UserDefNativeListUnion',
+ 'data': { 'integer': ['int'],
+ 'number': ['number'],
+ 'boolean': ['bool'],
+ 'string': ['str'] } }
+
# testing commands
{ 'command': 'user_def_cmd', 'data': {} }
{ 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 71367e6..9bb8c71 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -431,6 +431,174 @@ static void test_visitor_out_union(TestOutputVisitorData *data,
QDECREF(qdict);
}
+static void init_native_list(UserDefNativeListUnion *cvalue)
+{
+ int i;
+ switch (cvalue->kind) {
+ case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
+ intList **list = &cvalue->integer;
+ for (i = 0; i < 32; i++) {
+ *list = g_new0(intList, 1);
+ (*list)->value = i;
+ (*list)->next = NULL;
+ list = &(*list)->next;
+ }
+ break;
+ }
+ case USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN: {
+ boolList **list = &cvalue->boolean;
+ for (i = 0; i < 32; i++) {
+ *list = g_new0(boolList, 1);
+ (*list)->value = (i % 3 == 0);
+ (*list)->next = NULL;
+ list = &(*list)->next;
+ }
+ break;
+ }
+ case USER_DEF_NATIVE_LIST_UNION_KIND_STRING: {
+ strList **list = &cvalue->string;
+ for (i = 0; i < 32; i++) {
+ *list = g_new0(strList, 1);
+ (*list)->value = g_strdup_printf("%d", i);
+ (*list)->next = NULL;
+ list = &(*list)->next;
+ }
+ break;
+ }
+ case USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER: {
+ numberList **list = &cvalue->number;
+ for (i = 0; i < 32; i++) {
+ *list = g_new0(numberList, 1);
+ (*list)->value = (double)i / 3;
+ (*list)->next = NULL;
+ list = &(*list)->next;
+ }
+ break;
+ }
+ default:
+ g_assert(false);
+ }
+}
+
+static void check_native_list(QObject *qobj,
+ UserDefNativeListUnionKind kind)
+{
+ QDict *qdict;
+ QList *qlist;
+ int i;
+
+ g_assert(qobj);
+ g_assert(qobject_type(qobj) == QTYPE_QDICT);
+ qdict = qobject_to_qdict(qobj);
+ g_assert(qdict);
+ g_assert(qdict_haskey(qdict, "data"));
+ qlist = qlist_copy(qobject_to_qlist(qdict_get(qdict, "data")));
+
+ switch (kind) {
+ case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER:
+ for (i = 0; i < 32; i++) {
+ QObject *tmp;
+ QInt *qvalue;
+ tmp = qlist_peek(qlist);
+ g_assert(tmp);
+ qvalue = qobject_to_qint(tmp);
+ g_assert_cmpint(qint_get_int(qvalue), ==, i);
+ qobject_decref(qlist_pop(qlist));
+ }
+ break;
+ case USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN:
+ for (i = 0; i < 32; i++) {
+ QObject *tmp;
+ QBool *qvalue;
+ tmp = qlist_peek(qlist);
+ g_assert(tmp);
+ qvalue = qobject_to_qbool(tmp);
+ g_assert_cmpint(qbool_get_int(qvalue), ==, (i % 3 == 0) ? 1 : 0);
+ qobject_decref(qlist_pop(qlist));
+ }
+ break;
+ case USER_DEF_NATIVE_LIST_UNION_KIND_STRING:
+ for (i = 0; i < 32; i++) {
+ QObject *tmp;
+ QString *qvalue;
+ gchar str[8];
+ tmp = qlist_peek(qlist);
+ g_assert(tmp);
+ qvalue = qobject_to_qstring(tmp);
+ sprintf(str, "%d", i);
+ g_assert_cmpstr(qstring_get_str(qvalue), ==, str);
+ qobject_decref(qlist_pop(qlist));
+ }
+ break;
+ case USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER:
+ for (i = 0; i < 32; i++) {
+ QObject *tmp;
+ QFloat *qvalue;
+ GString *double_expected = g_string_new("");
+ GString *double_actual = g_string_new("");
+
+ tmp = qlist_peek(qlist);
+ g_assert(tmp);
+ qvalue = qobject_to_qfloat(tmp);
+ g_string_printf(double_expected, "%.6f", (double)i / 3);
+ g_string_printf(double_actual, "%.6f", qfloat_get_double(qvalue));
+ g_assert_cmpstr(double_actual->str, ==, double_expected->str);
+
+ qobject_decref(qlist_pop(qlist));
+ g_string_free(double_expected, true);
+ g_string_free(double_actual, true);
+ }
+ break;
+ default:
+ g_assert(false);
+ }
+ QDECREF(qlist);
+}
+
+static void test_native_list(TestOutputVisitorData *data,
+ const void *unused,
+ UserDefNativeListUnionKind kind)
+{
+ UserDefNativeListUnion *cvalue = g_new0(UserDefNativeListUnion, 1);
+ Error *err = NULL;
+ QObject *obj;
+
+ cvalue->kind = kind;
+ init_native_list(cvalue);
+
+ visit_type_UserDefNativeListUnion(data->ov, &cvalue, NULL, &err);
+ g_assert(err == NULL);
+
+ obj = qmp_output_get_qobject(data->qov);
+ check_native_list(obj, cvalue->kind);
+ qapi_free_UserDefNativeListUnion(cvalue);
+ qobject_decref(obj);
+}
+
+static void test_visitor_out_native_list_int(TestOutputVisitorData *data,
+ const void *unused)
+{
+ test_native_list(data, unused, USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER);
+}
+
+static void test_visitor_out_native_list_bool(TestOutputVisitorData *data,
+ const void *unused)
+{
+ test_native_list(data, unused, USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN);
+}
+
+static void test_visitor_out_native_list_str(TestOutputVisitorData *data,
+ const void *unused)
+{
+ test_native_list(data, unused, USER_DEF_NATIVE_LIST_UNION_KIND_STRING);
+}
+
+static void test_visitor_out_native_list_number(TestOutputVisitorData *data,
+ const void *unused)
+{
+ test_native_list(data, unused, USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER);
+}
+
static void output_visitor_test_add(const char *testpath,
TestOutputVisitorData *data,
void (*test_func)(TestOutputVisitorData *data, const void *user_data))
@@ -471,6 +639,14 @@ int main(int argc, char **argv)
&out_visitor_data, test_visitor_out_list_qapi_free);
output_visitor_test_add("/visitor/output/union",
&out_visitor_data, test_visitor_out_union);
+ output_visitor_test_add("/visitor/output/native_list/int",
+ &out_visitor_data, test_visitor_out_native_list_int);
+ output_visitor_test_add("/visitor/output/native_list/bool",
+ &out_visitor_data, test_visitor_out_native_list_bool);
+ output_visitor_test_add("/visitor/output/native_list/string",
+ &out_visitor_data, test_visitor_out_native_list_str);
+ output_visitor_test_add("/visitor/output/native_list/number",
+ &out_visitor_data, test_visitor_out_native_list_number);
g_test_run();
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 10/10] qapi: add native list coverage for QMP input visitor tests
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (8 preceding siblings ...)
2013-05-10 2:21 ` [Qemu-devel] [PATCH 09/10] qapi: add native list coverage for QMP output visitor tests Michael Roth
@ 2013-05-10 2:21 ` Michael Roth
2013-05-10 15:30 ` [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Luiz Capitulino
10 siblings, 0 replies; 30+ messages in thread
From: Michael Roth @ 2013-05-10 2:21 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, lersek, lcapitulino
This exercises schema-generated visitors for native list types and does
some sanity checking on validity of deserialized data.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
tests/test-qmp-input-visitor.c | 186 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 186 insertions(+)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 955a4c0..483a20b 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -61,6 +61,31 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
return v;
}
+/* similar to visitor_input_test_init(), but does not expect a string
+ * literal/format json_string argument and so can be used for
+ * programatically generated strings (and we can't pass in programatically
+ * generated strings via %s format parameters since qobject_from_jsonv()
+ * will wrap those in double-quotes and treat the entire object as a
+ * string)
+ */
+static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
+ const char *json_string)
+{
+ Visitor *v;
+
+ data->obj = qobject_from_json(json_string);
+
+ g_assert(data->obj != NULL);
+
+ data->qiv = qmp_input_visitor_new(data->obj);
+ g_assert(data->qiv != NULL);
+
+ v = qmp_input_get_visitor(data->qiv);
+ g_assert(v != NULL);
+
+ return v;
+}
+
static void test_visitor_in_int(TestInputVisitorData *data,
const void *unused)
{
@@ -259,6 +284,159 @@ static void test_visitor_in_union(TestInputVisitorData *data,
qapi_free_UserDefUnion(tmp);
}
+static void test_visitor_in_native_list_int(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefNativeListUnion *cvalue = NULL;
+ intList *elem = NULL;
+ Error *err = NULL;
+ Visitor *v;
+ GString *gstr_list = g_string_new("");
+ GString *gstr_union = g_string_new("");
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ g_string_append_printf(gstr_list, "%d", i);
+ if (i != 31) {
+ g_string_append(gstr_list, ", ");
+ }
+ }
+ g_string_append_printf(gstr_union, "{ 'type': 'integer', 'data': [ %s ] }",
+ gstr_list->str);
+ v = visitor_input_test_init_raw(data, gstr_union->str);
+
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
+ g_assert(err == NULL);
+ g_assert(cvalue != NULL);
+ g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER);
+
+ for (i = 0, elem = cvalue->integer; elem; elem = elem->next, i++) {
+ g_assert_cmpint(elem->value, ==, i);
+ }
+
+ g_string_free(gstr_union, true);
+ g_string_free(gstr_list, true);
+ qapi_free_UserDefNativeListUnion(cvalue);
+}
+
+static void test_visitor_in_native_list_bool(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefNativeListUnion *cvalue = NULL;
+ boolList *elem = NULL;
+ Error *err = NULL;
+ Visitor *v;
+ GString *gstr_list = g_string_new("");
+ GString *gstr_union = g_string_new("");
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ g_string_append_printf(gstr_list, "%s",
+ (i % 3 == 0) ? "true" : "false");
+ if (i != 31) {
+ g_string_append(gstr_list, ", ");
+ }
+ }
+ g_string_append_printf(gstr_union, "{ 'type': 'boolean', 'data': [ %s ] }",
+ gstr_list->str);
+ v = visitor_input_test_init_raw(data, gstr_union->str);
+
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
+ g_assert(err == NULL);
+ g_assert(cvalue != NULL);
+ g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN);
+
+ for (i = 0, elem = cvalue->boolean; elem; elem = elem->next, i++) {
+ g_assert_cmpint(elem->value, ==, (i % 3 == 0) ? 1 : 0);
+ }
+
+ g_string_free(gstr_union, true);
+ g_string_free(gstr_list, true);
+ qapi_free_UserDefNativeListUnion(cvalue);
+}
+
+static void test_visitor_in_native_list_string(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefNativeListUnion *cvalue = NULL;
+ strList *elem = NULL;
+ Error *err = NULL;
+ Visitor *v;
+ GString *gstr_list = g_string_new("");
+ GString *gstr_union = g_string_new("");
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ g_string_append_printf(gstr_list, "'%d'", i);
+ if (i != 31) {
+ g_string_append(gstr_list, ", ");
+ }
+ }
+ g_string_append_printf(gstr_union, "{ 'type': 'string', 'data': [ %s ] }",
+ gstr_list->str);
+ v = visitor_input_test_init_raw(data, gstr_union->str);
+
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
+ g_assert(err == NULL);
+ g_assert(cvalue != NULL);
+ g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_STRING);
+
+ for (i = 0, elem = cvalue->string; elem; elem = elem->next, i++) {
+ gchar str[8];
+ sprintf(str, "%d", i);
+ g_assert_cmpstr(elem->value, ==, str);
+ }
+
+ g_string_free(gstr_union, true);
+ g_string_free(gstr_list, true);
+ qapi_free_UserDefNativeListUnion(cvalue);
+}
+
+#define DOUBLE_STR_MAX 16
+
+static void test_visitor_in_native_list_number(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefNativeListUnion *cvalue = NULL;
+ numberList *elem = NULL;
+ Error *err = NULL;
+ Visitor *v;
+ GString *gstr_list = g_string_new("");
+ GString *gstr_union = g_string_new("");
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ g_string_append_printf(gstr_list, "%f", (double)i / 3);
+ if (i != 31) {
+ g_string_append(gstr_list, ", ");
+ }
+ }
+ g_string_append_printf(gstr_union, "{ 'type': 'number', 'data': [ %s ] }",
+ gstr_list->str);
+ v = visitor_input_test_init_raw(data, gstr_union->str);
+
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
+ g_assert(err == NULL);
+ g_assert(cvalue != NULL);
+ g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER);
+
+ for (i = 0, elem = cvalue->number; elem; elem = elem->next, i++) {
+ GString *double_expected = g_string_new("");
+ GString *double_actual = g_string_new("");
+
+ g_string_printf(double_expected, "%.6f", (double)i / 3);
+ g_string_printf(double_actual, "%.6f", elem->value);
+ g_assert_cmpstr(double_expected->str, ==, double_actual->str);
+
+ g_string_free(double_expected, true);
+ g_string_free(double_actual, true);
+ }
+
+ g_string_free(gstr_union, true);
+ g_string_free(gstr_list, true);
+ qapi_free_UserDefNativeListUnion(cvalue);
+}
+
static void input_visitor_test_add(const char *testpath,
TestInputVisitorData *data,
void (*test_func)(TestInputVisitorData *data, const void *user_data))
@@ -310,6 +488,14 @@ int main(int argc, char **argv)
&in_visitor_data, test_visitor_in_union);
input_visitor_test_add("/visitor/input/errors",
&in_visitor_data, test_visitor_in_errors);
+ input_visitor_test_add("/visitor/input/native_list/int",
+ &in_visitor_data, test_visitor_in_native_list_int);
+ input_visitor_test_add("/visitor/input/native_list/bool",
+ &in_visitor_data, test_visitor_in_native_list_bool);
+ input_visitor_test_add("/visitor/input/native_list/str",
+ &in_visitor_data, test_visitor_in_native_list_string);
+ input_visitor_test_add("/visitor/input/native_list/number",
+ &in_visitor_data, test_visitor_in_native_list_number);
g_test_run();
--
1.7.9.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support
2013-05-10 2:20 ` [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support Michael Roth
@ 2013-05-10 3:04 ` Amos Kong
2013-05-10 11:32 ` mdroth
2013-05-10 14:07 ` Luiz Capitulino
1 sibling, 1 reply; 30+ messages in thread
From: Amos Kong @ 2013-05-10 3:04 UTC (permalink / raw)
To: Michael Roth; +Cc: lersek, qemu-devel, lcapitulino
On Thu, May 09, 2013 at 09:20:53PM -0500, Michael Roth wrote:
> Teach type generators about native types so they can generate the
> appropriate linked list types.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> scripts/qapi-types.py | 43 ++++++++++++++++++++++++++++++++++++++++---
> scripts/qapi.py | 21 +++++++++++++++++++++
> 2 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 9e19920..96cb26d 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,7 +16,18 @@ import os
> import getopt
> import errno
<snip>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index afc5f32..0ac8c2b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -11,6 +11,10 @@
>
> from ordereddict import OrderedDict
>
> +builtin_types = [
> + 'str', 'int', 'number', 'bool'
Will we add size, int8, uint32, ... in future when they are needed?
> +]
> +
--
Amos.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support
2013-05-10 3:04 ` Amos Kong
@ 2013-05-10 11:32 ` mdroth
0 siblings, 0 replies; 30+ messages in thread
From: mdroth @ 2013-05-10 11:32 UTC (permalink / raw)
To: Amos Kong; +Cc: lersek, qemu-devel, lcapitulino
On Fri, May 10, 2013 at 11:04:22AM +0800, Amos Kong wrote:
> On Thu, May 09, 2013 at 09:20:53PM -0500, Michael Roth wrote:
> > Teach type generators about native types so they can generate the
> > appropriate linked list types.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > scripts/qapi-types.py | 43 ++++++++++++++++++++++++++++++++++++++++---
> > scripts/qapi.py | 21 +++++++++++++++++++++
> > 2 files changed, 61 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 9e19920..96cb26d 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -16,7 +16,18 @@ import os
> > import getopt
> > import errno
>
> <snip>
>
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index afc5f32..0ac8c2b 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -11,6 +11,10 @@
> >
> > from ordereddict import OrderedDict
> >
> > +builtin_types = [
> > + 'str', 'int', 'number', 'bool'
>
> Will we add size, int8, uint32, ... in future when they are needed?
Well, that was the plan, but I hadn't recalled that support for these
was already been added to the code generators as part of the Netdev
opts-visitor stuff, so I'll go ahead and do a re-spin with those added.
>
> > +]
> > +
>
> --
> Amos.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 2:20 ` [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values Michael Roth
@ 2013-05-10 11:55 ` Laszlo Ersek
2013-05-10 12:22 ` Eric Blake
2013-05-10 15:17 ` Luiz Capitulino
2 siblings, 0 replies; 30+ messages in thread
From: Laszlo Ersek @ 2013-05-10 11:55 UTC (permalink / raw)
To: Michael Roth; +Cc: akong, qemu-devel, lcapitulino
On 05/10/13 04:20, Michael Roth wrote:
> Currently our JSON parser assumes that numbers lacking a mantissa are
> integers and attempts to store them as QInt/int64 values. This breaks in
> the case where the number overflows/underflows int64 values (which is
> still valid JSON)
>
> Fix this by detecting such cases and using a QFloat to store the value
> instead.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qobject/json-parser.c | 26 +++++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 05279c1..4d14e71 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
> case JSON_STRING:
> obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
> break;
> - case JSON_INTEGER:
> - obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
> - break;
> + case JSON_INTEGER: {
> + /* A possibility exists that this is a whole-valued float where the
> + * mantissa was left out due to being 0 (.0). It's not a big deal to
> + * treat these as ints in the parser, so long as users of the
> + * resulting QObject know to expect a QInt in place of a QFloat in
> + * cases like these.
> + *
> + * However, in some cases these values will overflow/underflow a
> + * QInt/int64 container, thus we should assume these are to be handled
> + * as QFloats/doubles rather than silently changing their values.
> + *
> + * strtoll() indicates these instances by setting errno to ERANGE
> + */
> + int64_t value;
> +
> + errno = 0; /* strtoll doesn't set errno on success */
> + value = strtoll(token_get_value(token), NULL, 10);
> + if (errno != ERANGE) {
> + obj = QOBJECT(qint_from_int(value));
> + break;
> + }
> + /* fall through to JSON_FLOAT */
> + }
> case JSON_FLOAT:
> /* FIXME dependent on locale */
> obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), NULL)));
>
I wanted to correct you here and propose s/mantissa/fractional part/
everywhere in this patch (including commit message and patch body).
However <http://en.wiktionary.org/wiki/mantissa> tells me one meaning of
"mantissa" *is* "fractional part".
In that sense I won't try to "correct" you, but can you consider doing
the replacement still, in order not to confuse non-native speakers who
associate "mantissa" only with the third meaning Wiktionary gives, ie.
the significand in FP representation?
Otherwise the v2 series looks good to me (not R-b-ing it since you'll
post v3 to address Amos's note).
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 2:20 ` [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values Michael Roth
2013-05-10 11:55 ` Laszlo Ersek
@ 2013-05-10 12:22 ` Eric Blake
2013-05-10 12:47 ` Laszlo Ersek
2013-05-10 15:17 ` Luiz Capitulino
2 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2013-05-10 12:22 UTC (permalink / raw)
To: Michael Roth; +Cc: akong, lersek, qemu-devel, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]
On 05/09/2013 08:20 PM, Michael Roth wrote:
> Currently our JSON parser assumes that numbers lacking a mantissa are
> integers and attempts to store them as QInt/int64 values. This breaks in
> the case where the number overflows/underflows int64 values (which is
> still valid JSON)
>
> Fix this by detecting such cases and using a QFloat to store the value
> instead.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qobject/json-parser.c | 26 +++++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
This changes the error message handed back to QMP clients, and possibly
exposes problems in other qemu code that receives the result of json
parses. Previously, for an 'int' argument, if you passed in a too-large
number, you got an error that the argument was too large for int. Now,
the number is accepted as a double; are we guaranteed that in a context
that expects a qint, when that code is now handed a qfloat (a case which
was previously impossible because qint_from_int protected it), that the
code will still behave correctly?
At any rate, libvirt already checks that all numbers that fall outside
the range of int64_t are never passed over qmp when passing an int
argument (and yes, this is annoying, in that large 64-bit unsigned
numbers have to be passed as negative numbers, rather than exceeding
INT64_MAX), so libvirt should not be triggering this newly exposed code
path. But even if libvirt doesn't plan on triggering it, I'd still feel
better if your commit message documented evidence of testing what
happens in this case. For example, compare what
{"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}}
does before and after this patch.
> + /* fall through to JSON_FLOAT */
> + }
> case JSON_FLOAT:
> /* FIXME dependent on locale */
You know, strtoll() also is dependent on locale (that is, non-C locale
can accept strings that the C locale rejects), if you want to repeat
this comment earlier for case JSON_INTEGER.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 12:22 ` Eric Blake
@ 2013-05-10 12:47 ` Laszlo Ersek
2013-05-10 13:30 ` mdroth
2013-05-10 14:08 ` Eric Blake
0 siblings, 2 replies; 30+ messages in thread
From: Laszlo Ersek @ 2013-05-10 12:47 UTC (permalink / raw)
To: Eric Blake; +Cc: lcapitulino, akong, Michael Roth, qemu-devel
On 05/10/13 14:22, Eric Blake wrote:
> On 05/09/2013 08:20 PM, Michael Roth wrote:
>> Currently our JSON parser assumes that numbers lacking a mantissa are
>> integers and attempts to store them as QInt/int64 values. This breaks in
>> the case where the number overflows/underflows int64 values (which is
>> still valid JSON)
>>
>> Fix this by detecting such cases and using a QFloat to store the value
>> instead.
>>
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>> qobject/json-parser.c | 26 +++++++++++++++++++++++---
>> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> This changes the error message handed back to QMP clients, and possibly
> exposes problems in other qemu code that receives the result of json
> parses. Previously, for an 'int' argument, if you passed in a too-large
> number, you got an error that the argument was too large for int. Now,
> the number is accepted as a double; are we guaranteed that in a context
> that expects a qint, when that code is now handed a qfloat (a case which
> was previously impossible because qint_from_int protected it), that the
> code will still behave correctly?
I tried to consider this while reviewing... Maybe I was wrong.
The pre-patch code for JSON_INTEGER:
obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
doesn't check for errors at all. (I assume that JSON_INTEGER is selected
by the parser, token_get_type(), based on syntax purely.)
I thought when the pre-patch version encounters an int-looking decimal
string that's actually too big in magnitude for an int, you'd simply end
up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
value, errno is lost, and qint_from_int() sees nothing wrong.
With the patch, you end up with a float instead of an int-typed
LLONG_MIN/LLONG_MAX, and also no error.
> At any rate, libvirt already checks that all numbers that fall outside
> the range of int64_t are never passed over qmp when passing an int
> argument (and yes, this is annoying, in that large 64-bit unsigned
> numbers have to be passed as negative numbers, rather than exceeding
> INT64_MAX), so libvirt should not be triggering this newly exposed code
> path. But even if libvirt doesn't plan on triggering it, I'd still feel
> better if your commit message documented evidence of testing what
> happens in this case. For example, compare what
> {"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}}
> does before and after this patch.
That would be likely interesting to test, yes.
Laszlo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 12:47 ` Laszlo Ersek
@ 2013-05-10 13:30 ` mdroth
2013-05-10 14:08 ` Eric Blake
1 sibling, 0 replies; 30+ messages in thread
From: mdroth @ 2013-05-10 13:30 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: akong, qemu-devel, lcapitulino
On Fri, May 10, 2013 at 02:47:18PM +0200, Laszlo Ersek wrote:
> On 05/10/13 14:22, Eric Blake wrote:
> > On 05/09/2013 08:20 PM, Michael Roth wrote:
> >> Currently our JSON parser assumes that numbers lacking a mantissa are
> >> integers and attempts to store them as QInt/int64 values. This breaks in
> >> the case where the number overflows/underflows int64 values (which is
> >> still valid JSON)
> >>
> >> Fix this by detecting such cases and using a QFloat to store the value
> >> instead.
> >>
> >> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> ---
> >> qobject/json-parser.c | 26 +++++++++++++++++++++++---
> >> 1 file changed, 23 insertions(+), 3 deletions(-)
> >
> > This changes the error message handed back to QMP clients, and possibly
> > exposes problems in other qemu code that receives the result of json
> > parses. Previously, for an 'int' argument, if you passed in a too-large
> > number, you got an error that the argument was too large for int. Now,
> > the number is accepted as a double; are we guaranteed that in a context
> > that expects a qint, when that code is now handed a qfloat (a case which
> > was previously impossible because qint_from_int protected it), that the
> > code will still behave correctly?
>
> I tried to consider this while reviewing... Maybe I was wrong.
>
> The pre-patch code for JSON_INTEGER:
>
> obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
>
> doesn't check for errors at all. (I assume that JSON_INTEGER is selected
> by the parser, token_get_type(), based on syntax purely.)
>
> I thought when the pre-patch version encounters an int-looking decimal
> string that's actually too big in magnitude for an int, you'd simply end
> up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
> value, errno is lost, and qint_from_int() sees nothing wrong.
>
> With the patch, you end up with a float instead of an int-typed
> LLONG_MIN/LLONG_MAX, and also no error.
>
This is correct, but it does make code acting on the resulting QObject
capable of checking for this scenario and acting accordingly in whatever
manner is appropriate. I think that's appropriate, since there's no error
as far as the JSON spec goes, but further up the stack we may have stricter
requirements on what the valid ranges are for various fields.
For instance, in the case of QmpInputVisitor, these cases will
trigger an error case that was previously being bypassed when
out-of-range values were provided in place of 'int' due to the parser
silently capping the max/min values:
in qmp_input_type_int():
QObject *qobj = qmp_input_get_object(qiv, name);
if (!qobj || qobject_type(qobj) != QTYPE_QINT) {
error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"integer");
return;
}
We'd now trigger this error for such inputs (since we'd have QTYPE_QFLOAT),
which is stricter checking than before, but as intended since we map int to
int64_t and expect values in that range (though we could stand to be clearer
about this in the QAPI documentation)
> > At any rate, libvirt already checks that all numbers that fall outside
> > the range of int64_t are never passed over qmp when passing an int
> > argument (and yes, this is annoying, in that large 64-bit unsigned
> > numbers have to be passed as negative numbers, rather than exceeding
> > INT64_MAX), so libvirt should not be triggering this newly exposed code
> > path. But even if libvirt doesn't plan on triggering it, I'd still feel
> > better if your commit message documented evidence of testing what
> > happens in this case. For example, compare what
> > {"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}}
> > does before and after this patch.
>
> That would be likely interesting to test, yes.
That would trigger the error mentioned above, as opposed to silently changing
the value to LLONG_MAX. I'll see about covering this case somewhere in
tests/test-qmp-input-visitor.
>
> Laszlo
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support
2013-05-10 2:20 ` [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support Michael Roth
2013-05-10 3:04 ` Amos Kong
@ 2013-05-10 14:07 ` Luiz Capitulino
2013-05-10 15:51 ` mdroth
1 sibling, 1 reply; 30+ messages in thread
From: Luiz Capitulino @ 2013-05-10 14:07 UTC (permalink / raw)
To: Michael Roth; +Cc: akong, lersek, qemu-devel
On Thu, 9 May 2013 21:20:53 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Teach type generators about native types so they can generate the
> appropriate linked list types.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> scripts/qapi-types.py | 43 ++++++++++++++++++++++++++++++++++++++++---
> scripts/qapi.py | 21 +++++++++++++++++++++
> 2 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 9e19920..96cb26d 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,7 +16,18 @@ import os
> import getopt
> import errno
>
> -def generate_fwd_struct(name, members):
> +def generate_fwd_struct(name, members, builtin_type=False):
> + if builtin_type:
> + return mcgen('''
> +typedef struct %(name)sList
> +{
> + %(type)s value;
> + struct %(name)sList *next;
> +} %(name)sList;
> +''',
Sorry for the utterly minor comment, but as you're going to respin please
add a newline before ''' so that we get the declarations properly separated
when generated.
> + type=c_type(name),
> + name=name)
> +
> return mcgen('''
> typedef struct %(name)s %(name)s;
>
> @@ -164,6 +175,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
>
> def generate_type_cleanup(name):
> ret = mcgen('''
> +
> void qapi_free_%(type)s(%(c_type)s obj)
> {
> QapiDeallocVisitor *md;
> @@ -184,8 +196,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
>
>
> try:
> - opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> - ["source", "header", "prefix=", "output-dir="])
> + opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> + ["source", "header", "builtins",
> + "prefix=", "output-dir="])
> except getopt.GetoptError, err:
> print str(err)
> sys.exit(1)
> @@ -197,6 +210,7 @@ h_file = 'qapi-types.h'
>
> do_c = False
> do_h = False
> +do_builtins = False
>
> for o, a in opts:
> if o in ("-p", "--prefix"):
> @@ -207,6 +221,8 @@ for o, a in opts:
> do_c = True
> elif o in ("-h", "--header"):
> do_h = True
> + elif o in ("-b", "--builtins"):
> + do_builtins = True
>
> if not do_c and not do_h:
> do_c = True
> @@ -282,6 +298,11 @@ fdecl.write(mcgen('''
> exprs = parse_schema(sys.stdin)
> exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
>
> +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> +for typename in builtin_types:
> + fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
> +fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> +
> for expr in exprs:
> ret = "\n"
> if expr.has_key('type'):
> @@ -298,6 +319,22 @@ for expr in exprs:
> continue
> fdecl.write(ret)
>
> +# to avoid header dependency hell, we always generate declarations
> +# for built-in types in our header files and simply guard them
> +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> +for typename in builtin_types:
> + fdecl.write(generate_type_cleanup_decl(typename + "List"))
> +fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
I'm not sure I got why you're doing this. Is it because you're going to
generate them in more .h files? This is a bit ugly :(
> +
> +# ...this doesn't work for cases where we link in multiple objects that
> +# have the functions defined, so we use -b option to provide control
> +# over these cases
> +if do_builtins:
> + fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> + for typename in builtin_types:
> + fdef.write(generate_type_cleanup(typename + "List"))
> + fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> +
> for expr in exprs:
> ret = "\n"
> if expr.has_key('type'):
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index afc5f32..0ac8c2b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -11,6 +11,10 @@
>
> from ordereddict import OrderedDict
>
> +builtin_types = [
> + 'str', 'int', 'number', 'bool'
> +]
> +
> def tokenize(data):
> while len(data):
> ch = data[0]
> @@ -242,3 +246,20 @@ def guardname(filename):
> for substr in [".", " ", "-"]:
> guard = guard.replace(substr, "_")
> return guard.upper() + '_H'
> +
> +def guardstart(name):
> + return mcgen('''
> +
> +#ifndef %(name)s
> +#define %(name)s
> +
> +''',
> + name=guardname(name))
> +
> +def guardend(name):
> + return mcgen('''
> +
> +#endif /* %(name)s */
> +
> +''',
> + name=guardname(name))
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 12:47 ` Laszlo Ersek
2013-05-10 13:30 ` mdroth
@ 2013-05-10 14:08 ` Eric Blake
2013-05-10 14:51 ` mdroth
1 sibling, 1 reply; 30+ messages in thread
From: Eric Blake @ 2013-05-10 14:08 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: lcapitulino, akong, Michael Roth, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]
On 05/10/2013 06:47 AM, Laszlo Ersek wrote:
> The pre-patch code for JSON_INTEGER:
>
> obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
>
> doesn't check for errors at all. (I assume that JSON_INTEGER is selected
> by the parser, token_get_type(), based on syntax purely.)
>
> I thought when the pre-patch version encounters an int-looking decimal
> string that's actually too big in magnitude for an int, you'd simply end
> up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
> value, errno is lost, and qint_from_int() sees nothing wrong.
Oh, right. _That's_ why libvirt had to add checks that it wasn't
passing 0x8000000000000000ULL as a positive number - because the qemu
parser was silently clamping it to 0x7fffffffffffffffLL, which is not
what libvirt wanted. So the code was NOT erroring out with an overflow
message, but was acting on the wrong integer.
>
> With the patch, you end up with a float instead of an int-typed
> LLONG_MIN/LLONG_MAX, and also no error.
Ah, but here we have a difference - beforehand, the code was passing a
valid (albeit wrong value) qint, so the rest of the qemu code was
oblivious to the fact that the QMP message contained an overflow. But
now the code is passing a qdouble, and the rest of the qemu code may be
unprepared to handle it when expecting a qint.
>
>> At any rate, libvirt already checks that all numbers that fall outside
>> the range of int64_t are never passed over qmp when passing an int
>> argument (and yes, this is annoying, in that large 64-bit unsigned
>> numbers have to be passed as negative numbers, rather than exceeding
>> INT64_MAX), so libvirt should not be triggering this newly exposed code
>> path. But even if libvirt doesn't plan on triggering it, I'd still feel
>> better if your commit message documented evidence of testing what
>> happens in this case. For example, compare what
>> {"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}}
>> does before and after this patch.
>
> That would be likely interesting to test, yes.
add-fd may not be the best candidate (it expects an fd to be passed at
the same time, and does its own checking that it does not get a negative
number); but I'm sure there's plenty of other candidates (add-cpu is
another possibility that comes quickly to mind) - basically, pick a
command that takes an explicit 'int' argument, and overflow that
argument to see what happens when the command now has to deal with a
qdouble.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code
2013-05-10 2:20 ` [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code Michael Roth
@ 2013-05-10 14:10 ` Luiz Capitulino
2013-05-10 16:32 ` mdroth
0 siblings, 1 reply; 30+ messages in thread
From: Luiz Capitulino @ 2013-05-10 14:10 UTC (permalink / raw)
To: Michael Roth; +Cc: akong, lersek, qemu-devel
On Thu, 9 May 2013 21:20:56 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
> qapi-types.c/qapi-visit.c
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> Makefile | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 7dc0204..9695c9d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
> # Build libraries
>
> libqemustub.a: $(stub-obj-y)
> -libqemuutil.a: $(util-obj-y)
> +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
Don't we want this in for 1.5?
>
> ######################################################################
>
> @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>
> qapi-types.c qapi-types.h :\
> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." < $<, " GEN $@")
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -b < $<, " GEN $@")
> qapi-visit.c qapi-visit.h :\
> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." < $<, " GEN $@")
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@")
> qmp-commands.h qmp-marshal.c :\
> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@")
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 14:08 ` Eric Blake
@ 2013-05-10 14:51 ` mdroth
0 siblings, 0 replies; 30+ messages in thread
From: mdroth @ 2013-05-10 14:51 UTC (permalink / raw)
To: Eric Blake; +Cc: akong, Laszlo Ersek, qemu-devel, lcapitulino
On Fri, May 10, 2013 at 08:08:05AM -0600, Eric Blake wrote:
> On 05/10/2013 06:47 AM, Laszlo Ersek wrote:
>
> > The pre-patch code for JSON_INTEGER:
> >
> > obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
> >
> > doesn't check for errors at all. (I assume that JSON_INTEGER is selected
> > by the parser, token_get_type(), based on syntax purely.)
> >
> > I thought when the pre-patch version encounters an int-looking decimal
> > string that's actually too big in magnitude for an int, you'd simply end
> > up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
> > value, errno is lost, and qint_from_int() sees nothing wrong.
>
> Oh, right. _That's_ why libvirt had to add checks that it wasn't
> passing 0x8000000000000000ULL as a positive number - because the qemu
> parser was silently clamping it to 0x7fffffffffffffffLL, which is not
> what libvirt wanted. So the code was NOT erroring out with an overflow
> message, but was acting on the wrong integer.
>
> >
> > With the patch, you end up with a float instead of an int-typed
> > LLONG_MIN/LLONG_MAX, and also no error.
>
> Ah, but here we have a difference - beforehand, the code was passing a
> valid (albeit wrong value) qint, so the rest of the qemu code was
> oblivious to the fact that the QMP message contained an overflow. But
> now the code is passing a qdouble, and the rest of the qemu code may be
> unprepared to handle it when expecting a qint.
Yup, new error cases can be triggered, but in the case of
QmpInputVisitor this is handled appropriately (will add a test case to
confirm), and none of our other input visitors act on QObjects, and this
ambiguity isn't present for output visitors.
We also have monitor events that call qobject_from_json() to marshall
event payloads, but these are essentially open-coded QmpInputVisitors
where the JSON values come from native C types. The only case where I
can see this triggering the change is if they did something like:
obj = qobject_from_jsonf("{'myInt': %f}", whole_valued_float);
which would be evil, and thankfully such cases don't appear to exist:
mdroth@loki:~/w/qemu.git$ ack-grep qobject_from_json | grep "%f"
tests/check-qjson.c:987: obj = qobject_from_jsonf("%f", valuef);
mdroth@loki:~/w/qemu.git$
(the 'valuef' above is not whole-valued, and the output is expected to
be a QFloat)
I'm not aware of any other cases to consider, but I might've missed
something.
>
> >
> >> At any rate, libvirt already checks that all numbers that fall outside
> >> the range of int64_t are never passed over qmp when passing an int
> >> argument (and yes, this is annoying, in that large 64-bit unsigned
> >> numbers have to be passed as negative numbers, rather than exceeding
> >> INT64_MAX), so libvirt should not be triggering this newly exposed code
> >> path. But even if libvirt doesn't plan on triggering it, I'd still feel
> >> better if your commit message documented evidence of testing what
> >> happens in this case. For example, compare what
> >> {"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}}
> >> does before and after this patch.
> >
> > That would be likely interesting to test, yes.
>
> add-fd may not be the best candidate (it expects an fd to be passed at
> the same time, and does its own checking that it does not get a negative
> number); but I'm sure there's plenty of other candidates (add-cpu is
> another possibility that comes quickly to mind) - basically, pick a
> command that takes an explicit 'int' argument, and overflow that
> argument to see what happens when the command now has to deal with a
> qdouble.
Command params will end up getting marshalled in QObject prior to being
passed into commands:
mi = qmp_input_visitor_new_strict(QOBJECT(args));
v = qmp_input_get_visitor(mi);
visit_start_optional(v, &has_fdset_id, "fdset-id", errp);
if (has_fdset_id) {
visit_type_int(v, &fdset_id, "fdset-id", errp);
}
visit_end_optional(v, errp);
visit_start_optional(v, &has_opaque, "opaque", errp);
if (has_opaque) {
visit_type_str(v, &opaque, "opaque", errp);
}
visit_end_optional(v, errp);
qmp_input_visitor_cleanup(mi);
if (error_is_set(errp)) {
goto out;
}
retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, errp);
so i think a check in tests-qmp-input-visitor that verifies that values that
exceed LLONG_MAX/LLONG_MIN will get added into the QObject as QFloats
and trigger a type error when being passed to visit_type_int() should
cover the cases in question.
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests
2013-05-10 2:20 ` [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests Michael Roth
@ 2013-05-10 15:14 ` Luiz Capitulino
0 siblings, 0 replies; 30+ messages in thread
From: Luiz Capitulino @ 2013-05-10 15:14 UTC (permalink / raw)
To: Michael Roth; +Cc: akong, lersek, qemu-devel
On Thu, 9 May 2013 21:20:57 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> qmp_output_get_qobject() increments the qobject's reference count. Since
> we currently pass this straight into qobject_to_json() so we can feed
> the data into a QMP input visitor, we never actually free the underlying
> qobject when qmp_output_visitor_cleanup() is called. This causes leaks
> on all of the QMP serialization tests.
>
> Fix this by holding a pointer to the qobject and decref'ing it before
> returning from qmp_deserialize().
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
I've cherry-picked this one into the qmp branch for 1.5.
> ---
> tests/test-visitor-serialization.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
> index e84926f..8c8adac 100644
> --- a/tests/test-visitor-serialization.c
> +++ b/tests/test-visitor-serialization.c
> @@ -657,11 +657,16 @@ static void qmp_deserialize(void **native_out, void *datap,
> VisitorFunc visit, Error **errp)
> {
> QmpSerializeData *d = datap;
> - QString *output_json = qobject_to_json(qmp_output_get_qobject(d->qov));
> - QObject *obj = qobject_from_json(qstring_get_str(output_json));
> + QString *output_json;
> + QObject *obj_orig, *obj;
> +
> + obj_orig = qmp_output_get_qobject(d->qov);
> + output_json = qobject_to_json(obj_orig);
> + obj = qobject_from_json(qstring_get_str(output_json));
>
> QDECREF(output_json);
> d->qiv = qmp_input_visitor_new(obj);
> + qobject_decref(obj_orig);
> qobject_decref(obj);
> visit(qmp_input_get_visitor(d->qiv), native_out, errp);
> }
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 2:20 ` [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values Michael Roth
2013-05-10 11:55 ` Laszlo Ersek
2013-05-10 12:22 ` Eric Blake
@ 2013-05-10 15:17 ` Luiz Capitulino
2013-05-10 16:00 ` mdroth
2 siblings, 1 reply; 30+ messages in thread
From: Luiz Capitulino @ 2013-05-10 15:17 UTC (permalink / raw)
To: Michael Roth; +Cc: akong, lersek, qemu-devel
On Thu, 9 May 2013 21:20:58 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Currently our JSON parser assumes that numbers lacking a mantissa are
> integers and attempts to store them as QInt/int64 values. This breaks in
> the case where the number overflows/underflows int64 values (which is
> still valid JSON)
Anthony wanted to fix this by moving to another wire format :)
But, how this patch related to this series?
>
> Fix this by detecting such cases and using a QFloat to store the value
> instead.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qobject/json-parser.c | 26 +++++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 05279c1..4d14e71 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
> case JSON_STRING:
> obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
> break;
> - case JSON_INTEGER:
> - obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
> - break;
> + case JSON_INTEGER: {
> + /* A possibility exists that this is a whole-valued float where the
> + * mantissa was left out due to being 0 (.0). It's not a big deal to
> + * treat these as ints in the parser, so long as users of the
> + * resulting QObject know to expect a QInt in place of a QFloat in
> + * cases like these.
> + *
> + * However, in some cases these values will overflow/underflow a
> + * QInt/int64 container, thus we should assume these are to be handled
> + * as QFloats/doubles rather than silently changing their values.
> + *
> + * strtoll() indicates these instances by setting errno to ERANGE
> + */
> + int64_t value;
> +
> + errno = 0; /* strtoll doesn't set errno on success */
> + value = strtoll(token_get_value(token), NULL, 10);
> + if (errno != ERANGE) {
> + obj = QOBJECT(qint_from_int(value));
> + break;
> + }
> + /* fall through to JSON_FLOAT */
> + }
> case JSON_FLOAT:
> /* FIXME dependent on locale */
> obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), NULL)));
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
` (9 preceding siblings ...)
2013-05-10 2:21 ` [Qemu-devel] [PATCH 10/10] qapi: add native list coverage for QMP input " Michael Roth
@ 2013-05-10 15:30 ` Luiz Capitulino
2013-05-10 15:40 ` Laszlo Ersek
10 siblings, 1 reply; 30+ messages in thread
From: Luiz Capitulino @ 2013-05-10 15:30 UTC (permalink / raw)
To: Michael Roth; +Cc: akong, lersek, qemu-devel
On Thu, 9 May 2013 21:20:52 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> These patches apply on top of qemu.git master, and can also be obtained from:
> git://github.com/mdroth/qemu.git qapi-native-lists
>
> Sending this now since a number of series have popped up in the past that
> wanted this, and Amos has some pending patches (query-mac-tables) that rely
> on this as well.
>
> These patches add support for specifying lists of native qapi types
> (int/bool/str/number) like so:
>
> { 'type': 'foo',
> 'data': { 'bar': ['int'] }}
>
> for a 'bar' field that is a list of type 'int',
>
> { 'type': 'foo2',
> 'data': { 'bar2': ['str'] }}
>
> for a 'bar2' field that is a list of type 'str', and so on.
>
> This uses linked list types for the native C representations, just as we do
> for complex schema-defined types. In the future we may add schema annotations
> of some sort to specify a more natural/efficient array type for the C
> representations, but this should serve the majority of uses-cases for now.
Series looks good to me. I'd drop patch 06/10 if it's not required
for this series though.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types
2013-05-10 15:30 ` [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Luiz Capitulino
@ 2013-05-10 15:40 ` Laszlo Ersek
2013-05-10 15:43 ` Luiz Capitulino
0 siblings, 1 reply; 30+ messages in thread
From: Laszlo Ersek @ 2013-05-10 15:40 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, Michael Roth, qemu-devel
On 05/10/13 17:30, Luiz Capitulino wrote:
> On Thu, 9 May 2013 21:20:52 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>
>> These patches apply on top of qemu.git master, and can also be obtained from:
>> git://github.com/mdroth/qemu.git qapi-native-lists
>>
>> Sending this now since a number of series have popped up in the past that
>> wanted this, and Amos has some pending patches (query-mac-tables) that rely
>> on this as well.
>>
>> These patches add support for specifying lists of native qapi types
>> (int/bool/str/number) like so:
>>
>> { 'type': 'foo',
>> 'data': { 'bar': ['int'] }}
>>
>> for a 'bar' field that is a list of type 'int',
>>
>> { 'type': 'foo2',
>> 'data': { 'bar2': ['str'] }}
>>
>> for a 'bar2' field that is a list of type 'str', and so on.
>>
>> This uses linked list types for the native C representations, just as we do
>> for complex schema-defined types. In the future we may add schema annotations
>> of some sort to specify a more natural/efficient array type for the C
>> representations, but this should serve the majority of uses-cases for now.
>
> Series looks good to me. I'd drop patch 06/10 if it's not required
> for this series though.
I guess:
On 05/10/13 04:20, Michael Roth wrote:
> v1->v2:
> * fixed do-nothing float tests in pre-existing code and updated new
> unit tests accordingly (Laszlo)
> * added a fix for a bug in json parser that was exposed by above change
Laszlo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types
2013-05-10 15:40 ` Laszlo Ersek
@ 2013-05-10 15:43 ` Luiz Capitulino
0 siblings, 0 replies; 30+ messages in thread
From: Luiz Capitulino @ 2013-05-10 15:43 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: akong, Michael Roth, qemu-devel
On Fri, 10 May 2013 17:40:20 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/10/13 17:30, Luiz Capitulino wrote:
> > On Thu, 9 May 2013 21:20:52 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> >
> >> These patches apply on top of qemu.git master, and can also be obtained from:
> >> git://github.com/mdroth/qemu.git qapi-native-lists
> >>
> >> Sending this now since a number of series have popped up in the past that
> >> wanted this, and Amos has some pending patches (query-mac-tables) that rely
> >> on this as well.
> >>
> >> These patches add support for specifying lists of native qapi types
> >> (int/bool/str/number) like so:
> >>
> >> { 'type': 'foo',
> >> 'data': { 'bar': ['int'] }}
> >>
> >> for a 'bar' field that is a list of type 'int',
> >>
> >> { 'type': 'foo2',
> >> 'data': { 'bar2': ['str'] }}
> >>
> >> for a 'bar2' field that is a list of type 'str', and so on.
> >>
> >> This uses linked list types for the native C representations, just as we do
> >> for complex schema-defined types. In the future we may add schema annotations
> >> of some sort to specify a more natural/efficient array type for the C
> >> representations, but this should serve the majority of uses-cases for now.
> >
> > Series looks good to me. I'd drop patch 06/10 if it's not required
> > for this series though.
>
> I guess:
I see, although it's still a separate issue (which I'd personally fix
in a different series).
>
> On 05/10/13 04:20, Michael Roth wrote:
> > v1->v2:
> > * fixed do-nothing float tests in pre-existing code and updated new
> > unit tests accordingly (Laszlo)
> > * added a fix for a bug in json parser that was exposed by above change
>
> Laszlo
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support
2013-05-10 14:07 ` Luiz Capitulino
@ 2013-05-10 15:51 ` mdroth
0 siblings, 0 replies; 30+ messages in thread
From: mdroth @ 2013-05-10 15:51 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, lersek, qemu-devel
On Fri, May 10, 2013 at 10:07:45AM -0400, Luiz Capitulino wrote:
> On Thu, 9 May 2013 21:20:53 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>
> > Teach type generators about native types so they can generate the
> > appropriate linked list types.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > scripts/qapi-types.py | 43 ++++++++++++++++++++++++++++++++++++++++---
> > scripts/qapi.py | 21 +++++++++++++++++++++
> > 2 files changed, 61 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 9e19920..96cb26d 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -16,7 +16,18 @@ import os
> > import getopt
> > import errno
> >
> > -def generate_fwd_struct(name, members):
> > +def generate_fwd_struct(name, members, builtin_type=False):
> > + if builtin_type:
> > + return mcgen('''
> > +typedef struct %(name)sList
> > +{
> > + %(type)s value;
> > + struct %(name)sList *next;
> > +} %(name)sList;
> > +''',
>
> Sorry for the utterly minor comment, but as you're going to respin please
> add a newline before ''' so that we get the declarations properly separated
> when generated.
>
> > + type=c_type(name),
> > + name=name)
> > +
> > return mcgen('''
> > typedef struct %(name)s %(name)s;
> >
> > @@ -164,6 +175,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
> >
> > def generate_type_cleanup(name):
> > ret = mcgen('''
> > +
> > void qapi_free_%(type)s(%(c_type)s obj)
> > {
> > QapiDeallocVisitor *md;
> > @@ -184,8 +196,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
> >
> >
> > try:
> > - opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> > - ["source", "header", "prefix=", "output-dir="])
> > + opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> > + ["source", "header", "builtins",
> > + "prefix=", "output-dir="])
> > except getopt.GetoptError, err:
> > print str(err)
> > sys.exit(1)
> > @@ -197,6 +210,7 @@ h_file = 'qapi-types.h'
> >
> > do_c = False
> > do_h = False
> > +do_builtins = False
> >
> > for o, a in opts:
> > if o in ("-p", "--prefix"):
> > @@ -207,6 +221,8 @@ for o, a in opts:
> > do_c = True
> > elif o in ("-h", "--header"):
> > do_h = True
> > + elif o in ("-b", "--builtins"):
> > + do_builtins = True
> >
> > if not do_c and not do_h:
> > do_c = True
> > @@ -282,6 +298,11 @@ fdecl.write(mcgen('''
> > exprs = parse_schema(sys.stdin)
> > exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
> >
> > +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> > +for typename in builtin_types:
> > + fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
> > +fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> > +
> > for expr in exprs:
> > ret = "\n"
> > if expr.has_key('type'):
> > @@ -298,6 +319,22 @@ for expr in exprs:
> > continue
> > fdecl.write(ret)
> >
> > +# to avoid header dependency hell, we always generate declarations
> > +# for built-in types in our header files and simply guard them
> > +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> > +for typename in builtin_types:
> > + fdecl.write(generate_type_cleanup_decl(typename + "List"))
> > +fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
>
> I'm not sure I got why you're doing this. Is it because you're going to
> generate them in more .h files? This is a bit ugly :(
>
The issue is that things like the types generated from
qapi-schema-test.json or qga/qapi-schema.json may end up referencing
intList/strList/visit_type_intList/etc, which we'll also have declared for
use in the main qapi-schema.json. qapi-schema.json of course can't
depend on tests or qga, so we generate the definitions for the builtin types
when running the code generators on qapi-schema.json.
For everyone else, tests/qga/etc, to be able to use/link against those
definitions we need to include the declarations by either #include'ing
qapi-types.h/qapi-visit.h etc, or by always declaring them and simply
adding a guard.
In this case I've taken the latter approach since hard-coding a
reference in the code generators to header files created by separate calls
to the code generators seemed less modular. qapi-schema-test.json should
be capable of generating self-contained code if need be (and the only
reason we don't make it self-contained is that test-cases rely on
libqemuutil to build, which actually does have a dependency on
qapi-schema.json due to qemu-sockets.
> > +
> > +# ...this doesn't work for cases where we link in multiple objects that
> > +# have the functions defined, so we use -b option to provide control
> > +# over these cases
> > +if do_builtins:
> > + fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> > + for typename in builtin_types:
> > + fdef.write(generate_type_cleanup(typename + "List"))
> > + fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> > +
> > for expr in exprs:
> > ret = "\n"
> > if expr.has_key('type'):
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index afc5f32..0ac8c2b 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -11,6 +11,10 @@
> >
> > from ordereddict import OrderedDict
> >
> > +builtin_types = [
> > + 'str', 'int', 'number', 'bool'
> > +]
> > +
> > def tokenize(data):
> > while len(data):
> > ch = data[0]
> > @@ -242,3 +246,20 @@ def guardname(filename):
> > for substr in [".", " ", "-"]:
> > guard = guard.replace(substr, "_")
> > return guard.upper() + '_H'
> > +
> > +def guardstart(name):
> > + return mcgen('''
> > +
> > +#ifndef %(name)s
> > +#define %(name)s
> > +
> > +''',
> > + name=guardname(name))
> > +
> > +def guardend(name):
> > + return mcgen('''
> > +
> > +#endif /* %(name)s */
> > +
> > +''',
> > + name=guardname(name))
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
2013-05-10 15:17 ` Luiz Capitulino
@ 2013-05-10 16:00 ` mdroth
0 siblings, 0 replies; 30+ messages in thread
From: mdroth @ 2013-05-10 16:00 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, lersek, qemu-devel
On Fri, May 10, 2013 at 11:17:17AM -0400, Luiz Capitulino wrote:
> On Thu, 9 May 2013 21:20:58 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>
> > Currently our JSON parser assumes that numbers lacking a mantissa are
> > integers and attempts to store them as QInt/int64 values. This breaks in
> > the case where the number overflows/underflows int64 values (which is
> > still valid JSON)
>
> Anthony wanted to fix this by moving to another wire format :)
>
> But, how this patch related to this series?
In v1 Laszlo pointed out that the QFloat tests I added in
test-visitor-serialization were actually non-functional, and those tests were
based on pre-existing code. So I added a patch to fix the pre-existing
code as a pre-cursor to the new unit tests based on it. But fixing that code
exposed the json-parser bug, so I added that fix as a precursor to the
precursor :)
Same with mem leak fixes, etc, to ensure the tests were functional and
leak-free within this series.
>
> >
> > Fix this by detecting such cases and using a QFloat to store the value
> > instead.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > qobject/json-parser.c | 26 +++++++++++++++++++++++---
> > 1 file changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> > index 05279c1..4d14e71 100644
> > --- a/qobject/json-parser.c
> > +++ b/qobject/json-parser.c
> > @@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
> > case JSON_STRING:
> > obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
> > break;
> > - case JSON_INTEGER:
> > - obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
> > - break;
> > + case JSON_INTEGER: {
> > + /* A possibility exists that this is a whole-valued float where the
> > + * mantissa was left out due to being 0 (.0). It's not a big deal to
> > + * treat these as ints in the parser, so long as users of the
> > + * resulting QObject know to expect a QInt in place of a QFloat in
> > + * cases like these.
> > + *
> > + * However, in some cases these values will overflow/underflow a
> > + * QInt/int64 container, thus we should assume these are to be handled
> > + * as QFloats/doubles rather than silently changing their values.
> > + *
> > + * strtoll() indicates these instances by setting errno to ERANGE
> > + */
> > + int64_t value;
> > +
> > + errno = 0; /* strtoll doesn't set errno on success */
> > + value = strtoll(token_get_value(token), NULL, 10);
> > + if (errno != ERANGE) {
> > + obj = QOBJECT(qint_from_int(value));
> > + break;
> > + }
> > + /* fall through to JSON_FLOAT */
> > + }
> > case JSON_FLOAT:
> > /* FIXME dependent on locale */
> > obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), NULL)));
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code
2013-05-10 14:10 ` Luiz Capitulino
@ 2013-05-10 16:32 ` mdroth
2013-05-10 22:28 ` mdroth
0 siblings, 1 reply; 30+ messages in thread
From: mdroth @ 2013-05-10 16:32 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, lersek, qemu-devel
On Fri, May 10, 2013 at 10:10:03AM -0400, Luiz Capitulino wrote:
> On Thu, 9 May 2013 21:20:56 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>
> > Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
> > qapi-types.c/qapi-visit.c
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > Makefile | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 7dc0204..9695c9d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
> > # Build libraries
> >
> > libqemustub.a: $(stub-obj-y)
> > -libqemuutil.a: $(util-obj-y)
> > +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
>
> Don't we want this in for 1.5?
>
I don't think it's causing any issues currently since it's not causing
undefined reference errors upstream. users of libqemuutil that make use
of qemu-sockets seem to be pulling qapi-types/qapi-visit in through other
dependencies.
I only noticed it because I was attempting to generate the native list
code via tests/Makefile and running into redefinition conflicts with
qapi-types.o/qapi-visit.o, then noticed the qemu-sockets.o issue when I
attempted to remove the qapi-types/qapi-visit dependency from
tests/test-visitor-serialization
Now that we're generating the native list code from top-level Makefile,
it actually doesn't seem to be needed by this series anymore, so maybe
I'll pull it out for now. I think a better fix would be to have
qapi/Makefile.obj add these to $util-obj-y directly anyway.
> >
> > ######################################################################
> >
> > @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> >
> > qapi-types.c qapi-types.h :\
> > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." < $<, " GEN $@")
> > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -b < $<, " GEN $@")
> > qapi-visit.c qapi-visit.h :\
> > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> > - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." < $<, " GEN $@")
> > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@")
> > qmp-commands.h qmp-marshal.c :\
> > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> > $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@")
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code
2013-05-10 16:32 ` mdroth
@ 2013-05-10 22:28 ` mdroth
0 siblings, 0 replies; 30+ messages in thread
From: mdroth @ 2013-05-10 22:28 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, lersek, qemu-devel
On Fri, May 10, 2013 at 11:32:48AM -0500, mdroth wrote:
> On Fri, May 10, 2013 at 10:10:03AM -0400, Luiz Capitulino wrote:
> > On Thu, 9 May 2013 21:20:56 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> >
> > > Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs
> > > qapi-types.c/qapi-visit.c
> > >
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > ---
> > > Makefile | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 7dc0204..9695c9d 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
> > > # Build libraries
> > >
> > > libqemustub.a: $(stub-obj-y)
> > > -libqemuutil.a: $(util-obj-y)
> > > +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
> >
> > Don't we want this in for 1.5?
> >
>
> I don't think it's causing any issues currently since it's not causing
> undefined reference errors upstream. users of libqemuutil that make use
> of qemu-sockets seem to be pulling qapi-types/qapi-visit in through other
> dependencies.
>
> I only noticed it because I was attempting to generate the native list
> code via tests/Makefile and running into redefinition conflicts with
> qapi-types.o/qapi-visit.o, then noticed the qemu-sockets.o issue when I
> attempted to remove the qapi-types/qapi-visit dependency from
> tests/test-visitor-serialization
>
> Now that we're generating the native list code from top-level Makefile,
> it actually doesn't seem to be needed by this series anymore, so maybe
> I'll pull it out for now. I think a better fix would be to have
> qapi/Makefile.obj add these to $util-obj-y directly anyway.
^ this wasn't quite right, we do have a new dependency on
qapi-types/qapi-visit in the visitor tests for native list types, so
I've left this in place.
>
> > >
> > > ######################################################################
> > >
> > > @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> > >
> > > qapi-types.c qapi-types.h :\
> > > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > > - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." < $<, " GEN $@")
> > > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -b < $<, " GEN $@")
> > > qapi-visit.c qapi-visit.h :\
> > > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> > > - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." < $<, " GEN $@")
> > > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@")
> > > qmp-commands.h qmp-marshal.c :\
> > > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> > > $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@")
> >
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2013-05-10 22:30 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-10 2:20 [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support Michael Roth
2013-05-10 3:04 ` Amos Kong
2013-05-10 11:32 ` mdroth
2013-05-10 14:07 ` Luiz Capitulino
2013-05-10 15:51 ` mdroth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 02/10] qapi: qapi-visit.py, fix list handling for union types Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 03/10] qapi: qapi-visit.py, native list support Michael Roth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code Michael Roth
2013-05-10 14:10 ` Luiz Capitulino
2013-05-10 16:32 ` mdroth
2013-05-10 22:28 ` mdroth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests Michael Roth
2013-05-10 15:14 ` Luiz Capitulino
2013-05-10 2:20 ` [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values Michael Roth
2013-05-10 11:55 ` Laszlo Ersek
2013-05-10 12:22 ` Eric Blake
2013-05-10 12:47 ` Laszlo Ersek
2013-05-10 13:30 ` mdroth
2013-05-10 14:08 ` Eric Blake
2013-05-10 14:51 ` mdroth
2013-05-10 15:17 ` Luiz Capitulino
2013-05-10 16:00 ` mdroth
2013-05-10 2:20 ` [Qemu-devel] [PATCH 07/10] qapi: fix visitor serialization tests for numbers/doubles Michael Roth
2013-05-10 2:21 ` [Qemu-devel] [PATCH 08/10] qapi: add native list coverage for visitor serialization tests Michael Roth
2013-05-10 2:21 ` [Qemu-devel] [PATCH 09/10] qapi: add native list coverage for QMP output visitor tests Michael Roth
2013-05-10 2:21 ` [Qemu-devel] [PATCH 10/10] qapi: add native list coverage for QMP input " Michael Roth
2013-05-10 15:30 ` [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types Luiz Capitulino
2013-05-10 15:40 ` Laszlo Ersek
2013-05-10 15:43 ` Luiz Capitulino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.