[Qemu-devel] [PATCH v4 0/3] qapi: fix coding style in generated code

[Qemu-devel] [PATCH v4 0/3] qapi: fix coding style in generated code

[Amos Kong]

Not a serious issue, but it's helpful if we can fix it.

V2: split change of scripts/qapi-visit.py to a split patch,
    eat space by using a special char as Markus suggested
V3: update commitlog, update special string, fix of adding
    const replace string by pattern
V4: fix pattern to cleanup special string (Paolo)

Amos Kong (3):
  qapi: fix coding style in parameters list
  qapi: add const prefix to 'char *' insider c_type()
  qapi: Suppress unwanted space between type and identifier

 scripts/qapi-commands.py |  4 +---
 scripts/qapi-visit.py    | 20 ++++++++++----------
 scripts/qapi.py          | 18 ++++++++++++------
 3 files changed, 23 insertions(+), 19 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v4 1/3] qapi: fix coding style in parameters list

[Amos Kong]

The space before pointers is redundant.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 scripts/qapi-visit.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c6579be..25834e3 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -38,7 +38,7 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base =
 
     ret += mcgen('''
 
-static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp)
+static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s **obj, Error **errp)
 {
     Error *err = NULL;
 ''',
@@ -149,7 +149,7 @@ def generate_visit_struct(expr):
 
     ret += mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
 {
 ''',
                 name=name)
@@ -166,7 +166,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 def generate_visit_list(name, members):
     return mcgen('''
 
-void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
+void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp)
 {
     GenericList *i, **prev = (GenericList **)obj;
     Error *err = NULL;
@@ -193,7 +193,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name,
 def generate_visit_enum(name, members):
     return mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp)
 {
     visit_type_enum(m, (int *)obj, %(name)s_lookup, "%(name)s", name, errp);
 }
@@ -203,7 +203,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e
 def generate_visit_anon_union(name, members):
     ret = mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
@@ -279,7 +279,7 @@ def generate_visit_union(expr):
 
     ret += mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
@@ -367,13 +367,13 @@ def generate_declaration(name, members, genlist=True, builtin_type=False):
     if not builtin_type:
         ret += mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp);
 ''',
                     name=name)
 
     if genlist:
         ret += mcgen('''
-void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp);
+void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp);
 ''',
                  name=name)
 
@@ -383,7 +383,7 @@ def generate_enum_declaration(name, members, genlist=True):
     ret = ""
     if genlist:
         ret += mcgen('''
-void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp);
+void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp);
 ''',
                      name=name)
 
@@ -392,7 +392,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name,
 def generate_decl_enum(name, members, genlist=True):
     return mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp);
+void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp);
 ''',
                 name=name)
 
-- 
1.9.0

[Qemu-devel] [PATCH v4 2/3] qapi: add const prefix to 'char *' insider c_type()

[Amos Kong]

It's ugly to add const prefix for parameter type by a if statement
outsider c_type(). This patch adds a parameter to do it.

Signed-off-by: Amos Kong <akong@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 4 +---
 scripts/qapi.py          | 4 +++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 8d9096f..8f8a258 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -26,9 +26,7 @@ def type_visitor(name):
 def generate_command_decl(name, args, ret_type):
     arglist=""
     for argname, argtype, optional, structured in parse_args(args):
-        argtype = c_type(argtype)
-        if argtype == "char *":
-            argtype = "const char *"
+        argtype = c_type(argtype, is_param=True)
         if optional:
             arglist += "bool has_%s, " % c_var(argname)
         arglist += "%s %s, " % (argtype, c_var(argname))
diff --git a/scripts/qapi.py b/scripts/qapi.py
index ec806aa..1bc2bd9 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -462,8 +462,10 @@ def find_enum(name):
 def is_enum(name):
     return find_enum(name) != None
 
-def c_type(name):
+def c_type(name, is_param=False):
     if name == 'str':
+        if is_param:
+            return 'const char *'
         return 'char *'
     elif name == 'int':
         return 'int64_t'
-- 
1.9.0

[Qemu-devel] [PATCH v4 3/3] qapi: Suppress unwanted space between type and identifier

[Amos Kong]

