| Message ID | a1ac6a7644eb59203e1d3904baecece82399ab61.1343664167.git.blauwirbel@gmail.com |
|---|---|
| State | New |
| Headers | show |
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', >
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' } }
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.
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. >
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. >>
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. > >> >
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
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
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',