[Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Fam Zheng]

Please first take a look at patch 7 to see what is supported by this series.

Patch 1 ~ 3 allows some useful basic types in schema.

Patch 4 ~ 6 implements the new syntax.

Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
reducing the cleanness of the syntax. We should get rid of both of them in long
term. Here, this series compromises on this and introduces '@arg' because:

  - We have to distinguish the argument property dictionary from nested struct:

    I.e.:

        'data': {
            'arg1': { 'member1': 'int', 'member2': 'str' }
            '@arg2': { 'type': 'int', 'default': 100 }
         }

    Until we completely drop and forbid the 'arg1' nested struct use case.

  - Forbidding 'arg1' it's doable, but doing it now means we pull in many
    distractive patches to this series.

    Basically we need to:

    * converting existing nested structs to named structs in qapi-schema.json.

    * converting C code that references these structs because member structures
      become member structure pointers.

    * coverting qapi-schema test cases on nested structure.

    * updating docs/qapi-code-gen.txt.

  - So let's focus on the default value requirement for now, because it's not
    making the above change harder.

Test cases, documentation, more data types of default value, and conversion of
more qmp commands will come later, if we could agree on the schema design and
framework.

Thanks,
Fam


Fam Zheng (7):
  qapi: Allow decimal values
  qapi: Allow true, false and null in schema json
  tests: Add decimal test cases for qapi-schema
  qapi: Add c_val(t, val) for int
  qapi: Add @arg property dictionary syntax
  qapi: Initialize argument value in generated code if has 'default'
  qmp: Convert block-commit speed to arg property dict

 blockdev.c                                   |  6 +--
 qapi-schema.json                             |  2 +-
 scripts/qapi-commands.py                     | 55 +++++++++++++++----------
 scripts/qapi-types.py                        |  2 +-
 scripts/qapi-visit.py                        |  4 +-
 scripts/qapi.py                              | 60 +++++++++++++++++++++++++---
 tests/Makefile                               |  3 +-
 tests/qapi-schema/integers-leading-zero.err  |  1 +
 tests/qapi-schema/integers-leading-zero.exit |  1 +
 tests/qapi-schema/integers-leading-zero.json |  1 +
 tests/qapi-schema/integers-leading-zero.out  |  0
 tests/qapi-schema/integers-overflow.err      |  1 +
 tests/qapi-schema/integers-overflow.exit     |  1 +
 tests/qapi-schema/integers-overflow.json     |  1 +
 tests/qapi-schema/integers-overflow.out      |  0
 tests/qapi-schema/integers.err               |  0
 tests/qapi-schema/integers.exit              |  1 +
 tests/qapi-schema/integers.json              | 10 +++++
 tests/qapi-schema/integers.out               |  3 ++
 19 files changed, 117 insertions(+), 35 deletions(-)
 create mode 100644 tests/qapi-schema/integers-leading-zero.err
 create mode 100644 tests/qapi-schema/integers-leading-zero.exit
 create mode 100644 tests/qapi-schema/integers-leading-zero.json
 create mode 100644 tests/qapi-schema/integers-leading-zero.out
 create mode 100644 tests/qapi-schema/integers-overflow.err
 create mode 100644 tests/qapi-schema/integers-overflow.exit
 create mode 100644 tests/qapi-schema/integers-overflow.json
 create mode 100644 tests/qapi-schema/integers-overflow.out
 create mode 100644 tests/qapi-schema/integers.err
 create mode 100644 tests/qapi-schema/integers.exit
 create mode 100644 tests/qapi-schema/integers.json
 create mode 100644 tests/qapi-schema/integers.out

-- 
1.9.2

[Qemu-devel] [RFC PATCH v2 1/7] qapi: Allow decimal values

[Fam Zheng]

This allows giving decimal constants in the schema as expr.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scripts/qapi.py | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0265b40..4c945ad 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -157,6 +157,21 @@ class QAPISchema:
                         return
                     else:
                         string += ch
+            elif self.tok in "-0123456789":
+                val = self.tok
+                while self.src[self.cursor] in "0123456789":
+                    val += self.src[self.cursor]
+                    self.cursor += 1
+                try:
+                    if val.startswith("0") and len(val) > 1:
+                        raise Exception("Leading zero for non-zero integer")
+                    self.val = int(val)
+                    if self.val > 0x7fffffffffffffffL or self.val < -0x7fffffffffffffffL - 1:
+                        raise Exception("Value too big")
+                    return
+                except Exception, e:
+                    raise QAPISchemaError(self, 'Invalid number "%s": %s' % (val, e))
+
             elif self.tok == '\n':
                 if self.cursor == len(self.src):
                     self.tok = None
@@ -196,8 +211,8 @@ class QAPISchema:
         if self.tok == ']':
             self.accept()
             return expr
-        if not self.tok in [ '{', '[', "'" ]:
-            raise QAPISchemaError(self, 'Expected "{", "[", "]" or string')
+        if not self.tok in "{['-0123456789":
+            raise QAPISchemaError(self, 'Expected "{", "[", "]", string or number')
         while True:
             expr.append(self.get_expr(True))
             if self.tok == ']':
@@ -219,6 +234,9 @@ class QAPISchema:
         elif self.tok == "'":
             expr = self.val
             self.accept()
+        elif self.tok in "-0123456789":
+            expr = self.val
+            self.accept()
         else:
             raise QAPISchemaError(self, 'Expected "{", "[" or string')
         return expr
-- 
1.9.2

[Qemu-devel] [RFC PATCH v2 2/7] qapi: Allow true, false and null in schema json

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scripts/qapi.py | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4c945ad..0f275f3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -171,7 +171,20 @@ class QAPISchema:
                     return
                 except Exception, e:
                     raise QAPISchemaError(self, 'Invalid number "%s": %s' % (val, e))
-
+            elif self.tok in "tfn":
+                val = self.src[self.cursor - 1:]
+                if val.startswith("true"):
+                    self.val = True
+                    self.cursor += 3
+                    return
+                elif val.startswith("false"):
+                    self.val = False
+                    self.cursor += 4
+                    return
+                elif val.startswith("null"):
+                    self.val = None
+                    self.cursor += 3
+                    return
             elif self.tok == '\n':
                 if self.cursor == len(self.src):
                     self.tok = None
@@ -211,8 +224,8 @@ class QAPISchema:
         if self.tok == ']':
             self.accept()
             return expr
-        if not self.tok in "{['-0123456789":
-            raise QAPISchemaError(self, 'Expected "{", "[", "]", string or number')
+        if not self.tok in "{['-0123456789tfn":
+            raise QAPISchemaError(self, 'Expected "{", "[", "]", string, number, boolean or "null"')
         while True:
             expr.append(self.get_expr(True))
             if self.tok == ']':
@@ -231,10 +244,7 @@ class QAPISchema:
         elif self.tok == '[':
             self.accept()
             expr = self.get_values()
