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

login
register
mail settings
Submitter Amos Kong
Date April 24, 2013, 4:08 p.m.
Message ID <1366819685-24106-1-git-send-email-akong@redhat.com>
Download mbox | patch
Permalink /patch/239268/
State New
Headers show

Comments

Amos Kong - April 24, 2013, 4:08 p.m.
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(-)
Paolo Bonzini - April 24, 2013, 4:11 p.m.
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
Michael Roth - April 24, 2013, 4:46 p.m.
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
>
Amos Kong - April 24, 2013, 4:48 p.m.
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
Amos Kong - April 26, 2013, 10:12 a.m.
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.

Patch

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 = []