[v2] qapi: add struct strList and visit_type_strList()

Currently we can only use ['String'] to add string to a list,
it contains some additional JSON structure.
    "multicast": [
        {
            "str": "01:80:c2:00:00:21"
        },
        {
            "str": "00:00:00:00:00:00"
        }
    ]

This patch adds struct strList and visit function, we can use ['str'] in
schema file.

    "multicast": [
        "01:00:5e:00:00:01",
        "33:33:ff:12:34:57"
    ]

V2: remove ugly #ifndef, add forward declaration in qapi-types.h,
    and define the struct in include/qapi/visitor.h (Paolo)

Signed-off-by: Amos Kong <akong@redhat.com>
---
 include/qapi/visitor.h |    7 +++++++
 qapi/qapi-visit-core.c |   23 +++++++++++++++++++++++
 scripts/qapi-types.py  |    1 +
 scripts/qapi.py        |    4 ++--
 4 files changed, 33 insertions(+), 2 deletions(-)

Il 24/04/2013 18:08, Amos Kong ha scritto:
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 9e19920..e449a14 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -276,6 +276,7 @@ fdecl.write(mcgen('''
>  #include <stdbool.h>
>  #include <stdint.h>
>  
> +struct strList;
>  ''',
>                    guard=guardname(h_file)))
>  
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index afc5f32..14f9f4d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -166,11 +166,11 @@ def c_fun(name, protect=True):
>      return c_var(name, protect).replace('.', '_')
>  
>  def c_list_type(name):
> -    return '%sList' % name
> +    return 'struct %sList' % name
>  
>  def type_name(name):
>      if type(name) == list:
> -        return c_list_type(name[0])
> +        return '%sList' % name[0]
>      return name

This second change is needed to generate the correct function names,
right?  If so,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

On Thu, Apr 25, 2013 at 12:08:05AM +0800, Amos Kong wrote:
> Currently we can only use ['String'] to add string to a list,
> it contains some additional JSON structure.
>     "multicast": [
>         {
>             "str": "01:80:c2:00:00:21"
>         },
>         {
>             "str": "00:00:00:00:00:00"
>         }
>     ]
> 
> This patch adds struct strList and visit function, we can use ['str'] in
> schema file.
> 
>     "multicast": [
>         "01:00:5e:00:00:01",
>         "33:33:ff:12:34:57"
>     ]
> 
> V2: remove ugly #ifndef, add forward declaration in qapi-types.h,
>     and define the struct in include/qapi/visitor.h (Paolo)
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

Sorry for the delay in getting back to you, I started to respond last
week and ended up hacking something up that take a different approach.

Namely: we don't hardcode visit_type_strList(), but instead teach the
qapi code generators about native types we currently allow in schemas
('str', 'int', 'number', 'bool') and have them handle code generation
for these like they would for any user-defined type.

This approach works too, but I think this solution is more complete.
I'm still working out some bits with the unit tests but I've pushed my
WIP here for reference:

https://github.com/mdroth/qemu/commits/qapi-native-lists

Please give that a shot.