-        elif self.tok == "'":
-            expr = self.val
-            self.accept()
-        elif self.tok in "-0123456789":
+        elif self.tok in "'0123456789-tfn":
             expr = self.val
             self.accept()
         else:
-- 
1.9.2

[Qemu-devel] [RFC PATCH v2 3/7] tests: Add decimal test cases for qapi-schema

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/Makefile                               |  3 ++-
 tests/qapi-schema/integers-leading-zero.err  |  1 +
 tests/qapi-schema/integers-leading-zero.exit |  1 +
 tests/qapi-schema/integers-leading-zero.json |  1 +
 tests/qapi-schema/integers-leading-zero.out  |  0
 tests/qapi-schema/integers-overflow.err      |  1 +
 tests/qapi-schema/integers-overflow.exit     |  1 +
 tests/qapi-schema/integers-overflow.json     |  1 +
 tests/qapi-schema/integers-overflow.out      |  0
 tests/qapi-schema/integers.err               |  0
 tests/qapi-schema/integers.exit              |  1 +
 tests/qapi-schema/integers.json              | 10 ++++++++++
 tests/qapi-schema/integers.out               |  3 +++
 13 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 tests/qapi-schema/integers-leading-zero.err
 create mode 100644 tests/qapi-schema/integers-leading-zero.exit
 create mode 100644 tests/qapi-schema/integers-leading-zero.json
 create mode 100644 tests/qapi-schema/integers-leading-zero.out
 create mode 100644 tests/qapi-schema/integers-overflow.err
 create mode 100644 tests/qapi-schema/integers-overflow.exit
 create mode 100644 tests/qapi-schema/integers-overflow.json
 create mode 100644 tests/qapi-schema/integers-overflow.out
 create mode 100644 tests/qapi-schema/integers.err
 create mode 100644 tests/qapi-schema/integers.exit
 create mode 100644 tests/qapi-schema/integers.json
 create mode 100644 tests/qapi-schema/integers.out

diff --git a/tests/Makefile b/tests/Makefile
index 9f7ca61..d3c4897 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -194,7 +194,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
         include-simple.json include-relpath.json include-format-err.json \
         include-non-file.json include-no-file.json include-before-err.json \
         include-nested-err.json include-self-cycle.json include-cycle.json \
-        include-repetition.json)
+        include-repetition.json \
+        integers.json integers-overflow.json integers-leading-zero.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
diff --git a/tests/qapi-schema/integers-leading-zero.err b/tests/qapi-schema/integers-leading-zero.err
new file mode 100644
index 0000000..3a14e38
--- /dev/null
+++ b/tests/qapi-schema/integers-leading-zero.err
@@ -0,0 +1 @@
+tests/qapi-schema/integers-leading-zero.json:1:12: Invalid number "00": Leading zero for non-zero integer
diff --git a/tests/qapi-schema/integers-leading-zero.exit b/tests/qapi-schema/integers-leading-zero.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/integers-leading-zero.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/integers-leading-zero.json b/tests/qapi-schema/integers-leading-zero.json
new file mode 100644
index 0000000..8265fa8
--- /dev/null
+++ b/tests/qapi-schema/integers-leading-zero.json
@@ -0,0 +1 @@
+{ 'value': 00 }
diff --git a/tests/qapi-schema/integers-leading-zero.out b/tests/qapi-schema/integers-leading-zero.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/integers-overflow.err b/tests/qapi-schema/integers-overflow.err
new file mode 100644
index 0000000..6ace550
--- /dev/null
+++ b/tests/qapi-schema/integers-overflow.err
@@ -0,0 +1 @@
+tests/qapi-schema/integers-overflow.json:1:12: Invalid number "-1000000000000000000000000000000": Value too big
diff --git a/tests/qapi-schema/integers-overflow.exit b/tests/qapi-schema/integers-overflow.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/integers-overflow.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/integers-overflow.json b/tests/qapi-schema/integers-overflow.json
new file mode 100644
index 0000000..5daff7c
--- /dev/null
+++ b/tests/qapi-schema/integers-overflow.json
@@ -0,0 +1 @@
+{ 'value': -1000000000000000000000000000000 }
diff --git a/tests/qapi-schema/integers-overflow.out b/tests/qapi-schema/integers-overflow.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/integers.err b/tests/qapi-schema/integers.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/integers.exit b/tests/qapi-schema/integers.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/integers.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/integers.json b/tests/qapi-schema/integers.json
new file mode 100644
index 0000000..2341b9e
--- /dev/null
+++ b/tests/qapi-schema/integers.json
@@ -0,0 +1,10 @@
+{ 'data': [
+    0,
+    1,
+    10,
+    -1,
+    -3980000,
+    100,
+    132565
+] }
+
diff --git a/tests/qapi-schema/integers.out b/tests/qapi-schema/integers.out
new file mode 100644
index 0000000..2cc62c5
--- /dev/null
+++ b/tests/qapi-schema/integers.out
@@ -0,0 +1,3 @@
+[OrderedDict([('data', [0, 1, 10, -1, -3980000, 100, 132565])])]
+[]
+[]
-- 
1.9.2

[Qemu-devel] [RFC PATCH v2 4/7] qapi: Add c_val(t, val) for int

[Fam Zheng]

c_val should generate the C value for a value in a given type. Only
handle int for now.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scripts/qapi.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0f275f3..ca2f1cc 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -498,6 +498,12 @@ def find_enum(name):
 def is_enum(name):
     return find_enum(name) != None
 
+def c_val(t, val):
+    if t == 'int':
+        return val
+    else:
+        assert False, "Unknown type: %s" % t
+
 def c_type(name):
     if name == 'str':
         return 'char *'
-- 
1.9.2

[Qemu-devel] [RFC PATCH v2 5/7] qapi: Add @arg property dictionary syntax

[Fam Zheng]

parse_args now yields one more item in an iteration - default. It is the
default value for the argument, that is parsed from

    'data': { '@ARG_NAME': { PROP_NAME : PROP_VALUE } }

syntax added into qapi schema.

ARG_NAME is the name of argument. PROP_NAME is the property name of
argument, currently can be 'type', 'optional' or 'default'.

'type' defines the type of the argument, as what we have in:

    'data': { 'ARG_NAME': 'TYPE_NAME' }

'optional' can be true or false.

'default' gives the default value of argument. if 'default' is set,
'optional' is ignored and the qmp handler will always get a value, hence
parse_args yields an False, so that has_xxx parameter is not generated
in the handler prototype. Later, in qapi-commands.py, the value will be
used if user doesn't specify one.

See the coming change on block-commit for example of how qmp commands
can make use of this.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scripts/qapi-commands.py |  8 ++++----
 scripts/qapi-types.py    |  2 +-
 scripts/qapi-visit.py    |  4 ++--
 scripts/qapi.py          | 20 ++++++++++++++++++--
 4 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 386f17e..05904f9 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -28,7 +28,7 @@ def type_visitor(name):
 
 def generate_command_decl(name, args, ret_type):
     arglist=""
