From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:35384)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1TK30D-0006qJ-Dc
	for qemu-devel@nongnu.org; Fri, 05 Oct 2012 04:11:24 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1TK30A-000571-6M
	for qemu-devel@nongnu.org; Fri, 05 Oct 2012 04:11:21 -0400
Received: from mx1.redhat.com ([209.132.183.28]:49861)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1TK309-00056x-U0
	for qemu-devel@nongnu.org; Fri, 05 Oct 2012 04:11:18 -0400
Message-ID: <506E961E.3090403@redhat.com>
Date: Fri, 05 Oct 2012 10:11:10 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1349372021-31212-1-git-send-email-mdroth@linux.vnet.ibm.com>
	<1349372021-31212-6-git-send-email-mdroth@linux.vnet.ibm.com>
In-Reply-To: <1349372021-31212-6-git-send-email-mdroth@linux.vnet.ibm.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3 05/22] qapi: qapi_visit.py,
 support arrays and complex qapi definitions
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, eblake@redhat.com

Il 04/10/2012 19:33, Michael Roth ha scritto:
> Add support for arrays in the code generators.
> 
> Complex field descriptions can now be used to provide additional
> information to the visitor generators, such as the max size of an array,
> or the field within a struct to use to determine how many elements are
> present in the array to avoid serializing uninitialized elements.
> 
> Add handling for these in the code generators as well.
> 
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  scripts/qapi.py          |    8 ++++--
>  scripts/qapi_commands.py |    8 +++---
>  scripts/qapi_types.py    |    2 +-
>  scripts/qapi_visit.py    |   64 +++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 68 insertions(+), 14 deletions(-)
> 
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 122b4cb..a347203 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -110,12 +110,16 @@ def parse_args(typeinfo):
>          argentry = typeinfo[member]
>          optional = False
>          structured = False
> +        annotated = False
>          if member.startswith('*'):
>              argname = member[1:]
>              optional = True
>          if isinstance(argentry, OrderedDict):
> -            structured = True
> -        yield (argname, argentry, optional, structured)
> +            if argentry.has_key('<annotated>'):
> +                annotated = True
> +            else:
> +                structured = True
> +        yield (argname, argentry, optional, structured, annotated)
>  
>  def de_camel_case(name):
>      new_name = ''
> diff --git a/scripts/qapi_commands.py b/scripts/qapi_commands.py
> index 3c4678d..a2e0c23 100644
> --- a/scripts/qapi_commands.py
> +++ b/scripts/qapi_commands.py
> @@ -32,7 +32,7 @@ void %(visitor)s(Visitor *m, %(name)s * obj, const char *name, Error **errp);
>  
>  def generate_command_decl(name, args, ret_type):
>      arglist=""
> -    for argname, argtype, optional, structured in parse_args(args):
> +    for argname, argtype, optional, structured, annotated in parse_args(args):
>          argtype = c_type(argtype)
>          if argtype == "char *":
>              argtype = "const char *"
> @@ -50,7 +50,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, annotated in parse_args(args):
>          if optional:
>              arglist += "has_%s, " % c_var(argname)
>          arglist += "%s, " % (c_var(argname))
> @@ -106,7 +106,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, annotated in parse_args(args):
>          if optional:
>              ret += mcgen('''
>  bool has_%(argname)s = false;
> @@ -145,7 +145,7 @@ v = qmp_input_get_visitor(mi);
>  ''',
>                       obj=obj)
>  
> -    for argname, argtype, optional, structured in parse_args(args):
> +    for argname, argtype, optional, structured, annotated in parse_args(args):
>          if optional:
>              ret += mcgen('''
>  visit_start_optional(v, &has_%(c_name)s, "%(name)s", errp);
> diff --git a/scripts/qapi_types.py b/scripts/qapi_types.py
> index 49ef569..6518768 100644
> --- a/scripts/qapi_types.py
> +++ b/scripts/qapi_types.py
> @@ -45,7 +45,7 @@ struct %(name)s
>  ''',
>            name=structname)
>  
> -    for argname, argentry, optional, structured in parse_args(members):
> +    for argname, argentry, optional, structured, annotated 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 974e458..b3f34a2 100644
> --- a/scripts/qapi_visit.py
> +++ b/scripts/qapi_visit.py
> @@ -16,6 +16,52 @@ import sys
>  import os
>  import getopt
>  import errno
> +import types
> +
> +def generate_visit_carray_body(name, info):
> +    if info['array_size'][0].isdigit():
> +        array_size = info['array_size']
> +    elif info['array_size'][0] == '(' and info['array_size'][-1] == ')':
> +        array_size = info['array_size']
> +    else:
> +        array_size = "(*obj)->%s" % info['array_size']
> +
> +    if info.has_key('array_capacity'):
> +        array_capacity = info['array_capacity']
> +    else:
> +        array_capacity = array_size
> +
> +    if camel_case(de_camel_case(info['type'][0])) == info['type'][0]:

Please add a comment that this is for array of objects vs. array of pointers.

> +        ret = mcgen('''
> +visit_start_carray(m, (void **)obj, "%(name)s", %(array_capacity)s, sizeof(%(type)s), errp);
> +int %(name)s_i;
> +for (%(name)s_i = 0; %(name)s_i < %(array_size)s; %(name)s_i++) {
> +    %(type)s %(name)s_ptr = &(*obj)->%(name)s[%(name)s_i];
> +    visit_next_carray(m, errp);
> +    visit_type_%(type_short)s(m, &%(name)s_ptr, NULL, errp);
> +}
> +visit_end_carray(m, errp);

This may leave unbalanced items on the stack.  See the idiom that is used for
lists.

    if (!error_is_set(errp)) {
        visit_start_list(m, name, &err);
        if (!err) {
            for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
                %(name)sList *native_i = (%(name)sList *)i;
                visit_type_%(name)s(m, &native_i->value, NULL, &err);
            }
            error_propagate(errp, err);
            err = NULL;

            /* Always call end_list if start_list succeeded.  */
            visit_end_list(m, &err);
        }
        error_propagate(errp, err);
    }


> +''',
> +                    name=name, type=c_type(info['type'][0]),
> +                    type_short=info['type'][0],
> +                    array_size=array_size,
> +                    array_capacity=array_capacity)
> +    else:
> +        ret = mcgen('''
> +visit_start_carray(m, (void **)obj, "%(name)s", %(array_capacity)s, sizeof(%(type)s), errp);
> +int %(name)s_i;
> +for (%(name)s_i = 0; %(name)s_i < %(array_size)s; %(name)s_i++) {
> +    %(type)s *%(name)s_ptr = (%(type)s *)&(*obj)->%(name)s[%(name)s_i];

Is the cast needed?

> +    visit_next_carray(m, errp);
> +    visit_type_%(type_short)s(m, %(name)s_ptr, NULL, errp);
> +}
> +visit_end_carray(m, errp);
> +''',
> +                    name=name, type=c_type(info['type'][0]),
> +                    type_short=info['type'][0],
> +                    array_size=array_size,
> +                    array_capacity=array_capacity)
> +    return ret
>  
>  def generate_visit_struct_body(field_prefix, name, members):
>      ret = mcgen('''
> @@ -45,10 +91,10 @@ if (!err) {
>  
>      push_indent()
>      push_indent()
> -    for argname, argentry, optional, structured in parse_args(members):
> +    for argname, argentry, optional, structured, annotated in parse_args(members):
>          if optional:
>              ret += mcgen('''
> -visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
> +visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", errp);
>  if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
>  ''',
>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
> @@ -58,12 +104,16 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
>          if structured:
>              ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
>          else:
> -            ret += mcgen('''
> -visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
> +            if annotated:
> +                if isinstance(argentry['type'], types.ListType):
> +                    ret += generate_visit_carray_body(argname, argentry)

Please add documentation to docs/qapi-code-gen.txt for this and the other patches.

> +            else:
> +                ret += mcgen('''
> +visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", errp);
>  ''',
> -                         c_prefix=c_var(field_prefix), prefix=field_prefix,
> -                         type=type_name(argentry), c_name=c_var(argname),
> -                         name=argname)
> +                             c_prefix=c_var(field_prefix), prefix=field_prefix,
> +                             type=type_name(argentry), c_name=c_var(argname),
> +                             name=argname)
>  
>          if optional:
>              pop_indent()
>