scripts: qapi-event.py: support vendor extension

The event code generator barfs when it sees a dot in an event argument, this makes it impossible to support vendor extensions in event arguments as they always contain dots. Fix this by replacing dots by hyphens in the generated code.

PS: Event names and QMP command arguments may suffer from the same issue, but I'm not checking/fixing them today.

Signed-off-by Luiz Capitulino <[email protected]>

---
 scripts/qapi-event.py | 8 ++++----
 scripts/qapi.py       | 4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

Quoted 5 lines · Luiz Capitulino

The event code generator barfs when it sees a dot in an event argument, this makes it impossible to support vendor extensions in event arguments as they always contain dots. Fix this by replacing dots by hyphens in the generated code.

Quoted 8 lines

PS: Event names and QMP command arguments may suffer from the same issue, but I'm not checking/fixing them today.

Signed-off-by Luiz Capitulino <[email protected]>

---
 scripts/qapi-event.py | 8 ++++----
 scripts/qapi.py       | 4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

Reviewed-by Eric Blake <[email protected]>

This is borderline on whether it is a bug fix worth applying in 2.1 - it is fixing something that is new to this release (event-as-qapi) and which affects downstream vendors; but at the same time, it is something which cannot be triggered _except_ by downstream vendors, which are perfectly capable of applying this patch even if it misses 2.1. I'll leave it up to you.

The event code generator barfs when it sees a dot in an event argument, this makes it impossible to support vendor extensions in event arguments as they always contain dots. Fix this by replacing dots by hyphens in the generated code.

PS: Event names and QMP command arguments may suffer from the same issue, but I'm not checking/fixing them today.

Signed-off-by Luiz Capitulino <[email protected]>

---
 scripts/qapi-event.py | 8 ++++----
 scripts/qapi.py       | 4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

scripts/qapi-event.py +4 -4

index 601e307..485694b 100644
@@ -23,11 +23,11 @@ def _generate_event_api_name(event_name, params):
     if params:
         for argname, argentry, optional, structured in parse_args(params):
             if optional:
-                api_name += "bool has_%s,\n" % c_var(argname)
+                api_name += "bool has_%s,\n" % c_arg(argname)
                 api_name += "".ljust(l)
 
             api_name += "%s %s,\n" % (c_type(argentry, is_param=True),
-                                      c_var(argname))
+                                      c_arg(argname))
             api_name += "".ljust(l)
 
     api_name += "Error **errp)"

@@ -98,7 +98,7 @@ def generate_event_implement(api_name, event_name, params):
                 ret += mcgen("""
     if (has_%(var)s) {
 """,
-                             var = c_var(argname))
+                             var = c_arg(argname))
                 push_indent()
 
             if argentry == "str":

@@ -113,7 +113,7 @@ def generate_event_implement(api_name, event_name, params):
     }
 """,
                          var_type = var_type,
-                         var = c_var(argname),
+                         var = c_arg(argname),
                          type = type_name(argentry),
                          name = argname)
 

scripts/qapi.py +0 -0

index f2c6d1f..ddab14d 100644

Quoted 4 lines · Luiz Capitulino

The event code generator barfs when it sees a dot in an event argument, this makes it impossible to support vendor extensions in event arguments as they always contain dots. Fix this by replacing dots by hyphens in the generated code.

Code replaces by underbar, not hyphen.

Quoted 53 lines

PS: Event names and QMP command arguments may suffer from the same issue, but I'm not checking/fixing them today.

Signed-off-by Luiz Capitulino <[email protected]>

---
 scripts/qapi-event.py | 8 ++++----
 scripts/qapi.py       | 4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

scripts/qapi-event.py +4 -4

index 601e307..485694b 100644
@@ -23,11 +23,11 @@ def _generate_event_api_name(event_name, params):
     if params:
         for argname, argentry, optional, structured in parse_args(params):
             if optional:
-                api_name += "bool has_%s,\n" % c_var(argname)
+                api_name += "bool has_%s,\n" % c_arg(argname)
                 api_name += "".ljust(l)
 
             api_name += "%s %s,\n" % (c_type(argentry, is_param=True),
-                                      c_var(argname))
+                                      c_arg(argname))
             api_name += "".ljust(l)
 
     api_name += "Error **errp)"

@@ -98,7 +98,7 @@ def generate_event_implement(api_name, event_name, params):
                 ret += mcgen("""
     if (has_%(var)s) {
 """,
-                             var = c_var(argname))
+                             var = c_arg(argname))
                 push_indent()
 
             if argentry == "str":

@@ -113,7 +113,7 @@ def generate_event_implement(api_name, event_name, params):
     }
 """,
                          var_type = var_type,
-                         var = c_var(argname),
+                         var = c_arg(argname),
                          type = type_name(argentry),
                          name = argname)
 