> ---
>  include/qapi/visitor.h |    7 +++++++
>  qapi/qapi-visit-core.c |   23 +++++++++++++++++++++++
>  scripts/qapi-types.py  |    1 +
>  scripts/qapi.py        |    4 ++--
>  4 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 1fef18c..743ff92 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -22,6 +22,11 @@ typedef struct GenericList
>      struct GenericList *next;
>  } GenericList;
> 
> +typedef struct strList {
> +    char *value;
> +    struct strList *next;
> +} strList;
> +
>  typedef struct Visitor Visitor;
> 
>  void visit_start_handle(Visitor *v, void **obj, const char *kind,
> @@ -50,6 +55,8 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp);
>  void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
>  void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
>  void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
> +void visit_type_strList(Visitor *m, strList ** obj, const char *name,
> +                        Error **errp);
>  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
> 
>  #endif
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 401ee6e..dc54cc8 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -257,6 +257,29 @@ void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
>      }
>  }
> 
> +void visit_type_strList(Visitor *m, strList ** obj, const char *name,
> +                        Error **errp)
> +{
> +    GenericList *i, **prev = (GenericList **)obj;
> +    Error *err = NULL;
> +
> +    if (!error_is_set(errp)) {
> +        visit_start_list(m, name, &err);
> +        if (!err) {
> +            for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
> +                strList *native_i = (strList *)i;
> +                visit_type_str(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);
> +    }
> +}
> +
>  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
>  {
>      if (!error_is_set(errp)) {
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 9e19920..e449a14 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -276,6 +276,7 @@ fdecl.write(mcgen('''
>  #include <stdbool.h>
>  #include <stdint.h>
> 
> +struct strList;
>  ''',
>                    guard=guardname(h_file)))
> 
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index afc5f32..14f9f4d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -166,11 +166,11 @@ def c_fun(name, protect=True):
>      return c_var(name, protect).replace('.', '_')
> 
>  def c_list_type(name):
> -    return '%sList' % name
> +    return 'struct %sList' % name
> 
>  def type_name(name):
>      if type(name) == list:
> -        return c_list_type(name[0])
> +        return '%sList' % name[0]
>      return name
> 
>  enum_types = []
> -- 
> 1.7.1
>

On Wed, Apr 24, 2013 at 06:11:55PM +0200, Paolo Bonzini wrote:
> Il 24/04/2013 18:08, Amos Kong ha scritto:
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 9e19920..e449a14 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -276,6 +276,7 @@ fdecl.write(mcgen('''
> >  #include <stdbool.h>
> >  #include <stdint.h>
> >  
> > +struct strList;
> >  ''',
> >                    guard=guardname(h_file)))
> >  
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index afc5f32..14f9f4d 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -166,11 +166,11 @@ def c_fun(name, protect=True):
> >      return c_var(name, protect).replace('.', '_')
> >  
> >  def c_list_type(name):
> > -    return '%sList' % name
> > +    return 'struct %sList' % name
> >  
> >  def type_name(name):
> >      if type(name) == list:
> > -        return c_list_type(name[0])
> > +        return '%sList' % name[0]
> >      return name
> 
> This second change is needed to generate the correct function names,
> right?  If so,
 
Yes, otherwise, the visit function name will be "visit_type_struct nameList".

> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Paolo

Thanks

Hi Michael,

On Wed, Apr 24, 2013 at 11:46:25AM -0500, mdroth wrote:
> On Thu, Apr 25, 2013 at 12:08:05AM +0800, Amos Kong wrote:
> > Currently we can only use ['String'] to add string to a list,
> > it contains some additional JSON structure.
> >     "multicast": [
> >         {
> >             "str": "01:80:c2:00:00:21"
> >         },
> >         {
> >             "str": "00:00:00:00:00:00"
> >         }
> >     ]
> > 
> > This patch adds struct strList and visit function, we can use ['str'] in
> > schema file.
> > 
> >     "multicast": [
> >         "01:00:5e:00:00:01",
> >         "33:33:ff:12:34:57"
> >     ]
> > 
> > V2: remove ugly #ifndef, add forward declaration in qapi-types.h,
> >     and define the struct in include/qapi/visitor.h (Paolo)
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> Sorry for the delay in getting back to you, I started to respond last
> week and ended up hacking something up that take a different approach.
> 
> Namely: we don't hardcode visit_type_strList(), but instead teach the
> qapi code generators about native types we currently allow in schemas
> ('str', 'int', 'number', 'bool') and have them handle code generation
> for these like they would for any user-defined type.

It's better.
 
> This approach works too, but I think this solution is more complete.
> I'm still working out some bits with the unit tests but I've pushed my
> WIP here for reference:
> 
> https://github.com/mdroth/qemu/commits/qapi-native-lists
> 
> Please give that a shot.

Your patchset looks good to me, I only added some trivial comments on
github.