diff mbox

[V7,09/11] qapi script: do not allow string discriminator

Message ID 1392875695-15627-10-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia Feb. 20, 2014, 5:54 a.m. UTC
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 docs/qapi-code-gen.txt                  |    8 +++-----
 scripts/qapi.py                         |    6 ++++++
 tests/qapi-schema/qapi-schema-test.json |    9 ++++++---
 tests/qapi-schema/qapi-schema-test.out  |    5 +++--
 tests/test-qmp-input-strict.c           |    5 ++++-
 tests/test-qmp-input-visitor.c          |   10 +++++++---
 tests/test-qmp-output-visitor.c         |   10 ++++++----
 7 files changed, 35 insertions(+), 18 deletions(-)

Comments

Markus Armbruster Feb. 20, 2014, 4:50 p.m. UTC | #1
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  docs/qapi-code-gen.txt                  |    8 +++-----
>  scripts/qapi.py                         |    6 ++++++
>  tests/qapi-schema/qapi-schema-test.json |    9 ++++++---
>  tests/qapi-schema/qapi-schema-test.out  |    5 +++--
>  tests/test-qmp-input-strict.c           |    5 ++++-
>  tests/test-qmp-input-visitor.c          |   10 +++++++---
>  tests/test-qmp-output-visitor.c         |   10 ++++++----
>  7 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index a2e7921..c92add9 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -123,11 +123,9 @@ And it looks like this on the wire:
>  
>  Flat union types avoid the nesting on the wire. They are used whenever a
>  specific field of the base type is declared as the discriminator ('type' is
> -then no longer generated). The discriminator can be a string field or a
> -predefined enum field. If it is a string field, a hidden enum type will be
> -generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
> -will be done to verify the correctness. It is recommended to use an enum field.
> -The above example can then be modified as follows:
> +then no longer generated). The discriminator should be a predefined enum field,
> +and a compile time check will be done to verify the correctness. The above
> +example can then be modified as follows:

Suggest:

    The discriminator must be of enumeration type.  The above example...

>  
>   { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
>   { 'type': 'BlockdevCommonOptions',
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 2a5eb59..c3c118b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -215,6 +215,7 @@ def check_union(expr_elem):
>      enum_define = discriminator_find_enum_define(expr_elem)
>      name = expr_elem['expr']['union']
>      members = expr_elem['expr']['data']
> +    discriminator = expr_elem['expr'].get('discriminator')
>      if enum_define:
>          for key in members:
>              if not key in enum_define['enum_values']:
> @@ -228,6 +229,11 @@ def check_union(expr_elem):
>                                      "Enum value '%s' is not covered by a "
>                                      "branch of union '%s'" %
>                                      (key, name))
> +    elif discriminator:
> +        # Do not allow string discriminator
> +        raise QAPIExprError(expr_elem,
> +                            "Discriminator '%s' is not a pre-defined enum "
> +                            "type" % discriminator)

I have no idea what "pre-defined" means here :)

Suggest:

    "Discriminator '%s' must be of enumeration type"