scripts/qapi.py +0 -0

index f2c6d1f..ddab14d 100644
@@ -434,6 +434,10 @@ def c_var(name, protect=True):
 def c_fun(name, protect=True):
     return c_var(name, protect).replace('.', '_')
 

scripts/qapi.py def c_var(name, protect=True): · line 442

Quoted 6 lines

scripts/qapi.py +4 -0

+# Should be used where vendor extensions are supported
+def c_arg(name):
+	return c_var(name).replace('.', '_')
+
 def c_list_type(name):
     return '%sList' % name

Can anybody think of a use of c_var() that needs '.' preserved?

Quoted 62 lines · On Thu, 10 Jul 2014 16:31:38 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 4 lines · Luiz Capitulino

The event code generator barfs when it sees a dot in an event argument, this makes it impossible to support vendor extensions in event arguments as they always contain dots. Fix this by replacing dots by hyphens in the generated code.

Code replaces by underbar, not hyphen.

Quoted 53 lines

PS: Event names and QMP command arguments may suffer from the same issue, but I'm not checking/fixing them today.

Signed-off-by Luiz Capitulino <[email protected]>

---
 scripts/qapi-event.py | 8 ++++----
 scripts/qapi.py       | 4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

scripts/qapi-event.py +4 -4

index 601e307..485694b 100644
@@ -23,11 +23,11 @@ def _generate_event_api_name(event_name, params):
     if params:
         for argname, argentry, optional, structured in parse_args(params):
             if optional:
-                api_name += "bool has_%s,\n" % c_var(argname)
+                api_name += "bool has_%s,\n" % c_arg(argname)
                 api_name += "".ljust(l)
 
             api_name += "%s %s,\n" % (c_type(argentry, is_param=True),
-                                      c_var(argname))
+                                      c_arg(argname))
             api_name += "".ljust(l)
 
     api_name += "Error **errp)"

@@ -98,7 +98,7 @@ def generate_event_implement(api_name, event_name, params):
                 ret += mcgen("""
     if (has_%(var)s) {
 """,
-                             var = c_var(argname))
+                             var = c_arg(argname))
                 push_indent()
 
             if argentry == "str":