We always generate a space between type and identifier in parameter
and variable declarations, even when idiomatic C style doesn't have
a space there.  Suppress it.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 scripts/qapi.py | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 1bc2bd9..912787a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -462,11 +462,14 @@ def find_enum(name):
 def is_enum(name):
     return find_enum(name) != None
 
+eatspace = '\033EATSPACE.'
+
 def c_type(name, is_param=False):
     if name == 'str':
         if is_param:
-            return 'const char *'
-        return 'char *'
+            return 'const char *' + eatspace
+        return 'char *' + eatspace
+
     elif name == 'int':
         return 'int64_t'
     elif (name == 'int8' or name == 'int16' or name == 'int32' or
@@ -480,15 +483,15 @@ def c_type(name, is_param=False):
     elif name == 'number':
         return 'double'
     elif type(name) == list:
-        return '%s *' % c_list_type(name[0])
+        return '%s *%s' % (c_list_type(name[0]), eatspace)
     elif is_enum(name):
         return name
     elif name == None or len(name) == 0:
         return 'void'
     elif name == name.upper():
-        return '%sEvent *' % camel_case(name)
+        return '%sEvent *%s' % (camel_case(name), eatspace)
     else:
-        return '%s *' % name
+        return '%s *%s' % (name, eatspace)
 
 def genindent(count):
     ret = ""
@@ -513,7 +516,8 @@ def cgen(code, **kwds):
     return '\n'.join(lines) % kwds + '\n'
 
 def mcgen(code, **kwds):
-    return cgen('\n'.join(code.split('\n')[1:-1]), **kwds)
+    raw = cgen('\n'.join(code.split('\n')[1:-1]), **kwds)
+    return re.sub(re.escape(eatspace) + ' *', '', raw)
 
 def basename(filename):
     return filename.split("/")[-1]
-- 
1.9.0

Re: [Qemu-devel] [PATCH v4 1/3] qapi: fix coding style in parameters list

[Eric Blake]

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

On 05/07/2014 07:14 PM, Amos Kong wrote:
> The space before pointers is redundant.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi-visit.py | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

<rant>
Appears to be unchanged from v2.  Missing the Reviewed-by I gave on v2
for the code, and you were even reminded about that in v3.  I suggested
a better wording for the commit message in v2:

A space after * when declaring a pointer type is redundant.

but that still hasn't been done.  It's frustrating when review comments
are not addressed (even if you don't want to make a particular change,
at least document why keeping things unchanged is preferable, rather
than silently ignoring the review).
</rant>

That said, the change is still correct, so it still deserves:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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] [PATCH v4 2/3] qapi: add const prefix to 'char *' insider c_type()

[Eric Blake]

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

On 05/07/2014 07:14 PM, Amos Kong wrote:
> It's ugly to add const prefix for parameter type by a if statement

s/a if/an if/

> outsider c_type(). This patch adds a parameter to do it.

s/outsider/outside/

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 4 +---
>  scripts/qapi.py          | 4 +++-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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] [PATCH v4 0/3] qapi: fix coding style in generated code

[Luiz Capitulino]

On Thu,  8 May 2014 09:14:37 +0800
Amos Kong <akong@redhat.com> wrote:

> Not a serious issue, but it's helpful if we can fix it.
> 
> V2: split change of scripts/qapi-visit.py to a split patch,
>     eat space by using a special char as Markus suggested
> V3: update commitlog, update special string, fix of adding
>     const replace string by pattern
> V4: fix pattern to cleanup special string (Paolo)

Paolo, can you ACK this now?

Re: [Qemu-devel] [PATCH v4 3/3] qapi: Suppress unwanted space between type and identifier

[Eric Blake]

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

On 05/07/2014 07:14 PM, Amos Kong wrote:
> We always generate a space between type and identifier in parameter
> and variable declarations, even when idiomatic C style doesn't have
> a space there.  Suppress it.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi.py | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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] [PATCH v4 0/3] qapi: fix coding style in generated code

[Paolo Bonzini]

Il 15/05/2014 21:05, Luiz Capitulino ha scritto:
> On Thu,  8 May 2014 09:14:37 +0800
> Amos Kong <akong@redhat.com> wrote:
>
>> Not a serious issue, but it's helpful if we can fix it.
>>
>> V2: split change of scripts/qapi-visit.py to a split patch,
>>     eat space by using a special char as Markus suggested
>> V3: update commitlog, update special string, fix of adding
>>     const replace string by pattern
>> V4: fix pattern to cleanup special string (Paolo)
>
> Paolo, can you ACK this now?
>