-    for argname, argtype, optional, structured in parse_args(args):
+    for argname, argtype, optional, structured, default in parse_args(args):
         argtype = c_type(argtype)
         if argtype == "char *":
             argtype = "const char *"
@@ -55,7 +55,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
     retval=""
     if ret_type:
         retval = "retval = "
-    for argname, argtype, optional, structured in parse_args(args):
+    for argname, argtype, optional, structured, default in parse_args(args):
         if optional:
             arglist += "has_%s, " % c_var(argname)
         arglist += "%s, " % (c_var(argname))
@@ -98,7 +98,7 @@ Visitor *v;
 def gen_visitor_input_vars_decl(args):
     ret = ""
     push_indent()
-    for argname, argtype, optional, structured in parse_args(args):
+    for argname, argtype, optional, structured, default in parse_args(args):
         if optional:
             ret += mcgen('''
 bool has_%(argname)s = false;
@@ -141,7 +141,7 @@ v = qapi_dealloc_get_visitor(md);
 v = qmp_input_get_visitor(mi);
 ''')
 
-    for argname, argtype, optional, structured in parse_args(args):
+    for argname, argtype, optional, structured, default in parse_args(args):
         if optional:
             ret += mcgen('''
 visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b463232..ba3aa2b 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -63,7 +63,7 @@ typedef struct %(name)sList
 def generate_struct_fields(members):
     ret = ''
 
-    for argname, argentry, optional, structured in parse_args(members):
+    for argname, argentry, optional, structured, default in parse_args(members):
         if optional:
             ret += mcgen('''
     bool has_%(c_name)s;
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 06a79f1..4d9ee66 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -51,7 +51,7 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base =
     else:
         full_name = "%s_%s" % (name, fn_prefix)
 
-    for argname, argentry, optional, structured in parse_args(members):
+    for argname, argentry, optional, structured, default in parse_args(members):
         if structured:
             if not fn_prefix:
                 nested_fn_prefix = argname
@@ -94,7 +94,7 @@ if (err) {
                      c_prefix=c_var(field_prefix),
                      type=type_name(base), c_name=c_var('base'))
 
-    for argname, argentry, optional, structured in parse_args(members):
+    for argname, argentry, optional, structured, default in parse_args(members):
         if optional:
             ret += mcgen('''
 visit_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
diff --git a/scripts/qapi.py b/scripts/qapi.py
index ca2f1cc..fbe3ce3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -384,13 +384,29 @@ def parse_args(typeinfo):
         argname = member
         argentry = typeinfo[member]
         optional = False
+        argtype = argentry
+        default = None
         structured = False
         if member.startswith('*'):
             argname = member[1:]
             optional = True
+        if member.startswith('@'):
+            if not isinstance(argentry, OrderedDict):
+                raise Exception("Expecting property dictionary for argument '%s'" % member)
+            # Parse argument property dict
+            argname = member[1:]
+            argtype = argentry.get("type", None)
+            optional = argentry.get("optional", False)
+            default = argentry.get("default", None)
+            if default is not None:
+                optional = False
         if isinstance(argentry, OrderedDict):
-            structured = True
-        yield (argname, argentry, optional, structured)
+                structured = True
+
+        else:
+            argtype = argentry
+
+        yield (argname, argtype, optional, structured, default)
 
 def de_camel_case(name):
     new_name = ''
-- 
1.9.2

[Qemu-devel] [RFC PATCH v2 6/7] qapi: Initialize argument value in generated code if has 'default'

[Fam Zheng]

Input marshalling code generation is modified.

If an argument has default, initialize its value in generated code to
that default. If this argument is specified by user input, visit
argument with deallocate visitor, before input visitor, so that the
initial value is deallocated.

This moves initialization of md, mi, vd, vi before input block as
necessary, and moves cleanup of mi aside cleanup of md, for symmetry.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scripts/qapi-commands.py | 47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 05904f9..62ab51a 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -87,8 +87,8 @@ def gen_visitor_input_containers_decl(args, obj):
     if len(args) > 0:
         ret += mcgen('''
 QmpInputVisitor *mi = qmp_input_visitor_new_strict(%(obj)s);
-QapiDeallocVisitor *md;
-Visitor *v;
+QapiDeallocVisitor *md = qapi_dealloc_visitor_new();
+Visitor *vi, *vd;
 ''',
                      obj=obj)
     pop_indent()
@@ -99,12 +99,19 @@ def gen_visitor_input_vars_decl(args):
     ret = ""
     push_indent()
     for argname, argtype, optional, structured, default in parse_args(args):
-        if optional:
+        has_default = default is not None
+        if optional or has_default:
             ret += mcgen('''
 bool has_%(argname)s = false;
 ''',
                          argname=c_var(argname))
-        if c_type(argtype).endswith("*"):
+        if has_default:
+            ret += mcgen('''
+%(argtype)s %(argname)s = %(argval)s;
+''',
+                         argname=c_var(argname), argtype=c_type(argtype),
+                         argval=c_val(argtype, default))
+        elif c_type(argtype).endswith("*"):
             ret += mcgen('''
 %(argtype)s %(argname)s = NULL;
 ''',
@@ -122,6 +129,7 @@ def gen_visitor_input_block(args, dealloc=False):
     ret = ""
     errparg = '&local_err'
     errarg = 'local_err'
+    vvar = 'vi'
 
     if len(args) == 0:
         return ret
@@ -131,35 +139,41 @@ def gen_visitor_input_block(args, dealloc=False):
     if dealloc:
         errparg = 'NULL'
         errarg = None;
-        ret += mcgen('''
-qmp_input_visitor_cleanup(mi);
-md = qapi_dealloc_visitor_new();
-v = qapi_dealloc_get_visitor(md);
-''')
+        vvar = 'vd'
     else:
         ret += mcgen('''
-v = qmp_input_get_visitor(mi);
+vi = qmp_input_get_visitor(mi);
+vd = qapi_dealloc_get_visitor(md);
 ''')
 
     for argname, argtype, optional, structured, default in parse_args(args):
-        if optional:
+        has_default = default is not None
+        if optional or has_default:
             ret += mcgen('''
-visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
+visit_optional(%(v)s, &has_%(c_name)s, "%(name)s", %(errp)s);
 ''',
-                         c_name=c_var(argname), name=argname, errp=errparg)
+                         v=vvar, c_name=c_var(argname), name=argname, errp=errparg)
             ret += gen_err_check(errarg)
             ret += mcgen('''
 if (has_%(c_name)s) {
 ''',
                          c_name=c_var(argname))
             push_indent()
+            if not dealloc and has_default:
+                # For input block, we should deallocate argument that is
+                # initialized with a defualt value, before reading in from input
+                ret += mcgen('''
+%(visitor)s(vd, &%(c_name)s, "%(name)s", NULL);
+''',
+                         c_name=c_var(argname), name=argname, argtype=argtype,
+                         visitor=type_visitor(argtype), errp=errparg)
         ret += mcgen('''
-%(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s);
+%(visitor)s(%(v)s, &%(c_name)s, "%(name)s", %(errp)s);
 ''',
-                     c_name=c_var(argname), name=argname, argtype=argtype,
+                     v=vvar, c_name=c_var(argname), name=argname, argtype=argtype,
                      visitor=type_visitor(argtype), errp=errparg)
         ret += gen_err_check(errarg)
-        if optional:
+        if optional or has_default:
             pop_indent()
             ret += mcgen('''
 }
@@ -167,6 +181,7 @@ if (has_%(c_name)s) {
 
     if dealloc:
         ret += mcgen('''
+qmp_input_visitor_cleanup(mi);
 qapi_dealloc_visitor_cleanup(md);
 ''')
     pop_indent()
-- 
1.9.2

[Qemu-devel] [RFC PATCH v2 7/7] qmp: Convert block-commit speed to arg property dict

[Fam Zheng]

Moving the default value logic from qmp handler to generated code.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 6 +-----
 qapi-schema.json | 2 +-
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7810e9f..de06218 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1869,7 +1869,7 @@ void qmp_block_stream(const char *device, bool has_base,
 
 void qmp_block_commit(const char *device,
                       bool has_base, const char *base, const char *top,
-                      bool has_speed, int64_t speed,
+                      int64_t speed,
                       Error **errp)
 {
     BlockDriverState *bs;
@@ -1880,10 +1880,6 @@ void qmp_block_commit(const char *device,
      */
     BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
 
-    if (!has_speed) {
-        speed = 0;
-    }
-
     /* drain all i/o before commits */
     bdrv_drain_all();
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 36cb964..06e373c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2129,7 +2129,7 @@
 ##
 { 'command': 'block-commit',
   'data': { 'device': 'str', '*base': 'str', 'top': 'str',
-            '*speed': 'int' } }
+            '@speed': { 'type': 'int', 'optional': true, 'default': 0 } } }
 
 ##
 # @drive-backup
-- 
1.9.2

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Fam Zheng]

My script failed to generate a good Cc list, manual fix. Sorry for the noise.

Fam

On Tue, 05/20 17:07, Fam Zheng wrote:
> Please first take a look at patch 7 to see what is supported by this series.
> 
> Patch 1 ~ 3 allows some useful basic types in schema.
> 
> Patch 4 ~ 6 implements the new syntax.
> 
> Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
> reducing the cleanness of the syntax. We should get rid of both of them in long
> term. Here, this series compromises on this and introduces '@arg' because:
> 
>   - We have to distinguish the argument property dictionary from nested struct:
> 
>     I.e.:
> 
>         'data': {
>             'arg1': { 'member1': 'int', 'member2': 'str' }
>             '@arg2': { 'type': 'int', 'default': 100 }
>          }
> 
>     Until we completely drop and forbid the 'arg1' nested struct use case.
> 
>   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
>     distractive patches to this series.
> 
>     Basically we need to:
> 
>     * converting existing nested structs to named structs in qapi-schema.json.
> 
>     * converting C code that references these structs because member structures
>       become member structure pointers.
> 
>     * coverting qapi-schema test cases on nested structure.
> 
>     * updating docs/qapi-code-gen.txt.
> 
>   - So let's focus on the default value requirement for now, because it's not
>     making the above change harder.
> 
> Test cases, documentation, more data types of default value, and conversion of
> more qmp commands will come later, if we could agree on the schema design and
> framework.
> 
> Thanks,
> Fam
> 
> 
> Fam Zheng (7):
>   qapi: Allow decimal values
>   qapi: Allow true, false and null in schema json
>   tests: Add decimal test cases for qapi-schema
>   qapi: Add c_val(t, val) for int
>   qapi: Add @arg property dictionary syntax
>   qapi: Initialize argument value in generated code if has 'default'
>   qmp: Convert block-commit speed to arg property dict
> 
>  blockdev.c                                   |  6 +--
>  qapi-schema.json                             |  2 +-
>  scripts/qapi-commands.py                     | 55 +++++++++++++++----------
>  scripts/qapi-types.py                        |  2 +-
>  scripts/qapi-visit.py                        |  4 +-
>  scripts/qapi.py                              | 60 +++++++++++++++++++++++++---
>  tests/Makefile                               |  3 +-
>  tests/qapi-schema/integers-leading-zero.err  |  1 +
>  tests/qapi-schema/integers-leading-zero.exit |  1 +
>  tests/qapi-schema/integers-leading-zero.json |  1 +
>  tests/qapi-schema/integers-leading-zero.out  |  0
>  tests/qapi-schema/integers-overflow.err      |  1 +
>  tests/qapi-schema/integers-overflow.exit     |  1 +
>  tests/qapi-schema/integers-overflow.json     |  1 +
>  tests/qapi-schema/integers-overflow.out      |  0
>  tests/qapi-schema/integers.err               |  0
>  tests/qapi-schema/integers.exit              |  1 +
>  tests/qapi-schema/integers.json              | 10 +++++
>  tests/qapi-schema/integers.out               |  3 ++
>  19 files changed, 117 insertions(+), 35 deletions(-)
>  create mode 100644 tests/qapi-schema/integers-leading-zero.err
>  create mode 100644 tests/qapi-schema/integers-leading-zero.exit
>  create mode 100644 tests/qapi-schema/integers-leading-zero.json
>  create mode 100644 tests/qapi-schema/integers-leading-zero.out
>  create mode 100644 tests/qapi-schema/integers-overflow.err
>  create mode 100644 tests/qapi-schema/integers-overflow.exit
>  create mode 100644 tests/qapi-schema/integers-overflow.json
>  create mode 100644 tests/qapi-schema/integers-overflow.out
>  create mode 100644 tests/qapi-schema/integers.err
>  create mode 100644 tests/qapi-schema/integers.exit
>  create mode 100644 tests/qapi-schema/integers.json
>  create mode 100644 tests/qapi-schema/integers.out
> 
> -- 
> 1.9.2
> 
> 

Re: [Qemu-devel] [RFC PATCH v2 3/7] tests: Add decimal test cases for qapi-schema

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

On 05/20/2014 03:07 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---

> +++ b/tests/qapi-schema/integers-leading-zero.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/integers-leading-zero.json:1:12: Invalid number "00": Leading zero for non-zero integer

You are correct that this is not valid JSON.  But "00" is not a non-zero
integer.  This error message needs help (in patch 1/7).

-- 
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: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 1/7] qapi: Allow decimal values

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On 05/20/2014 03:07 AM, Fam Zheng wrote:
> This allows giving decimal constants in the schema as expr.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  scripts/qapi.py | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 0265b40..4c945ad 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -157,6 +157,21 @@ class QAPISchema:
>                          return
>                      else:
>                          string += ch
> +            elif self.tok in "-0123456789":
> +                val = self.tok
> +                while self.src[self.cursor] in "0123456789":
> +                    val += self.src[self.cursor]
> +                    self.cursor += 1
> +                try:
> +                    if val.startswith("0") and len(val) > 1:

This fails to diagnose '-0123' as invalid.  Which means your test case
in 3/7 is incomplete.

> +                        raise Exception("Leading zero for non-zero integer")

For the string "00", this message makes no sense.  You are properly
diagnosing that double 0 is not a valid JSON int, but it is also not a
"non-zero integer".  Maybe just "Leading zero not permitted for integer".

> +                    self.val = int(val)
> +                    if self.val > 0x7fffffffffffffffL or self.val < -0x7fffffffffffffffL - 1:
> +                        raise Exception("Value too big")

Forces the user to use int64_t rather than allowing for uint64_t
defaults (if we have an unsigned value that defaults to anything larger
than INT64_MAX, it would have to be written in 2s-complement negative
form) - but that's okay with me (I really doubt we'd have any default in
that situation except possibly UINT64_MAX, but writing -1 for UINT64_MAX
doesn't feel too bad).

-- 
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: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

On 05/20/2014 03:07 AM, Fam Zheng wrote:
> Please first take a look at patch 7 to see what is supported by this series.
> 
> Patch 1 ~ 3 allows some useful basic types in schema.
> 
> Patch 4 ~ 6 implements the new syntax.
> 
> Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
> reducing the cleanness of the syntax. We should get rid of both of them in long
> term. Here, this series compromises on this and introduces '@arg' because:
> 
>   - We have to distinguish the argument property dictionary from nested struct:
> 
>     I.e.:
> 
>         'data': {
>             'arg1': { 'member1': 'int', 'member2': 'str' }
>             '@arg2': { 'type': 'int', 'default': 100 }
>          }
> 
>     Until we completely drop and forbid the 'arg1' nested struct use case.
> 
>   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
>     distractive patches to this series.

Question - since we WANT to get rid of nested struct, why not reverse
the sense?  Mark all existing nested structs (weren't there just three
that we found?) with the '@' sigil, and let the new syntax be
sigil-free.  Then when we clean up the nesting, we are also getting rid
of the bad syntax, plus the sigil gives us something to search for in
knowing how much to clean up.  But if you stick the sigil on the new
code, instead of the obsolete code, then as more and more places in the
schema use defaults, it gets harder and harder to remove the use of the
sigil even if the nested structs are eventually removed.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 2/7] qapi: Allow true, false and null in schema json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On 05/20/2014 03:07 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  scripts/qapi.py | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)

