Patchwork [3/5] qapi: avoid reserved word restrict

login
register
mail settings
Submitter Blue Swirl
Date July 30, 2012, 4:04 p.m.
Message ID <a1ac6a7644eb59203e1d3904baecece82399ab61.1343664167.git.blauwirbel@gmail.com>
Download mbox | patch
Permalink /patch/174035/
State New
Headers show

Comments

Blue Swirl - July 30, 2012, 4:04 p.m.
From: Blue Swirl <blauwirbel@gmail.com>

Clang compiler complained about use of reserved word 'restrict' in SLIRP
and QAPI.

Rename 'restrict' to 'restricted' which also matches other SLIRP code.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 net/slirp.c      |    6 +++---
 qapi-schema.json |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)
Paolo Bonzini - July 31, 2012, 7:28 a.m.
Il 30/07/2012 18:04, blauwirbel@gmail.com ha scritto:
> From: Blue Swirl <blauwirbel@gmail.com>
> 
> Clang compiler complained about use of reserved word 'restrict' in SLIRP
> and QAPI.
> 
> Rename 'restrict' to 'restricted' which also matches other SLIRP code.

Can't do it, this changes the command-line option.

Luiz, Michael, any ideas?

Paolo

> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  net/slirp.c      |    6 +++---
>  qapi-schema.json |    4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/slirp.c b/net/slirp.c
> index 5c2e6b2..8c42b53 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -722,9 +722,9 @@ int net_init_slirp(const NetClientOptions *opts, const char *name,
>      net_init_slirp_configs(user->hostfwd, SLIRP_CFG_HOSTFWD);
>      net_init_slirp_configs(user->guestfwd, 0);
>  
> -    ret = net_slirp_init(vlan, "user", name, user->restrict, vnet, user->host,
> -                         user->hostname, user->tftp, user->bootfile,
> -                         user->dhcpstart, user->dns, user->smb,
> +    ret = net_slirp_init(vlan, "user", name, user->restricted, vnet,
> +                         user->host, user->hostname, user->tftp,
> +                         user->bootfile, user->dhcpstart, user->dns, user->smb,
>                           user->smbserver);
>  
>      while (slirp_configs) {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index bc55ed2..3912430 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1925,7 +1925,7 @@
>  #
>  # @hostname: #optional client hostname reported by the builtin DHCP server
>  #
> -# @restrict: #optional isolate the guest from the host
> +# @restricted: #optional isolate the guest from the host
>  #
>  # @ip: #optional legacy parameter, use net= instead
>  #
> @@ -1956,7 +1956,7 @@
>  { 'type': 'NetdevUserOptions',
>    'data': {
>      '*hostname':  'str',
> -    '*restrict':  'bool',
> +    '*restricted':'bool',
>      '*ip':        'str',
>      '*net':       'str',
>      '*host':      'str',
>
Luiz Capitulino - July 31, 2012, 12:58 p.m.
On Tue, 31 Jul 2012 09:28:43 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 30/07/2012 18:04, blauwirbel@gmail.com ha scritto:
> > From: Blue Swirl <blauwirbel@gmail.com>
> > 
> > Clang compiler complained about use of reserved word 'restrict' in SLIRP
> > and QAPI.
> > 
> > Rename 'restrict' to 'restricted' which also matches other SLIRP code.
> 
> Can't do it, this changes the command-line option.
> 
> Luiz, Michael, any ideas?

I'm not sure how complicated it would be to implement this, but we could add
a 'bind' keyword to the type dict to control mapping between protocol names
and generated variable names. Like this:

{ 'type': 'NetdevUserOptions',
  'data': {
    '*hostname':  'str',
    '*restrict':  'bool',
    ...
    '*hostfwd':   ['String'],
    '*guestfwd':  ['String'] },

  'bind': { 'restrict': 'restricted' } }
Blue Swirl - July 31, 2012, 4:56 p.m.
On Tue, Jul 31, 2012 at 12:58 PM, Luiz Capitulino
<lcapitulino@redhat.com> wrote:
> On Tue, 31 Jul 2012 09:28:43 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> Il 30/07/2012 18:04, blauwirbel@gmail.com ha scritto:
>> > From: Blue Swirl <blauwirbel@gmail.com>
>> >
>> > Clang compiler complained about use of reserved word 'restrict' in SLIRP
>> > and QAPI.
>> >
>> > Rename 'restrict' to 'restricted' which also matches other SLIRP code.
>>
>> Can't do it, this changes the command-line option.
>>
>> Luiz, Michael, any ideas?
>
> I'm not sure how complicated it would be to implement this, but we could add
> a 'bind' keyword to the type dict to control mapping between protocol names
> and generated variable names. Like this:
>
> { 'type': 'NetdevUserOptions',
>   'data': {
>     '*hostname':  'str',
>     '*restrict':  'bool',
>     ...
>     '*hostfwd':   ['String'],
>     '*guestfwd':  ['String'] },
>
>   'bind': { 'restrict': 'restricted' } }

How about prefixing all json-generated field names with for example
'json_'? Should be a simple mechanical change.

In addition to 'restrict', there may also be problems with 'if'
(-drive, HMP drive_add) and maybe also 'auto' as value (several
command line options, HMP pci_add) in the future.
Michael Roth - July 31, 2012, 6:55 p.m.
On Tue, Jul 31, 2012 at 04:56:50PM +0000, Blue Swirl wrote:
> On Tue, Jul 31, 2012 at 12:58 PM, Luiz Capitulino
> <lcapitulino@redhat.com> wrote:
> > On Tue, 31 Jul 2012 09:28:43 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >> Il 30/07/2012 18:04, blauwirbel@gmail.com ha scritto:
> >> > From: Blue Swirl <blauwirbel@gmail.com>
> >> >
> >> > Clang compiler complained about use of reserved word 'restrict' in SLIRP
> >> > and QAPI.
> >> >
> >> > Rename 'restrict' to 'restricted' which also matches other SLIRP code.
> >>
> >> Can't do it, this changes the command-line option.
> >>
> >> Luiz, Michael, any ideas?
> >
> > I'm not sure how complicated it would be to implement this, but we could add
> > a 'bind' keyword to the type dict to control mapping between protocol names
> > and generated variable names. Like this:
> >
> > { 'type': 'NetdevUserOptions',
> >   'data': {
> >     '*hostname':  'str',
> >     '*restrict':  'bool',
> >     ...
> >     '*hostfwd':   ['String'],
> >     '*guestfwd':  ['String'] },
> >
> >   'bind': { 'restrict': 'restricted' } }
> 
> How about prefixing all json-generated field names with for example
> 'json_'? Should be a simple mechanical change.

It's a whole lot of churn though, and clobbers the history for most QMP
functions. It also seems like a strange thing for clang to complain about...

I think special casing is probably the way to go...

Luiz's bind approach seems reasonable, though "alias" might be the more
familiar name.

As an alternative I'll throw out there, the QIDL series introduces the
notion of "annotated" fields, which allow us to pass additional
information to the code generators (instead of just the typename)
regarding how to handle that field internally. So we could do something like:

{ 'type': 'NetdevUserOptions',
  'data': {
    '*hostname':  'str',
    '*restrict': {
      '<annotated>': 'true',
      'type': 'bool',
      'native_name': 'restrict' },
    ... 
    '*hostfwd':   ['String'],
    '*guestfwd':  ['String'] } }

It's pretty flexible, but I'm hesitant to expose in documented schemas.

> 
> In addition to 'restrict', there may also be problems with 'if'
> (-drive, HMP drive_add) and maybe also 'auto' as value (several
> command line options, HMP pci_add) in the future.
>
Blue Swirl - July 31, 2012, 8:38 p.m.
On Tue, Jul 31, 2012 at 6:55 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On Tue, Jul 31, 2012 at 04:56:50PM +0000, Blue Swirl wrote:
>> On Tue, Jul 31, 2012 at 12:58 PM, Luiz Capitulino
>> <lcapitulino@redhat.com> wrote:
>> > On Tue, 31 Jul 2012 09:28:43 +0200
>> > Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >
>> >> Il 30/07/2012 18:04, blauwirbel@gmail.com ha scritto:
>> >> > From: Blue Swirl <blauwirbel@gmail.com>
>> >> >
>> >> > Clang compiler complained about use of reserved word 'restrict' in SLIRP
>> >> > and QAPI.
>> >> >
>> >> > Rename 'restrict' to 'restricted' which also matches other SLIRP code.
>> >>
>> >> Can't do it, this changes the command-line option.
>> >>
>> >> Luiz, Michael, any ideas?
>> >
>> > I'm not sure how complicated it would be to implement this, but we could add
>> > a 'bind' keyword to the type dict to control mapping between protocol names
>> > and generated variable names. Like this:
>> >
>> > { 'type': 'NetdevUserOptions',
>> >   'data': {
>> >     '*hostname':  'str',
>> >     '*restrict':  'bool',
>> >     ...
>> >     '*hostfwd':   ['String'],
>> >     '*guestfwd':  ['String'] },
>> >
>> >   'bind': { 'restrict': 'restricted' } }
>>
>> How about prefixing all json-generated field names with for example
>> 'json_'? Should be a simple mechanical change.
>
> It's a whole lot of churn though, and clobbers the history for most QMP
> functions. It also seems like a strange thing for clang to complain about...

Not really, it's just C99:
http://en.wikipedia.org/wiki/Restrict

Prefixing would solve also future problems: 'if', 'auto', maybe
'static' can make sense for network options (as compared to DHCP) one
day etc.

>
> I think special casing is probably the way to go...
>
> Luiz's bind approach seems reasonable, though "alias" might be the more
> familiar name.
>
> As an alternative I'll throw out there, the QIDL series introduces the
> notion of "annotated" fields, which allow us to pass additional
> information to the code generators (instead of just the typename)
> regarding how to handle that field internally. So we could do something like:
>
> { 'type': 'NetdevUserOptions',
>   'data': {
>     '*hostname':  'str',
>     '*restrict': {
>       '<annotated>': 'true',
>       'type': 'bool',
>       'native_name': 'restrict' },
>     ...
>     '*hostfwd':   ['String'],
>     '*guestfwd':  ['String'] } }
>
> It's pretty flexible, but I'm hesitant to expose in documented schemas.

This could work too.

We could also introduce new names and deprecate old ones, one day we
could remove the old versions. This is bit drastic change to be done
just because a new user invisible implementation makes command line
names clash with C keywords. I'd rather prefix.

>
>>
>> In addition to 'restrict', there may also be problems with 'if'
>> (-drive, HMP drive_add) and maybe also 'auto' as value (several
>> command line options, HMP pci_add) in the future.
>>
Michael Roth - July 31, 2012, 10:30 p.m.
On Tue, Jul 31, 2012 at 08:38:45PM +0000, Blue Swirl wrote:
> On Tue, Jul 31, 2012 at 6:55 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > On Tue, Jul 31, 2012 at 04:56:50PM +0000, Blue Swirl wrote:
> >> On Tue, Jul 31, 2012 at 12:58 PM, Luiz Capitulino
> >> <lcapitulino@redhat.com> wrote:
> >> > On Tue, 31 Jul 2012 09:28:43 +0200
> >> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> >
> >> >> Il 30/07/2012 18:04, blauwirbel@gmail.com ha scritto:
> >> >> > From: Blue Swirl <blauwirbel@gmail.com>
> >> >> >
> >> >> > Clang compiler complained about use of reserved word 'restrict' in SLIRP
> >> >> > and QAPI.
> >> >> >
> >> >> > Rename 'restrict' to 'restricted' which also matches other SLIRP code.
> >> >>
> >> >> Can't do it, this changes the command-line option.
> >> >>
> >> >> Luiz, Michael, any ideas?
> >> >
> >> > I'm not sure how complicated it would be to implement this, but we could add
> >> > a 'bind' keyword to the type dict to control mapping between protocol names
> >> > and generated variable names. Like this:
> >> >
> >> > { 'type': 'NetdevUserOptions',
> >> >   'data': {
> >> >     '*hostname':  'str',
> >> >     '*restrict':  'bool',
> >> >     ...
> >> >     '*hostfwd':   ['String'],
> >> >     '*guestfwd':  ['String'] },
> >> >
> >> >   'bind': { 'restrict': 'restricted' } }
> >>
> >> How about prefixing all json-generated field names with for example
> >> 'json_'? Should be a simple mechanical change.
> >
> > It's a whole lot of churn though, and clobbers the history for most QMP
> > functions. It also seems like a strange thing for clang to complain about...
> 
> Not really, it's just C99:
> http://en.wikipedia.org/wiki/Restrict

As a struct field it didn't seem like there was a case where the usage
would be ambiguous, but yah, it's still justified to complain.

> 
> Prefixing would solve also future problems: 'if', 'auto', maybe
> 'static' can make sense for network options (as compared to DHCP) one
> day etc.
> 

I don't think there's a necessary risk of this happening again. We can
catch it in code review, warn against it in documentation, and even go as
far as adding a list of reserved keywords that the schema parser/code
generators can complain about.

(I'm also somewhat biased because QIDL needs the schema names to align with the
actual field names, since the schema in that case is a description of an
exiting type that is not generated by QAPI. Although it wouldn't be too
hard to add a command-line option to still support that...)

Another option I'll put out there is to have the code generators
special-case `field_name in [ <reserved keywords> ]:` to re-name the
fields internally, and document the behavior for developers. This keeps
our mistakes from spilling out into the QAPI schemas.

> >
> > I think special casing is probably the way to go...
> >
> > Luiz's bind approach seems reasonable, though "alias" might be the more
> > familiar name.
> >
> > As an alternative I'll throw out there, the QIDL series introduces the
> > notion of "annotated" fields, which allow us to pass additional
> > information to the code generators (instead of just the typename)
> > regarding how to handle that field internally. So we could do something like:
> >
> > { 'type': 'NetdevUserOptions',
> >   'data': {
> >     '*hostname':  'str',
> >     '*restrict': {
> >       '<annotated>': 'true',
> >       'type': 'bool',
> >       'native_name': 'restrict' },
> >     ...
> >     '*hostfwd':   ['String'],
> >     '*guestfwd':  ['String'] } }
> >
> > It's pretty flexible, but I'm hesitant to expose in documented schemas.
> 
> This could work too.
> 
> We could also introduce new names and deprecate old ones, one day we
> could remove the old versions. This is bit drastic change to be done
> just because a new user invisible implementation makes command line
> names clash with C keywords. I'd rather prefix.
> 
> >
> >>
> >> In addition to 'restrict', there may also be problems with 'if'
> >> (-drive, HMP drive_add) and maybe also 'auto' as value (several
> >> command line options, HMP pci_add) in the future.
> >>
>
Paolo Bonzini - Aug. 1, 2012, 6:45 a.m.
Il 31/07/2012 22:38, Blue Swirl ha scritto:
>> > It's a whole lot of churn though, and clobbers the history for most QMP
>> > functions. It also seems like a strange thing for clang to complain about...
> Not really, it's just C99:
> http://en.wikipedia.org/wiki/Restrict
> 
> Prefixing would solve also future problems: 'if', 'auto', maybe
> 'static' can make sense for network options (as compared to DHCP) one
> day etc.
> 

We could detect C keywords and prefix (or suffix) an underscore
automatically.

Paolo
Blue Swirl - Aug. 1, 2012, 5:35 p.m.
On Wed, Aug 1, 2012 at 6:45 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 31/07/2012 22:38, Blue Swirl ha scritto:
>>> > It's a whole lot of churn though, and clobbers the history for most QMP
>>> > functions. It also seems like a strange thing for clang to complain about...
>> Not really, it's just C99:
>> http://en.wikipedia.org/wiki/Restrict
>>
>> Prefixing would solve also future problems: 'if', 'auto', maybe
>> 'static' can make sense for network options (as compared to DHCP) one
>> day etc.
>>
>
> We could detect C keywords and prefix (or suffix) an underscore
> automatically.

I'll send a new patch which avoids all C keywords.

>
> Paolo

Patch

diff --git a/net/slirp.c b/net/slirp.c
index 5c2e6b2..8c42b53 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -722,9 +722,9 @@  int net_init_slirp(const NetClientOptions *opts, const char *name,
     net_init_slirp_configs(user->hostfwd, SLIRP_CFG_HOSTFWD);
     net_init_slirp_configs(user->guestfwd, 0);
 
-    ret = net_slirp_init(vlan, "user", name, user->restrict, vnet, user->host,
-                         user->hostname, user->tftp, user->bootfile,
-                         user->dhcpstart, user->dns, user->smb,
+    ret = net_slirp_init(vlan, "user", name, user->restricted, vnet,
+                         user->host, user->hostname, user->tftp,
+                         user->bootfile, user->dhcpstart, user->dns, user->smb,
                          user->smbserver);
 
     while (slirp_configs) {
diff --git a/qapi-schema.json b/qapi-schema.json
index bc55ed2..3912430 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1925,7 +1925,7 @@ 
 #
 # @hostname: #optional client hostname reported by the builtin DHCP server
 #
-# @restrict: #optional isolate the guest from the host
+# @restricted: #optional isolate the guest from the host
 #
 # @ip: #optional legacy parameter, use net= instead
 #
@@ -1956,7 +1956,7 @@ 
 { 'type': 'NetdevUserOptions',
   'data': {
     '*hostname':  'str',
-    '*restrict':  'bool',
+    '*restricted':'bool',
     '*ip':        'str',
     '*net':       'str',
     '*host':      'str',