Sure!

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

Paolo

Re: [Qemu-devel] [PATCH v4 0/3] qapi: fix coding style in generated code

[Luiz Capitulino]

On Thu,  8 May 2014 09:14:37 +0800
Amos Kong <akong@redhat.com> wrote:

> Not a serious issue, but it's helpful if we can fix it.
> 
> V2: split change of scripts/qapi-visit.py to a split patch,
>     eat space by using a special char as Markus suggested
> V3: update commitlog, update special string, fix of adding
>     const replace string by pattern
> V4: fix pattern to cleanup special string (Paolo)
> 
> Amos Kong (3):
>   qapi: fix coding style in parameters list
>   qapi: add const prefix to 'char *' insider c_type()
>   qapi: Suppress unwanted space between type and identifier
> 
>  scripts/qapi-commands.py |  4 +---
>  scripts/qapi-visit.py    | 20 ++++++++++----------
>  scripts/qapi.py          | 18 ++++++++++++------
>  3 files changed, 23 insertions(+), 19 deletions(-)

Amos, it seems that this series breaks test-qmp-commands (trace below).

I'm not sure what is causing this and at first I thought your series was
only making an existing bug visible, but I took a quick look and it seems
that your last patch changes the code generator to drop the initialization
of generated types pointers in (generated) qmp commands.

Can you please investigate this? And also, please, carry Reviewed-bys
of unmodified patches and address Eric's comments if you respin.