I applied this patch and tried to break things.

Pre-patch,
{ 'random': tru } => foo.json:1:13: Stray "t"
{ 'random': true } => foo.json:1:13: Stray "t"
{ 'random': truest } => foo.json:1:13: Stray "t"

Post-patch,
{ 'random': tru } => foo.json:1:14: Stray "r"
{ 'random': true } => accepted
{ 'random': truest } => foo.json:1:17: Stray "s"

Not ideal that you are reporting a different stray character based on
how much (1 or 4 bytes) that you tentatively already consumed.  Better
might be to report a stray word (all characters until the next
whitespace, starting at the first bare character rather than mid-word).

Are these cases tested in 3/7?

-- 
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: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Fam Zheng]

On Tue, 05/20 13:13, Eric Blake wrote:
> On 05/20/2014 03:07 AM, Fam Zheng wrote:
> > Please first take a look at patch 7 to see what is supported by this series.
> > 
> > Patch 1 ~ 3 allows some useful basic types in schema.
> > 
> > Patch 4 ~ 6 implements the new syntax.
> > 
> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
> > reducing the cleanness of the syntax. We should get rid of both of them in long
> > term. Here, this series compromises on this and introduces '@arg' because:
> > 
> >   - We have to distinguish the argument property dictionary from nested struct:
> > 
> >     I.e.:
> > 
> >         'data': {
> >             'arg1': { 'member1': 'int', 'member2': 'str' }
> >             '@arg2': { 'type': 'int', 'default': 100 }
> >          }
> > 
> >     Until we completely drop and forbid the 'arg1' nested struct use case.
> > 
> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
> >     distractive patches to this series.
> 
> Question - since we WANT to get rid of nested struct, why not reverse
> the sense?  Mark all existing nested structs (weren't there just three
> that we found?) with the '@' sigil, and let the new syntax be
> sigil-free.  Then when we clean up the nesting, we are also getting rid
> of the bad syntax, plus the sigil gives us something to search for in
> knowing how much to clean up.  But if you stick the sigil on the new
> code, instead of the obsolete code, then as more and more places in the
> schema use defaults, it gets harder and harder to remove the use of the
> sigil even if the nested structs are eventually removed.
> 