>  
>  def check_exprs(schema):
>      for expr_elem in schema.exprs:
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 471ba47..818c06d 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -37,10 +37,13 @@
>    'base': 'UserDefZero',
>    'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
>  
> +{ 'type': 'UserDefUnionBase',
> +  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
> +
>  { 'union': 'UserDefFlatUnion',
> -  'base': 'UserDefOne',
> -  'discriminator': 'string',
> -  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
> +  'base': 'UserDefUnionBase',
> +  'discriminator': 'enum1',
> +  'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } }
>  # FIXME generated struct UserDefFlatUnion has members for direct base
>  # UserDefOne, but lacks members for indirect base UserDefZero
>  
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 01685d4..6cd03f3 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -7,7 +7,8 @@
>   OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
>   OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
>   OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
> - OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefOne'), ('discriminator', 'string'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
> + OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
> + OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
>   OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
>   OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
>   OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
> @@ -17,7 +18,6 @@
>   OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
>  [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
>   {'enum_name': 'UserDefUnionKind', 'enum_values': None},
> - {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
>   {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
>   {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
>  [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
> @@ -27,4 +27,5 @@
>   OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
>   OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
>   OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
> + OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
>   OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
> index 09fc1ef..c715a50 100644
> --- a/tests/test-qmp-input-strict.c
> +++ b/tests/test-qmp-input-strict.c
> @@ -146,7 +146,10 @@ static void test_validate_union_flat(TestInputVisitorData *data,
>      Visitor *v;
>      Error *errp = NULL;
>  
> -    v = validate_test_init(data, "{ 'string': 'a', 'boolean': true }");
> +    v = validate_test_init(data,
> +                           "{ 'enum1': 'value1',\
> +                              'string': 'str', \
> +                              'boolean': true }");
>      /* TODO when generator bug is fixed, add 'integer': 41 */
>  

Please use string concatenation rather than line continuation to avoid
long lines, like this:

       v = validate_test_init(data,
                              "{ 'enum1': 'value1', "
                              "'string': 'str', "
                              "'boolean': true }");

The existing tests don't bother to avoid long lines, but that's not your
fault.

>      visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index f1ab541..b549746 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -310,14 +310,18 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
>      Error *err = NULL;
>      UserDefFlatUnion *tmp;
>  
> -    v = visitor_input_test_init(data, "{ 'string': 'a', 'boolean': true }");
> +    v = visitor_input_test_init(data,
> +                                "{ 'enum1': 'value1',\
> +                                   'string': 'str', \
> +                                   'boolean': true }");

Likewise.

>      /* TODO when generator bug is fixed, add 'integer': 41 */
>  
>      visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
>      g_assert(err == NULL);
> -    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_A);
> +    g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE1);
> +    g_assert_cmpstr(tmp->string, ==, "str");
>      /* TODO g_assert_cmpint(tmp->integer, ==, 41); */
> -    g_assert_cmpint(tmp->a->boolean, ==, true);
> +    g_assert_cmpint(tmp->value1->boolean, ==, true);
>      qapi_free_UserDefFlatUnion(tmp);
>  }
>  
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 5613396..6f4abb7 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -449,10 +449,11 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
>      Error *err = NULL;
>  
>      UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
> -    tmp->kind = USER_DEF_UNION_KIND_A;
> -    tmp->a = g_malloc0(sizeof(UserDefA));
> +    tmp->kind = ENUM_ONE_VALUE1;
> +    tmp->string = g_strdup("str");
> +    tmp->value1 = g_malloc0(sizeof(UserDefA));
>      /* TODO when generator bug is fixed: tmp->integer = 41; */
> -    tmp->a->boolean = true;
> +    tmp->value1->boolean = true;
>  
>      visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
>      g_assert(err == NULL);
> @@ -461,7 +462,8 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
>      g_assert(qobject_type(arg) == QTYPE_QDICT);
>      qdict = qobject_to_qdict(arg);
>  
> -    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "a");
> +    g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
> +    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
>      /* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
>      g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
diff mbox

Patch

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index a2e7921..c92add9 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -123,11 +123,9 @@  And it looks like this on the wire:
 
 Flat union types avoid the nesting on the wire. They are used whenever a
 specific field of the base type is declared as the discriminator ('type' is
-then no longer generated). The discriminator can be a string field or a
-predefined enum field. If it is a string field, a hidden enum type will be
-generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
-will be done to verify the correctness. It is recommended to use an enum field.
-The above example can then be modified as follows:
+then no longer generated). The discriminator should be a predefined enum field,
+and a compile time check will be done to verify the correctness. The above
+example can then be modified as follows:
 
  { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
  { 'type': 'BlockdevCommonOptions',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 2a5eb59..c3c118b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -215,6 +215,7 @@  def check_union(expr_elem):
     enum_define = discriminator_find_enum_define(expr_elem)
     name = expr_elem['expr']['union']
     members = expr_elem['expr']['data']
+    discriminator = expr_elem['expr'].get('discriminator')
     if enum_define:
         for key in members:
             if not key in enum_define['enum_values']:
@@ -228,6 +229,11 @@  def check_union(expr_elem):
                                     "Enum value '%s' is not covered by a "
                                     "branch of union '%s'" %
                                     (key, name))
+    elif discriminator:
+        # Do not allow string discriminator
+        raise QAPIExprError(expr_elem,
+                            "Discriminator '%s' is not a pre-defined enum "
+                            "type" % discriminator)
 
 def check_exprs(schema):
     for expr_elem in schema.exprs:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 471ba47..818c06d 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -37,10 +37,13 @@ 
   'base': 'UserDefZero',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
+{ 'type': 'UserDefUnionBase',
+  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
+
 { 'union': 'UserDefFlatUnion',
-  'base': 'UserDefOne',
-  'discriminator': 'string',
-  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+  'base': 'UserDefUnionBase',
+  'discriminator': 'enum1',
+  'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } }
 # FIXME generated struct UserDefFlatUnion has members for direct base
 # UserDefOne, but lacks members for indirect base UserDefZero
 
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 01685d4..6cd03f3 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -7,7 +7,8 @@ 
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
- OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefOne'), ('discriminator', 'string'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
  OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
@@ -17,7 +18,6 @@ 
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
 [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
  {'enum_name': 'UserDefUnionKind', 'enum_values': None},
- {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
@@ -27,4 +27,5 @@ 
  OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 09fc1ef..c715a50 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -146,7 +146,10 @@  static void test_validate_union_flat(TestInputVisitorData *data,
     Visitor *v;
     Error *errp = NULL;
 
-    v = validate_test_init(data, "{ 'string': 'a', 'boolean': true }");
+    v = validate_test_init(data,
+                           "{ 'enum1': 'value1',\
+                              'string': 'str', \
+                              'boolean': true }");
     /* TODO when generator bug is fixed, add 'integer': 41 */
 
     visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index f1ab541..b549746 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -310,14 +310,18 @@  static void test_visitor_in_union_flat(TestInputVisitorData *data,
     Error *err = NULL;
     UserDefFlatUnion *tmp;
 
-    v = visitor_input_test_init(data, "{ 'string': 'a', 'boolean': true }");
+    v = visitor_input_test_init(data,
+                                "{ 'enum1': 'value1',\
+                                   'string': 'str', \
+                                   'boolean': true }");
     /* TODO when generator bug is fixed, add 'integer': 41 */
 
     visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
     g_assert(err == NULL);
-    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_A);
+    g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE1);
+    g_assert_cmpstr(tmp->string, ==, "str");
     /* TODO g_assert_cmpint(tmp->integer, ==, 41); */
-    g_assert_cmpint(tmp->a->boolean, ==, true);
+    g_assert_cmpint(tmp->value1->boolean, ==, true);
     qapi_free_UserDefFlatUnion(tmp);
 }
 
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 5613396..6f4abb7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -449,10 +449,11 @@  static void test_visitor_out_union_flat(TestOutputVisitorData *data,
     Error *err = NULL;
 
     UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
-    tmp->kind = USER_DEF_UNION_KIND_A;
-    tmp->a = g_malloc0(sizeof(UserDefA));
+    tmp->kind = ENUM_ONE_VALUE1;
+    tmp->string = g_strdup("str");
+    tmp->value1 = g_malloc0(sizeof(UserDefA));
     /* TODO when generator bug is fixed: tmp->integer = 41; */
-    tmp->a->boolean = true;
+    tmp->value1->boolean = true;
 
     visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
     g_assert(err == NULL);
@@ -461,7 +462,8 @@  static void test_visitor_out_union_flat(TestOutputVisitorData *data,
     g_assert(qobject_type(arg) == QTYPE_QDICT);
     qdict = qobject_to_qdict(arg);
 
-    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "a");
+    g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
+    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
     /* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
     g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);