$ make check
[...]
GTESTER tests/test-qmp-commands
*** Error in `tests/test-qmp-commands': free(): invalid pointer: 0x00007f8a2fef3030 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x3926875cff)[0x7f8a2d421cff]
/lib64/libc.so.6(+0x392687cff8)[0x7f8a2d428ff8]
/lib64/libglib-2.0.so.0(g_free+0xf)[0x7f8a2e616f7f]
tests/test-qmp-commands(+0x2b267)[0x7f8a2ef4b267]
tests/test-qmp-commands(+0x2a3fb)[0x7f8a2ef4a3fb]
tests/test-qmp-commands(+0x262ca)[0x7f8a2ef462ca]
tests/test-qmp-commands(+0x2633b)[0x7f8a2ef4633b]
tests/test-qmp-commands(+0x26494)[0x7f8a2ef46494]
tests/test-qmp-commands(+0x29f73)[0x7f8a2ef49f73]
tests/test-qmp-commands(+0x2d2ac)[0x7f8a2ef4d2ac]
tests/test-qmp-commands(+0x2d3de)[0x7f8a2ef4d3de]
tests/test-qmp-commands(+0x291ee)[0x7f8a2ef491ee]
/lib64/libglib-2.0.so.0(+0x392946d5e1)[0x7f8a2e6355e1]
/lib64/libglib-2.0.so.0(+0x392946d7a6)[0x7f8a2e6357a6]
/lib64/libglib-2.0.so.0(g_test_run_suite+0x17b)[0x7f8a2e635b1b]
tests/test-qmp-commands(main+0x9a)[0x7f8a2ef49bac]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f8a2d3cdd65]
tests/test-qmp-commands(+0x4499)[0x7f8a2ef24499]
======= Memory map: ========
7f8a2d3ac000-7f8a2d560000 r-xp 00000000 fd:00 551385                     /usr/lib64/libc-2.18.so
7f8a2d560000-7f8a2d760000 ---p 001b4000 fd:00 551385                     /usr/lib64/libc-2.18.so
7f8a2d760000-7f8a2d764000 r--p 001b4000 fd:00 551385                     /usr/lib64/libc-2.18.so
7f8a2d764000-7f8a2d766000 rw-p 001b8000 fd:00 551385                     /usr/lib64/libc-2.18.so
7f8a2d766000-7f8a2d76b000 rw-p 00000000 00:00 0 
7f8a2d76b000-7f8a2d783000 r-xp 00000000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
7f8a2d783000-7f8a2d982000 ---p 00018000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
7f8a2d982000-7f8a2d983000 r--p 00017000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
7f8a2d983000-7f8a2d984000 rw-p 00018000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
7f8a2d984000-7f8a2d988000 rw-p 00000000 00:00 0 
7f8a2d988000-7f8a2d99d000 r-xp 00000000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
7f8a2d99d000-7f8a2db9c000 ---p 00015000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
7f8a2db9c000-7f8a2db9d000 r--p 00014000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
7f8a2db9d000-7f8a2db9e000 rw-p 00015000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
7f8a2db9e000-7f8a2dca3000 r-xp 00000000 fd:00 569400                     /usr/lib64/libm-2.18.so
7f8a2dca3000-7f8a2dea3000 ---p 00105000 fd:00 569400                     /usr/lib64/libm-2.18.so
7f8a2dea3000-7f8a2dea4000 r--p 00105000 fd:00 569400                     /usr/lib64/libm-2.18.so
7f8a2dea4000-7f8a2dea5000 rw-p 00106000 fd:00 569400                     /usr/lib64/libm-2.18.so
7f8a2dea5000-7f8a2df8e000 r-xp 00000000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
7f8a2df8e000-7f8a2e18e000 ---p 000e9000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
7f8a2e18e000-7f8a2e196000 r--p 000e9000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
7f8a2e196000-7f8a2e198000 rw-p 000f1000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
7f8a2e198000-7f8a2e1ad000 rw-p 00000000 00:00 0 
7f8a2e1ad000-7f8a2e1b1000 r-xp 00000000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
7f8a2e1b1000-7f8a2e3b0000 ---p 00004000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
7f8a2e3b0000-7f8a2e3b1000 r--p 00003000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
7f8a2e3b1000-7f8a2e3b2000 rw-p 00004000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
7f8a2e3b2000-7f8a2e3c7000 r-xp 00000000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
7f8a2e3c7000-7f8a2e5c6000 ---p 00015000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
7f8a2e5c6000-7f8a2e5c7000 r--p 00014000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
7f8a2e5c7000-7f8a2e5c8000 rw-p 00015000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
7f8a2e5c8000-7f8a2e6f1000 r-xp 00000000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
7f8a2e6f1000-7f8a2e8f1000 ---p 00129000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
7f8a2e8f1000-7f8a2e8f2000 r--p 00129000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
7f8a2e8f2000-7f8a2e8f3000 rw-p 0012a000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
7f8a2e8f3000-7f8a2e8f4000 rw-p 00000000 00:00 0 
7f8a2e8f4000-7f8a2e8f5000 r-xp 00000000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
7f8a2e8f5000-7f8a2eaf4000 ---p 00001000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
7f8a2eaf4000-7f8a2eaf5000 r--p 00000000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
7f8a2eaf5000-7f8a2eaf6000 rw-p 00001000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
7f8a2eaf6000-7f8a2eafd000 r-xp 00000000 fd:00 531444                     /usr/lib64/librt-2.18.so
7f8a2eafd000-7f8a2ecfc000 ---p 00007000 fd:00 531444                     /usr/lib64/librt-2.18.so
7f8a2ecfc000-7f8a2ecfd000 r--p 00006000 fd:00 531444                     /usr/lib64/librt-2.18.so
7f8a2ecfd000-7f8a2ecfe000 rw-p 00007000 fd:00 531444                     /usr/lib64/librt-2.18.so
7f8a2ecfe000-7f8a2ed1e000 r-xp 00000000 fd:00 551384                     /usr/lib64/ld-2.18.so
7f8a2eefa000-7f8a2ef02000 rw-p 00000000 00:00 0 
7f8a2ef1b000-7f8a2ef1d000 rw-p 00000000 00:00 0 
7f8a2ef1d000-7f8a2ef1e000 r--p 0001f000 fd:00 551384                     /usr/lib64/ld-2.18.so
7f8a2ef1e000-7f8a2ef1f000 rw-p 00020000 fd:00 551384                     /usr/lib64/ld-2.18.so
7f8a2ef1f000-7f8a2ef20000 rw-p 00000000 00:00 0 
7f8a2ef20000-7f8a2ef65000 r-xp 00000000 fd:04 282340                     /home/lcapitulino/work/src/upstream/qmp-unstable/build/tests/test-qmp-commands
7f8a2f165000-7f8a2f166000 r--p 00045000 fd:04 282340                     /home/lcapitulino/work/src/upstream/qmp-unstable/build/tests/test-qmp-commands
7f8a2f166000-7f8a2f167000 rw-p 00046000 fd:04 282340                     /home/lcapitulino/work/src/upstream/qmp-unstable/build/tests/test-qmp-commands
7f8a2fef0000-7f8a2ff11000 rw-p 00000000 00:00 0                          [heap]
7fffc9896000-7fffc98b8000 rw-p 00000000 00:00 0                          [stack]
7fffc98cd000-7fffc98cf000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
GTester: last random seed: R02S53adc33aac3bcdb0168c5aa5f74a577d
make: *** [check-tests/test-qmp-commands] Error 1

Re: [Qemu-devel] [PATCH v4 1/3] qapi: fix coding style in parameters list

[Amos Kong]

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

On Mon, May 12, 2014 at 08:57:30PM -0600, Eric Blake wrote:
> On 05/07/2014 07:14 PM, Amos Kong wrote:
> > The space before pointers is redundant.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  scripts/qapi-visit.py | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> <rant>
> Appears to be unchanged from v2.  Missing the Reviewed-by I gave on v2
> for the code, and you were even reminded about that in v3.  I suggested
> a better wording for the commit message in v2:
> 
> A space after * when declaring a pointer type is redundant.
> 
> but that still hasn't been done.  It's frustrating when review comments
> are not addressed (even if you don't want to make a particular change,
> at least document why keeping things unchanged is preferable, rather
> than silently ignoring the review).
> </rant>

Sorry about the careless.
 
> That said, the change is still correct, so it still deserves:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
 
Thanks for your patience of the new review-by.

> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

-- 
			Amos.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 0/3] qapi: fix coding style in generated code

[Amos Kong]

On Fri, May 16, 2014 at 10:19:02AM -0400, Luiz Capitulino wrote:
> On Thu,  8 May 2014 09:14:37 +0800
> Amos Kong <akong@redhat.com> wrote:
> 
> > Not a serious issue, but it's helpful if we can fix it.
> > 
> > V2: split change of scripts/qapi-visit.py to a split patch,
> >     eat space by using a special char as Markus suggested
> > V3: update commitlog, update special string, fix of adding
> >     const replace string by pattern
> > V4: fix pattern to cleanup special string (Paolo)
> > 
> > Amos Kong (3):
> >   qapi: fix coding style in parameters list
> >   qapi: add const prefix to 'char *' insider c_type()
> >   qapi: Suppress unwanted space between type and identifier
> > 
> >  scripts/qapi-commands.py |  4 +---
> >  scripts/qapi-visit.py    | 20 ++++++++++----------
> >  scripts/qapi.py          | 18 ++++++++++++------
> >  3 files changed, 23 insertions(+), 19 deletions(-)
> 
> Amos, it seems that this series breaks test-qmp-commands (trace below).

Thanks for the catch.
 
> I'm not sure what is causing this and at first I thought your series was
> only making an existing bug visible, but I took a quick look and it seems
> that your last patch changes the code generator to drop the initialization
> of generated types pointers in (generated) qmp commands.
> 
> Can you please investigate this? And also, please, carry Reviewed-bys
> of unmodified patches and address Eric's comments if you respin.

I found the root reason.

We added a suffix in c_type() for pointer types, and clean it
in mcgen(). But there is a place that we directly check return
value of c_type().

I will respin.

A quick fix:

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 8f8a258..980573e 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -90,7 +92,7 @@ def gen_visitor_input_vars_decl(args):
 bool has_%(argname)s = false;
 ''',
                          argname=c_var(argname))
-        if c_type(argtype).endswith("*"):
+        if c_type(argtype).endswith("*\033EATSPACE."):
             ret += mcgen('''
 %(argtype)s %(argname)s = NULL;
 ''',

Thanks, Amos
 
> $ make check
> [...]
> GTESTER tests/test-qmp-commands
> *** Error in `tests/test-qmp-commands': free(): invalid pointer: 0x00007f8a2fef3030 ***
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x3926875cff)[0x7f8a2d421cff]
> /lib64/libc.so.6(+0x392687cff8)[0x7f8a2d428ff8]
> /lib64/libglib-2.0.so.0(g_free+0xf)[0x7f8a2e616f7f]
> tests/test-qmp-commands(+0x2b267)[0x7f8a2ef4b267]
> tests/test-qmp-commands(+0x2a3fb)[0x7f8a2ef4a3fb]
> tests/test-qmp-commands(+0x262ca)[0x7f8a2ef462ca]
> tests/test-qmp-commands(+0x2633b)[0x7f8a2ef4633b]
> tests/test-qmp-commands(+0x26494)[0x7f8a2ef46494]
> tests/test-qmp-commands(+0x29f73)[0x7f8a2ef49f73]
> tests/test-qmp-commands(+0x2d2ac)[0x7f8a2ef4d2ac]
> tests/test-qmp-commands(+0x2d3de)[0x7f8a2ef4d3de]
> tests/test-qmp-commands(+0x291ee)[0x7f8a2ef491ee]
> /lib64/libglib-2.0.so.0(+0x392946d5e1)[0x7f8a2e6355e1]
> /lib64/libglib-2.0.so.0(+0x392946d7a6)[0x7f8a2e6357a6]
> /lib64/libglib-2.0.so.0(g_test_run_suite+0x17b)[0x7f8a2e635b1b]
> tests/test-qmp-commands(main+0x9a)[0x7f8a2ef49bac]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f8a2d3cdd65]
> tests/test-qmp-commands(+0x4499)[0x7f8a2ef24499]
> ======= Memory map: ========
> 7f8a2d3ac000-7f8a2d560000 r-xp 00000000 fd:00 551385                     /usr/lib64/libc-2.18.so
> 7f8a2d560000-7f8a2d760000 ---p 001b4000 fd:00 551385                     /usr/lib64/libc-2.18.so
> 7f8a2d760000-7f8a2d764000 r--p 001b4000 fd:00 551385                     /usr/lib64/libc-2.18.so
> 7f8a2d764000-7f8a2d766000 rw-p 001b8000 fd:00 551385                     /usr/lib64/libc-2.18.so
> 7f8a2d766000-7f8a2d76b000 rw-p 00000000 00:00 0 
> 7f8a2d76b000-7f8a2d783000 r-xp 00000000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
> 7f8a2d783000-7f8a2d982000 ---p 00018000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
> 7f8a2d982000-7f8a2d983000 r--p 00017000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
> 7f8a2d983000-7f8a2d984000 rw-p 00018000 fd:00 530647                     /usr/lib64/libpthread-2.18.so
> 7f8a2d984000-7f8a2d988000 rw-p 00000000 00:00 0 
> 7f8a2d988000-7f8a2d99d000 r-xp 00000000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
> 7f8a2d99d000-7f8a2db9c000 ---p 00015000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
> 7f8a2db9c000-7f8a2db9d000 r--p 00014000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
> 7f8a2db9d000-7f8a2db9e000 rw-p 00015000 fd:00 551415                     /usr/lib64/libgcc_s-4.8.2-20131212.so.1
> 7f8a2db9e000-7f8a2dca3000 r-xp 00000000 fd:00 569400                     /usr/lib64/libm-2.18.so
> 7f8a2dca3000-7f8a2dea3000 ---p 00105000 fd:00 569400                     /usr/lib64/libm-2.18.so
> 7f8a2dea3000-7f8a2dea4000 r--p 00105000 fd:00 569400                     /usr/lib64/libm-2.18.so
> 7f8a2dea4000-7f8a2dea5000 rw-p 00106000 fd:00 569400                     /usr/lib64/libm-2.18.so
> 7f8a2dea5000-7f8a2df8e000 r-xp 00000000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
> 7f8a2df8e000-7f8a2e18e000 ---p 000e9000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
> 7f8a2e18e000-7f8a2e196000 r--p 000e9000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
> 7f8a2e196000-7f8a2e198000 rw-p 000f1000 fd:00 569411                     /usr/lib64/libstdc++.so.6.0.19
> 7f8a2e198000-7f8a2e1ad000 rw-p 00000000 00:00 0 
> 7f8a2e1ad000-7f8a2e1b1000 r-xp 00000000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
> 7f8a2e1b1000-7f8a2e3b0000 ---p 00004000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
> 7f8a2e3b0000-7f8a2e3b1000 r--p 00003000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
> 7f8a2e3b1000-7f8a2e3b2000 rw-p 00004000 fd:00 535852                     /usr/lib64/libuuid.so.1.3.0
> 7f8a2e3b2000-7f8a2e3c7000 r-xp 00000000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
> 7f8a2e3c7000-7f8a2e5c6000 ---p 00015000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
> 7f8a2e5c6000-7f8a2e5c7000 r--p 00014000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
> 7f8a2e5c7000-7f8a2e5c8000 rw-p 00015000 fd:00 551391                     /usr/lib64/libz.so.1.2.8
> 7f8a2e5c8000-7f8a2e6f1000 r-xp 00000000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
> 7f8a2e6f1000-7f8a2e8f1000 ---p 00129000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
> 7f8a2e8f1000-7f8a2e8f2000 r--p 00129000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
> 7f8a2e8f2000-7f8a2e8f3000 rw-p 0012a000 fd:00 535571                     /usr/lib64/libglib-2.0.so.0.3800.2
> 7f8a2e8f3000-7f8a2e8f4000 rw-p 00000000 00:00 0 
> 7f8a2e8f4000-7f8a2e8f5000 r-xp 00000000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
> 7f8a2e8f5000-7f8a2eaf4000 ---p 00001000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
> 7f8a2eaf4000-7f8a2eaf5000 r--p 00000000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
> 7f8a2eaf5000-7f8a2eaf6000 rw-p 00001000 fd:00 551438                     /usr/lib64/libgthread-2.0.so.0.3800.2
> 7f8a2eaf6000-7f8a2eafd000 r-xp 00000000 fd:00 531444                     /usr/lib64/librt-2.18.so
> 7f8a2eafd000-7f8a2ecfc000 ---p 00007000 fd:00 531444                     /usr/lib64/librt-2.18.so
> 7f8a2ecfc000-7f8a2ecfd000 r--p 00006000 fd:00 531444                     /usr/lib64/librt-2.18.so
> 7f8a2ecfd000-7f8a2ecfe000 rw-p 00007000 fd:00 531444                     /usr/lib64/librt-2.18.so
> 7f8a2ecfe000-7f8a2ed1e000 r-xp 00000000 fd:00 551384                     /usr/lib64/ld-2.18.so
> 7f8a2eefa000-7f8a2ef02000 rw-p 00000000 00:00 0 
> 7f8a2ef1b000-7f8a2ef1d000 rw-p 00000000 00:00 0 
> 7f8a2ef1d000-7f8a2ef1e000 r--p 0001f000 fd:00 551384                     /usr/lib64/ld-2.18.so
> 7f8a2ef1e000-7f8a2ef1f000 rw-p 00020000 fd:00 551384                     /usr/lib64/ld-2.18.so
> 7f8a2ef1f000-7f8a2ef20000 rw-p 00000000 00:00 0 
> 7f8a2ef20000-7f8a2ef65000 r-xp 00000000 fd:04 282340                     /home/lcapitulino/work/src/upstream/qmp-unstable/build/tests/test-qmp-commands
> 7f8a2f165000-7f8a2f166000 r--p 00045000 fd:04 282340                     /home/lcapitulino/work/src/upstream/qmp-unstable/build/tests/test-qmp-commands
> 7f8a2f166000-7f8a2f167000 rw-p 00046000 fd:04 282340                     /home/lcapitulino/work/src/upstream/qmp-unstable/build/tests/test-qmp-commands
> 7f8a2fef0000-7f8a2ff11000 rw-p 00000000 00:00 0                          [heap]
> 7fffc9896000-7fffc98b8000 rw-p 00000000 00:00 0                          [stack]
> 7fffc98cd000-7fffc98cf000 r-xp 00000000 00:00 0                          [vdso]
> ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
> GTester: last random seed: R02S53adc33aac3bcdb0168c5aa5f74a577d
> make: *** [check-tests/test-qmp-commands] Error 1
> 

-- 
			Amos.