It makes not much difference I can see. The hard part is actaully dropping
nested, converting from sigil <-> non-sigil is easy. Of course, nothing is
seriously hard, there are only three nested structs plus some more qapi-schema
test code.

A question before that is, if we are determined to drop '@' sigil (whether from
nested or property dict), are we as determined to drop '*' sigil as well?

Fam

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Tue, 05/20 13:13, Eric Blake wrote:
>> On 05/20/2014 03:07 AM, Fam Zheng wrote:
>> > Please first take a look at patch 7 to see what is supported by this series.
>> > 
>> > Patch 1 ~ 3 allows some useful basic types in schema.
>> > 
>> > Patch 4 ~ 6 implements the new syntax.
>> > 
>> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
>> > reducing the cleanness of the syntax. We should get rid of both of them in long
>> > term. Here, this series compromises on this and introduces '@arg' because:
>> > 
>> >   - We have to distinguish the argument property dictionary from nested struct:
>> > 
>> >     I.e.:
>> > 
>> >         'data': {
>> >             'arg1': { 'member1': 'int', 'member2': 'str' }
>> >             '@arg2': { 'type': 'int', 'default': 100 }
>> >          }
>> > 
>> >     Until we completely drop and forbid the 'arg1' nested struct use case.
>> > 
>> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
>> >     distractive patches to this series.
>> 
>> Question - since we WANT to get rid of nested struct, why not reverse
>> the sense?  Mark all existing nested structs (weren't there just three
>> that we found?) with the '@' sigil, and let the new syntax be
>> sigil-free.  Then when we clean up the nesting, we are also getting rid
>> of the bad syntax, plus the sigil gives us something to search for in
>> knowing how much to clean up.  But if you stick the sigil on the new
>> code, instead of the obsolete code, then as more and more places in the
>> schema use defaults, it gets harder and harder to remove the use of the
>> sigil even if the nested structs are eventually removed.
>> 
>
> It makes not much difference I can see. The hard part is actaully dropping
> nested, converting from sigil <-> non-sigil is easy. Of course, nothing is
> seriously hard, there are only three nested structs plus some more qapi-schema
> test code.