@@ -113,7 +113,7 @@ def generate_event_implement(api_name, event_name, params):
     }
 """,
                          var_type = var_type,
-                         var = c_var(argname),
+                         var = c_arg(argname),
                          type = type_name(argentry),
                          name = argname)
 

scripts/qapi.py +0 -0

index f2c6d1f..ddab14d 100644
@@ -434,6 +434,10 @@ def c_var(name, protect=True):
 def c_fun(name, protect=True):
     return c_var(name, protect).replace('.', '_')
 

Quoted 8 lines

scripts/qapi.py def c_var(name, protect=True): · line 442

Quoted 6 lines

scripts/qapi.py +4 -0

+# Should be used where vendor extensions are supported
+def c_arg(name):
+	return c_var(name).replace('.', '_')
+
 def c_list_type(name):
     return '%sList' % name

Can anybody think of a use of c_var() that needs '.' preserved?

Doing the replace in c_var() breaks some struct accesses in the generated code. I didn't look deeper to determine the users though.

Quoted 4 lines

Quoted 1 line

Can anybody think of a use of c_var() that needs '.' preserved?

Doing the replace in c_var() breaks some struct accesses in the generated code. I didn't look deeper to determine the users though.

Feels like a misuse of c_var() to me.

Dig, dig... aha. generate_visit_struct_fields() joins QAPI names separated by '.', and passes the result to c_var(). I expect such code to break when one of the names contains '.'.

It does indeed; try the appended patch to see it yourself. It generates

struct VersionInfo

{
    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } qemu;

    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } __com.redhat.crap;
    char *package;
};

Conclusion: this is simply a bug that needs fixing.

qapi/common.json +0 -0

index 4e9a21f..74ccde3 100644

Quoted 13 lines · Markus Armbruster

Quoted 4 lines

Quoted 1 line

Can anybody think of a use of c_var() that needs '.' preserved?

Doing the replace in c_var() breaks some struct accesses in the generated code. I didn't look deeper to determine the users though.

Feels like a misuse of c_var() to me.

Dig, dig... aha. generate_visit_struct_fields() joins QAPI names separated by '.', and passes the result to c_var(). I expect such code to break when one of the names contains '.'.

It does indeed; try the appended patch to see it yourself. It generates

Quoted 8 lines

struct VersionInfo

{
    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } qemu;

Wait a minute. Isn't this one of the three cases of nested structs, where we were already arguing that nested structs are evil if we are going to introduce a fuller syntax for optional argument defaults?

Quoted 17 lines

    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } __com.redhat.crap;
    char *package;
};

Conclusion: this is simply a bug that needs fixing.

qapi/common.json +0 -0

index 4e9a21f..74ccde3 100644

qapi/common.json line 56

Quoted 5 lines

qapi/common.json +1 -0

@@ -52,6 +52,7 @@
 ##
 { 'type': 'VersionInfo',
   'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
+           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
            'package': 'str'} }

And the fix may be as simple as ditching support for nested structs in the first place, and rewriting this as:

{ 'type': 'VersionDetails',
'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
{ 'type': 'VersionInfo',
'data': {'qemu': 'VersionDetails',

           '__com.redhat.crap': 'VersionDetails',
           'package': 'str' } }

But the fact that we are still discussing makes it obvious - this is 2.2 material.

Quoted 24 lines · Eric Blake

Quoted 13 lines · Markus Armbruster

Quoted 4 lines

Quoted 1 line

Can anybody think of a use of c_var() that needs '.' preserved?

Doing the replace in c_var() breaks some struct accesses in the generated code. I didn't look deeper to determine the users though.

Feels like a misuse of c_var() to me.

Dig, dig... aha. generate_visit_struct_fields() joins QAPI names separated by '.', and passes the result to c_var(). I expect such code to break when one of the names contains '.'.

It does indeed; try the appended patch to see it yourself. It generates

Quoted 8 lines

struct VersionInfo

{
    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } qemu;

Quoted 3 lines

Wait a minute. Isn't this one of the three cases of nested structs, where we were already arguing that nested structs are evil if we are going to introduce a fuller syntax for optional argument defaults?

Yes.

Quoted 33 lines

Quoted 17 lines

    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } __com.redhat.crap;
    char *package;
};

Conclusion: this is simply a bug that needs fixing.

qapi/common.json +0 -0

index 4e9a21f..74ccde3 100644

qapi/common.json line 56

Quoted 5 lines

qapi/common.json +1 -0

@@ -52,6 +52,7 @@
 ##
 { 'type': 'VersionInfo',
   'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
+           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
            'package': 'str'} }

And the fix may be as simple as ditching support for nested structs in the first place, and rewriting this as:

{ 'type': 'VersionDetails',
'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
{ 'type': 'VersionInfo',
'data': {'qemu': 'VersionDetails',

           '__com.redhat.crap': 'VersionDetails',
           'package': 'str' } }

Quoted 2 lines

But the fact that we are still discussing makes it obvious - this is 2.2 material.

Agree. Let's ditch nested structs and see whether there are any misuses of c_var() left.

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

{ 'type': 'VersionInfo',
'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
+ '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},

            'package': 'str'} }

And the fix may be as simple as ditching support for nested structs in the first place, and rewriting this as:

{ 'type': 'VersionDetails',
'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
{ 'type': 'VersionInfo',
'data': {'qemu': 'VersionDetails',

           '__com.redhat.crap': 'VersionDetails',
           'package': 'str' } }

Quoted 5 lines

Quoted 2 lines

But the fact that we are still discussing makes it obvious - this is 2.2 material.

Agree. Let's ditch nested structs and see whether there are any misuses of c_var() left.

This is an honest question: do we really want to drop nested struct support, wasn't it added by the block layer or am I just confused?

Quoted 15 lines · Luiz Capitulino

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

{ 'type': 'VersionInfo',
'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
+ '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},

            'package': 'str'} }

And the fix may be as simple as ditching support for nested structs in the first place, and rewriting this as:

{ 'type': 'VersionDetails',
'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
{ 'type': 'VersionInfo',
'data': {'qemu': 'VersionDetails',

           '__com.redhat.crap': 'VersionDetails',
           'package': 'str' } }

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

But the fact that we are still discussing makes it obvious - this is 2.2 material.

Agree. Let's ditch nested structs and see whether there are any misuses of c_var() left.

This is an honest question: do we really want to drop nested struct support, wasn't it added by the block layer or am I just confused?

We're talking about raw inline structs - there's only 3 impacted QAPI typesMP commands (if I counted correctly), and they have nothing to do with block layer complex structs. The idea is that we want to outlaw 'foo':{...} implicit structs, and instead require 'foo':'type', where 'type' was earlier defined with the {...} guts. The QMP wire format would be unchanged; it is just a change to the QAPI template that the generators read. Removing inline structs would also simplify the generators. Then, with that gone, we are free to to repurpose 'foo':{...} for default values of optional arguments. Here's a link to some of the earlier conversation:

https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00708.html
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04268.html

Quoted 17 lines

Quoted 5 lines

Quoted 2 lines

Agree. Let's ditch nested structs and see whether there are any misuses of c_var() left.

This is an honest question: do we really want to drop nested struct support, wasn't it added by the block layer or am I just confused?

We're talking about raw inline structs - there's only 3 impacted QAPI typesMP commands (if I counted correctly), and they have nothing to do with block layer complex structs. The idea is that we want to outlaw 'foo':{...} implicit structs, and instead require 'foo':'type', where 'type' was earlier defined with the {...} guts. The QMP wire format would be unchanged; it is just a change to the QAPI template that the generators read. Removing inline structs would also simplify the generators. Then, with that gone, we are free to to repurpose 'foo':{...} for default values of optional arguments. Here's a link to some of the earlier conversation:

Quoted 2 lines

https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00708.html
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04268.html

Right, this makes sense. Thanks for the URLs.

Is there anyone planning on doing this? I don't think I'll have cycles anytime soon... I'd be willing to merge my fix if nobody steps up.

Quoted 20 lines · Luiz Capitulino

Quoted 19 lines

Quoted 5 lines

Quoted 2 lines

Agree. Let's ditch nested structs and see whether there are any misuses of c_var() left.

This is an honest question: do we really want to drop nested struct support, wasn't it added by the block layer or am I just confused?

We're talking about raw inline structs - there's only 3 impacted QAPI typesMP commands (if I counted correctly), and they have nothing to do with block layer complex structs. The idea is that we want to outlaw 'foo':{...} implicit structs, and instead require 'foo':'type', where 'type' was earlier defined with the {...} guts. The QMP wire format would be unchanged; it is just a change to the QAPI template that the generators read. Removing inline structs would also simplify the generators. Then, with that gone, we are free to to repurpose 'foo':{...} for default values of optional arguments. Here's a link to some of the earlier conversation:

https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00708.html
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04268.html

Quoted 4 lines

Right, this makes sense. Thanks for the URLs.

Is there anyone planning on doing this? I don't think I'll have cycles anytime soon... I'd be willing to merge my fix if nobody steps up.

I may take a stab at removing raw inline structs after 2.1 is released, if no one beats me to it.

Quoted 4 lines · Luiz Capitulino

The event code generator barfs when it sees a dot in an event argument, this makes it impossible to support vendor extensions in event arguments as they always contain dots. Fix this by replacing dots by hyphens in the generated code.

Code replaces by underbar, not hyphen.

Quoted 5 lines

PS: Event names and QMP command arguments may suffer from the same issue, but I'm not checking/fixing them today.

Signed-off-by Luiz Capitulino <[email protected]>

---

scripts/qapi.py def c_var(name, protect=True): · line 442

Quoted 6 lines

scripts/qapi.py +4 -0

+# Should be used where vendor extensions are supported
+def c_arg(name):
+	return c_var(name).replace('.', '_')
+
 def c_list_type(name):
     return '%sList' % name

Can anybody think of a use of c_var() that needs '.' preserved?