Adding three ugly sigils and making everybody include one when they add
a nested struct feels much better to me than ugly sigils all over the
place.

> A question before that is, if we are determined to drop '@' sigil (whether from
> nested or property dict), are we as determined to drop '*' sigil as well?

We decided to wait and see how many optionals pick up defaults.  '*' is
only for optionals without defaults.

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Fam Zheng]

On Wed, 05/21 07:54, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Tue, 05/20 13:13, Eric Blake wrote:
> >> On 05/20/2014 03:07 AM, Fam Zheng wrote:
> >> > Please first take a look at patch 7 to see what is supported by this series.
> >> > 
> >> > Patch 1 ~ 3 allows some useful basic types in schema.
> >> > 
> >> > Patch 4 ~ 6 implements the new syntax.
> >> > 
> >> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
> >> > reducing the cleanness of the syntax. We should get rid of both of them in long
> >> > term. Here, this series compromises on this and introduces '@arg' because:
> >> > 
> >> >   - We have to distinguish the argument property dictionary from nested struct:
> >> > 
> >> >     I.e.:
> >> > 
> >> >         'data': {
> >> >             'arg1': { 'member1': 'int', 'member2': 'str' }
> >> >             '@arg2': { 'type': 'int', 'default': 100 }
> >> >          }
> >> > 
> >> >     Until we completely drop and forbid the 'arg1' nested struct use case.
> >> > 
> >> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
> >> >     distractive patches to this series.
> >> 
> >> Question - since we WANT to get rid of nested struct, why not reverse
> >> the sense?  Mark all existing nested structs (weren't there just three
> >> that we found?) with the '@' sigil, and let the new syntax be
> >> sigil-free.  Then when we clean up the nesting, we are also getting rid
> >> of the bad syntax, plus the sigil gives us something to search for in
> >> knowing how much to clean up.  But if you stick the sigil on the new
> >> code, instead of the obsolete code, then as more and more places in the
> >> schema use defaults, it gets harder and harder to remove the use of the
> >> sigil even if the nested structs are eventually removed.
> >> 
> >
> > It makes not much difference I can see. The hard part is actaully dropping
> > nested, converting from sigil <-> non-sigil is easy. Of course, nothing is
> > seriously hard, there are only three nested structs plus some more qapi-schema
> > test code.
> 
> Adding three ugly sigils and making everybody include one when they add
> a nested struct feels much better to me than ugly sigils all over the
> place.

Well, I could use some background here. Why did we introduce nested structure
in the first place?

Fam

> 
> > A question before that is, if we are determined to drop '@' sigil (whether from
> > nested or property dict), are we as determined to drop '*' sigil as well?
> 
> We decided to wait and see how many optionals pick up defaults.  '*' is
> only for optionals without defaults.

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Wed, 05/21 07:54, Markus Armbruster wrote:
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > On Tue, 05/20 13:13, Eric Blake wrote:
>> >> On 05/20/2014 03:07 AM, Fam Zheng wrote:
>> >> > Please first take a look at patch 7 to see what is supported by this series.
>> >> > 
>> >> > Patch 1 ~ 3 allows some useful basic types in schema.
>> >> > 
>> >> > Patch 4 ~ 6 implements the new syntax.
>> >> > 
>> >> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
>> >> > reducing the cleanness of the syntax. We should get rid of both of them in long
>> >> > term. Here, this series compromises on this and introduces '@arg' because:
>> >> > 
>> >> >   - We have to distinguish the argument property dictionary from nested struct:
>> >> > 
>> >> >     I.e.:
>> >> > 
>> >> >         'data': {
>> >> >             'arg1': { 'member1': 'int', 'member2': 'str' }
>> >> >             '@arg2': { 'type': 'int', 'default': 100 }
>> >> >          }
>> >> > 
>> >> >     Until we completely drop and forbid the 'arg1' nested struct use case.
>> >> > 
>> >> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
>> >> >     distractive patches to this series.
>> >> 
>> >> Question - since we WANT to get rid of nested struct, why not reverse
>> >> the sense?  Mark all existing nested structs (weren't there just three
>> >> that we found?) with the '@' sigil, and let the new syntax be
>> >> sigil-free.  Then when we clean up the nesting, we are also getting rid
>> >> of the bad syntax, plus the sigil gives us something to search for in
>> >> knowing how much to clean up.  But if you stick the sigil on the new
>> >> code, instead of the obsolete code, then as more and more places in the
>> >> schema use defaults, it gets harder and harder to remove the use of the
>> >> sigil even if the nested structs are eventually removed.
>> >> 
>> >
>> > It makes not much difference I can see. The hard part is actaully dropping
>> > nested, converting from sigil <-> non-sigil is easy. Of course, nothing is
>> > seriously hard, there are only three nested structs plus some more
>> > qapi-schema
>> > test code.
>> 
>> Adding three ugly sigils and making everybody include one when they add
>> a nested struct feels much better to me than ugly sigils all over the
>> place.
>
> Well, I could use some background here. Why did we introduce nested structure
> in the first place?

Because we could?

Felt like a good idea at the time?

I quick glance at commit 0f923be and fb3182c suggests they have been
supported since the beginning.  There is no design rationale.

[...]

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Kevin Wolf]

Am 21.05.2014 um 09:46 hat Markus Armbruster geschrieben:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Wed, 05/21 07:54, Markus Armbruster wrote:
> >> Fam Zheng <famz@redhat.com> writes:
> >> 
> >> > On Tue, 05/20 13:13, Eric Blake wrote:
> >> >> On 05/20/2014 03:07 AM, Fam Zheng wrote:
> >> >> > Please first take a look at patch 7 to see what is supported by this series.
> >> >> > 
> >> >> > Patch 1 ~ 3 allows some useful basic types in schema.
> >> >> > 
> >> >> > Patch 4 ~ 6 implements the new syntax.
> >> >> > 
> >> >> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
> >> >> > reducing the cleanness of the syntax. We should get rid of both of them in long
> >> >> > term. Here, this series compromises on this and introduces '@arg' because:
> >> >> > 
> >> >> >   - We have to distinguish the argument property dictionary from nested struct:
> >> >> > 
> >> >> >     I.e.:
> >> >> > 
> >> >> >         'data': {
> >> >> >             'arg1': { 'member1': 'int', 'member2': 'str' }
> >> >> >             '@arg2': { 'type': 'int', 'default': 100 }
> >> >> >          }
> >> >> > 
> >> >> >     Until we completely drop and forbid the 'arg1' nested struct use case.
> >> >> > 
> >> >> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
> >> >> >     distractive patches to this series.
> >> >> 
> >> >> Question - since we WANT to get rid of nested struct, why not reverse
> >> >> the sense?  Mark all existing nested structs (weren't there just three
> >> >> that we found?) with the '@' sigil, and let the new syntax be
> >> >> sigil-free.  Then when we clean up the nesting, we are also getting rid
> >> >> of the bad syntax, plus the sigil gives us something to search for in
> >> >> knowing how much to clean up.  But if you stick the sigil on the new
> >> >> code, instead of the obsolete code, then as more and more places in the
> >> >> schema use defaults, it gets harder and harder to remove the use of the
> >> >> sigil even if the nested structs are eventually removed.
> >> >> 
> >> >
> >> > It makes not much difference I can see. The hard part is actaully dropping
> >> > nested, converting from sigil <-> non-sigil is easy. Of course, nothing is
> >> > seriously hard, there are only three nested structs plus some more
> >> > qapi-schema
> >> > test code.
> >> 
> >> Adding three ugly sigils and making everybody include one when they add
> >> a nested struct feels much better to me than ugly sigils all over the
> >> place.
> >
> > Well, I could use some background here. Why did we introduce nested structure
> > in the first place?
> 
> Because we could?
> 
> Felt like a good idea at the time?
> 
> I quick glance at commit 0f923be and fb3182c suggests they have been
> supported since the beginning.  There is no design rationale.

Let me extend Fam's question: Why don't we simply remove them right
now? If it's really only three instances, converting them to full
types should be a matter of five minutes.

Kevin

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Fam Zheng]

On Wed, 05/21 10:23, Kevin Wolf wrote:
> Am 21.05.2014 um 09:46 hat Markus Armbruster geschrieben:
> > Fam Zheng <famz@redhat.com> writes:
> > 
> > > On Wed, 05/21 07:54, Markus Armbruster wrote:
> > >> Fam Zheng <famz@redhat.com> writes:
> > >> 
> > >> > On Tue, 05/20 13:13, Eric Blake wrote:
> > >> >> On 05/20/2014 03:07 AM, Fam Zheng wrote:
> > >> >> > Please first take a look at patch 7 to see what is supported by this series.
> > >> >> > 
> > >> >> > Patch 1 ~ 3 allows some useful basic types in schema.
> > >> >> > 
> > >> >> > Patch 4 ~ 6 implements the new syntax.
> > >> >> > 
> > >> >> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
> > >> >> > reducing the cleanness of the syntax. We should get rid of both of them in long
> > >> >> > term. Here, this series compromises on this and introduces '@arg' because:
> > >> >> > 
> > >> >> >   - We have to distinguish the argument property dictionary from nested struct:
> > >> >> > 
> > >> >> >     I.e.:
> > >> >> > 
> > >> >> >         'data': {
> > >> >> >             'arg1': { 'member1': 'int', 'member2': 'str' }
> > >> >> >             '@arg2': { 'type': 'int', 'default': 100 }
> > >> >> >          }
> > >> >> > 
> > >> >> >     Until we completely drop and forbid the 'arg1' nested struct use case.
> > >> >> > 
> > >> >> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
> > >> >> >     distractive patches to this series.
> > >> >> 
> > >> >> Question - since we WANT to get rid of nested struct, why not reverse
> > >> >> the sense?  Mark all existing nested structs (weren't there just three
> > >> >> that we found?) with the '@' sigil, and let the new syntax be
> > >> >> sigil-free.  Then when we clean up the nesting, we are also getting rid
> > >> >> of the bad syntax, plus the sigil gives us something to search for in
> > >> >> knowing how much to clean up.  But if you stick the sigil on the new
> > >> >> code, instead of the obsolete code, then as more and more places in the
> > >> >> schema use defaults, it gets harder and harder to remove the use of the
> > >> >> sigil even if the nested structs are eventually removed.
> > >> >> 
> > >> >
> > >> > It makes not much difference I can see. The hard part is actaully dropping
> > >> > nested, converting from sigil <-> non-sigil is easy. Of course, nothing is
> > >> > seriously hard, there are only three nested structs plus some more
> > >> > qapi-schema
> > >> > test code.
> > >> 
> > >> Adding three ugly sigils and making everybody include one when they add
> > >> a nested struct feels much better to me than ugly sigils all over the
> > >> place.
> > >
> > > Well, I could use some background here. Why did we introduce nested structure
> > > in the first place?
> > 
> > Because we could?
> > 
> > Felt like a good idea at the time?
> > 
> > I quick glance at commit 0f923be and fb3182c suggests they have been
> > supported since the beginning.  There is no design rationale.
> 
> Let me extend Fam's question: Why don't we simply remove them right
> now? If it's really only three instances, converting them to full
> types should be a matter of five minutes.
> 

Actually, my question is: do we want it independently, or do we want to include
the removal of nested as the first part of this series?

I would prefer the former because I feel uncomfortable with making more changes
in this series, since there are already many things to do: adding qapi types,
adding argument property dict, adding all test cases for all of them, updating
documentation, and apply the new syntax in qapi-schema.json. A non-RFC revision
could be long and hard to review.

Fam

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Kevin Wolf]

Am 21.05.2014 um 10:42 hat Fam Zheng geschrieben:
> On Wed, 05/21 10:23, Kevin Wolf wrote:
> > Am 21.05.2014 um 09:46 hat Markus Armbruster geschrieben:
> > > Fam Zheng <famz@redhat.com> writes:
> > > 
> > > > On Wed, 05/21 07:54, Markus Armbruster wrote:
> > > >> Fam Zheng <famz@redhat.com> writes:
> > > >> 
> > > >> > On Tue, 05/20 13:13, Eric Blake wrote:
> > > >> >> On 05/20/2014 03:07 AM, Fam Zheng wrote:
> > > >> >> > Please first take a look at patch 7 to see what is supported by this series.
> > > >> >> > 
> > > >> >> > Patch 1 ~ 3 allows some useful basic types in schema.
> > > >> >> > 
> > > >> >> > Patch 4 ~ 6 implements the new syntax.
> > > >> >> > 
> > > >> >> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
> > > >> >> > reducing the cleanness of the syntax. We should get rid of both of them in long
> > > >> >> > term. Here, this series compromises on this and introduces '@arg' because:
> > > >> >> > 
> > > >> >> >   - We have to distinguish the argument property dictionary from nested struct:
> > > >> >> > 
> > > >> >> >     I.e.:
> > > >> >> > 
> > > >> >> >         'data': {
> > > >> >> >             'arg1': { 'member1': 'int', 'member2': 'str' }
> > > >> >> >             '@arg2': { 'type': 'int', 'default': 100 }
> > > >> >> >          }
> > > >> >> > 
> > > >> >> >     Until we completely drop and forbid the 'arg1' nested struct use case.
> > > >> >> > 
> > > >> >> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
> > > >> >> >     distractive patches to this series.
> > > >> >> 
> > > >> >> Question - since we WANT to get rid of nested struct, why not reverse
> > > >> >> the sense?  Mark all existing nested structs (weren't there just three
> > > >> >> that we found?) with the '@' sigil, and let the new syntax be
> > > >> >> sigil-free.  Then when we clean up the nesting, we are also getting rid
> > > >> >> of the bad syntax, plus the sigil gives us something to search for in
> > > >> >> knowing how much to clean up.  But if you stick the sigil on the new
> > > >> >> code, instead of the obsolete code, then as more and more places in the
> > > >> >> schema use defaults, it gets harder and harder to remove the use of the
> > > >> >> sigil even if the nested structs are eventually removed.
> > > >> >> 
> > > >> >
> > > >> > It makes not much difference I can see. The hard part is actaully dropping
> > > >> > nested, converting from sigil <-> non-sigil is easy. Of course, nothing is
> > > >> > seriously hard, there are only three nested structs plus some more
> > > >> > qapi-schema
> > > >> > test code.
> > > >> 
> > > >> Adding three ugly sigils and making everybody include one when they add
> > > >> a nested struct feels much better to me than ugly sigils all over the
> > > >> place.
> > > >
> > > > Well, I could use some background here. Why did we introduce nested structure
> > > > in the first place?
> > > 
> > > Because we could?
> > > 
> > > Felt like a good idea at the time?
> > > 
> > > I quick glance at commit 0f923be and fb3182c suggests they have been
> > > supported since the beginning.  There is no design rationale.
> > 
> > Let me extend Fam's question: Why don't we simply remove them right
> > now? If it's really only three instances, converting them to full
> > types should be a matter of five minutes.
> > 
> 
> Actually, my question is: do we want it independently, or do we want to include
> the removal of nested as the first part of this series?
> 
> I would prefer the former because I feel uncomfortable with making more changes
> in this series, since there are already many things to do: adding qapi types,
> adding argument property dict, adding all test cases for all of them, updating
> documentation, and apply the new syntax in qapi-schema.json. A non-RFC revision
> could be long and hard to review.

The removal of nested structs must come first. Whether it's done as part
of this series or as a separate series that this one will depend on
doesn't really matter that much. I think I would do it as one big series
(and 8 patches with a diffstat like this is not that big, after all),
but if you prefer to split it up, I think that should be okay, too.

Kevin

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 21.05.2014 um 09:46 hat Markus Armbruster geschrieben:
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > On Wed, 05/21 07:54, Markus Armbruster wrote:
>> >> Fam Zheng <famz@redhat.com> writes:
>> >> 
>> >> > On Tue, 05/20 13:13, Eric Blake wrote:
>> >> >> On 05/20/2014 03:07 AM, Fam Zheng wrote:
>> >> >> > Please first take a look at patch 7 to see what is supported by this series.
>> >> >> > 
>> >> >> > Patch 1 ~ 3 allows some useful basic types in schema.
>> >> >> > 
>> >> >> > Patch 4 ~ 6 implements the new syntax.
>> >> >> > 
>> >> >> > Note: The introduced '@arg' sigil, just like the preexisting '*arg', is
>> >> >> > reducing the cleanness of the syntax. We should get rid of both of them in long
>> >> >> > term. Here, this series compromises on this and introduces '@arg' because:
>> >> >> > 
>> >> >> >   - We have to distinguish the argument property dictionary from nested struct:
>> >> >> > 
>> >> >> >     I.e.:
>> >> >> > 
>> >> >> >         'data': {
>> >> >> >             'arg1': { 'member1': 'int', 'member2': 'str' }
>> >> >> >             '@arg2': { 'type': 'int', 'default': 100 }
>> >> >> >          }
>> >> >> > 
>> >> >> >     Until we completely drop and forbid the 'arg1' nested struct use case.
>> >> >> > 
>> >> >> >   - Forbidding 'arg1' it's doable, but doing it now means we pull in many
>> >> >> >     distractive patches to this series.
>> >> >> 
>> >> >> Question - since we WANT to get rid of nested struct, why not reverse
>> >> >> the sense?  Mark all existing nested structs (weren't there just three
>> >> >> that we found?) with the '@' sigil, and let the new syntax be
>> >> >> sigil-free.  Then when we clean up the nesting, we are also getting rid
>> >> >> of the bad syntax, plus the sigil gives us something to search for in
>> >> >> knowing how much to clean up.  But if you stick the sigil on the new
>> >> >> code, instead of the obsolete code, then as more and more places in the
>> >> >> schema use defaults, it gets harder and harder to remove the use of the
>> >> >> sigil even if the nested structs are eventually removed.
>> >> >> 
>> >> >
>> >> > It makes not much difference I can see. The hard part is
>> >> > actaully dropping
>> >> > nested, converting from sigil <-> non-sigil is easy. Of course,
>> >> > nothing is
>> >> > seriously hard, there are only three nested structs plus some more
>> >> > qapi-schema
>> >> > test code.
>> >> 
>> >> Adding three ugly sigils and making everybody include one when they add
>> >> a nested struct feels much better to me than ugly sigils all over the
>> >> place.
>> >
>> > Well, I could use some background here. Why did we introduce
>> > nested structure
>> > in the first place?
>> 
>> Because we could?
>> 
>> Felt like a good idea at the time?
>> 
>> I quick glance at commit 0f923be and fb3182c suggests they have been
>> supported since the beginning.  There is no design rationale.
>
> Let me extend Fam's question: Why don't we simply remove them right
> now? If it's really only three instances, converting them to full
> types should be a matter of five minutes.

Trades some convenience of expresssion we haven't found useful all that
often for simplicity.

I take the simplicity.

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]

On 05/21/2014 02:42 AM, Fam Zheng wrote:

>>>>> Adding three ugly sigils and making everybody include one when they add
>>>>> a nested struct feels much better to me than ugly sigils all over the
>>>>> place.
>>>>
>>>> Well, I could use some background here. Why did we introduce nested structure
>>>> in the first place?
>>>
>>> Because we could?
>>>
>>> Felt like a good idea at the time?
>>>
>>> I quick glance at commit 0f923be and fb3182c suggests they have been
>>> supported since the beginning.  There is no design rationale.
>>
>> Let me extend Fam's question: Why don't we simply remove them right
>> now? If it's really only three instances, converting them to full
>> types should be a matter of five minutes.
>>
> 
> Actually, my question is: do we want it independently, or do we want to include
> the removal of nested as the first part of this series?

Doing it as an independent series first might be the way forward -
independent so that it doesn't stall on reviews of the new syntax for
default values, and up front because it seems like a simple enough
conversion that then makes the entire generator simpler that it will be
easy to approve and get in tree.

> 
> I would prefer the former because I feel uncomfortable with making more changes
> in this series, since there are already many things to do: adding qapi types,
> adding argument property dict, adding all test cases for all of them, updating
> documentation, and apply the new syntax in qapi-schema.json. A non-RFC revision
> could be long and hard to review.

At the end of the day, we want both things; and it makes more sense to
remove the conflicting syntax up front than it does to add an alternate
syntax only to later remove it.  Doing it as two shorter series one
after the other rather than cramming it into one long series is
psychologically easier to review.

-- 
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: 604 bytes --]