[Qemu-devel] [PATCH 0/5] Clang patches

[Qemu-devel] [PATCH 0/5] Clang patches

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

With this patch set, I'm able to compile AREG0 free softmmu targets.

Blue Swirl (5):
  sparc: fix floppy TC line setup
  sparc: fix expression with uninitialized initial value
  qapi: avoid reserved word restrict
  user: fix accidental AREG0 use
  configure: disable a few Clang compiler warnings

 configure           |    5 ++++-
 hw/sun4m.c          |   18 +++++++++++++-----
 linux-user/signal.c |    2 +-
 net/slirp.c         |    6 +++---
 qapi-schema.json    |    4 ++--
 user-exec.c         |   17 ++++++++++++-----
 6 files changed, 35 insertions(+), 17 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH 1/5] sparc: fix floppy TC line setup

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

The qemu_irq for Terminal Count (TC) line between FDC and Slavio misc
device was created only after use, spotted by Clang compiler. Also,
it was not created if the FDC didn't exist.

Rearrange code to fix order. Always create the TC line.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/sun4m.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/sun4m.c b/hw/sun4m.c
index a959261..0f909b5 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -832,6 +832,10 @@ static void cpu_devinit(const char *cpu_model, unsigned int id,
     env->prom_addr = prom_addr;
 }
 
+static void dummy_fdc_tc(void *opaque, int irq, int level)
+{
+}
+
 static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
                           const char *boot_device,
                           const char *kernel_filename,
@@ -942,9 +946,6 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
               serial_hds[0], serial_hds[1], ESCC_CLOCK, 1);
 
     cpu_halt = qemu_allocate_irqs(cpu_halt_signal, NULL, 1);
-    slavio_misc_init(hwdef->slavio_base, hwdef->aux1_base, hwdef->aux2_base,
-                     slavio_irq[30], fdc_tc);
-
     if (hwdef->apc_base) {
         apc_init(hwdef->apc_base, cpu_halt[0]);
     }
@@ -955,8 +956,13 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
         fd[0] = drive_get(IF_FLOPPY, 0, 0);
         sun4m_fdctrl_init(slavio_irq[22], hwdef->fd_base, fd,
                           &fdc_tc);
+    } else {
+        fdc_tc = *qemu_allocate_irqs(dummy_fdc_tc, NULL, 1);
     }
 
+    slavio_misc_init(hwdef->slavio_base, hwdef->aux1_base, hwdef->aux2_base,
+                     slavio_irq[30], fdc_tc);
+
     if (drive_get_max_bus(IF_SCSI) > 0) {
         fprintf(stderr, "qemu: too many SCSI bus\n");
         exit(1);
@@ -1772,16 +1778,18 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
               slavio_irq[1], serial_hds[0], serial_hds[1],
               ESCC_CLOCK, 1);
 
-    slavio_misc_init(0, hwdef->aux1_base, 0, slavio_irq[1], fdc_tc);
-
     if (hwdef->fd_base != (target_phys_addr_t)-1) {
         /* there is zero or one floppy drive */
         memset(fd, 0, sizeof(fd));
         fd[0] = drive_get(IF_FLOPPY, 0, 0);
         sun4m_fdctrl_init(slavio_irq[1], hwdef->fd_base, fd,
                           &fdc_tc);
+    } else {
+        fdc_tc = *qemu_allocate_irqs(dummy_fdc_tc, NULL, 1);
     }
 
+    slavio_misc_init(0, hwdef->aux1_base, 0, slavio_irq[1], fdc_tc);
+
     if (drive_get_max_bus(IF_SCSI) > 0) {
         fprintf(stderr, "qemu: too many SCSI bus\n");
         exit(1);
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/5] sparc: fix expression with uninitialized initial value

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

err was uninitalized, it's not OK to use |=. Spotted by Clang
compiler.

Fix by replacing |= by =.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 linux-user/signal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 97f30d9..3d6b5df 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -2061,7 +2061,7 @@ restore_fpu_state(CPUSPARCState *env, qemu_siginfo_fpu_t *fpu)
         err = __copy_from_user(&env->fpr[0], &fpu->si_float_regs[0],
 	                             (sizeof(unsigned long) * 32));
 #endif
-        err |= __get_user(env->fsr, &fpu->si_fsr);
+        err = __get_user(env->fsr, &fpu->si_fsr);
 #if 0
         err |= __get_user(current->thread.fpqdepth, &fpu->si_fpqdepth);
         if (current->thread.fpqdepth != 0)
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 2/5] sparc: fix expression with uninitialized initial value

[Peter Maydell]

On 30 July 2012 17:04,  <blauwirbel@gmail.com> wrote:
> From: Blue Swirl <blauwirbel@gmail.com>
>
> err was uninitalized, it's not OK to use |=. Spotted by Clang

"uninitialized" (feel free to just fix typo on commit).

> compiler.
>
> Fix by replacing |= by =.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  linux-user/signal.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 97f30d9..3d6b5df 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -2061,7 +2061,7 @@ restore_fpu_state(CPUSPARCState *env, qemu_siginfo_fpu_t *fpu)
>          err = __copy_from_user(&env->fpr[0], &fpu->si_float_regs[0],
>                                      (sizeof(unsigned long) * 32));
>  #endif
> -        err |= __get_user(env->fsr, &fpu->si_fsr);
> +        err = __get_user(env->fsr, &fpu->si_fsr);
>  #if 0
>          err |= __get_user(current->thread.fpqdepth, &fpu->si_fpqdepth);
>          if (current->thread.fpqdepth != 0)

This will need changing again if we ever fix the #if 0-d out
code in this function, but I guess that will be obvious to whoever
does that.

Incidentally, __get_user() can never fail [we catch unreadable memory
earlier when wo do the lock_user_struct] so you could also just
not do anything with its return value. Some of the other targets
rely on this in their signal save/restore code. I think the use
of the return value is mostly in code that was copy-and-pasted
from the Linux kernel (which does use a __get_user() that can fail).

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH 2/5] sparc: fix expression with uninitialized initial value

[Andreas Färber]

Am 30.07.2012 18:13, schrieb Peter Maydell:
> On 30 July 2012 17:04,  <blauwirbel@gmail.com> wrote:
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> err was uninitalized, it's not OK to use |=. Spotted by Clang
> 
> "uninitialized" (feel free to just fix typo on commit).
> 
>> compiler.
>>
>> Fix by replacing |= by =.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  linux-user/signal.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index 97f30d9..3d6b5df 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -2061,7 +2061,7 @@ restore_fpu_state(CPUSPARCState *env, qemu_siginfo_fpu_t *fpu)
>>          err = __copy_from_user(&env->fpr[0], &fpu->si_float_regs[0],
>>                                      (sizeof(unsigned long) * 32));
>>  #endif
>> -        err |= __get_user(env->fsr, &fpu->si_fsr);
>> +        err = __get_user(env->fsr, &fpu->si_fsr);
>>  #if 0
>>          err |= __get_user(current->thread.fpqdepth, &fpu->si_fpqdepth);
>>          if (current->thread.fpqdepth != 0)
> 
> This will need changing again if we ever fix the #if 0-d out
> code in this function, but I guess that will be obvious to whoever
> does that.

You mean the #endif part? Would an explicit err = 0 make things better?

Andreas

> 
> Incidentally, __get_user() can never fail [we catch unreadable memory
> earlier when wo do the lock_user_struct] so you could also just
> not do anything with its return value. Some of the other targets
> rely on this in their signal save/restore code. I think the use
> of the return value is mostly in code that was copy-and-pasted
> from the Linux kernel (which does use a __get_user() that can fail).
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> -- PMM

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/5] sparc: fix expression with uninitialized initial value

[Peter Maydell]

On 30 July 2012 17:59, Andreas Färber <afaerber@suse.de> wrote:
> Am 30.07.2012 18:13, schrieb Peter Maydell:
>> This will need changing again if we ever fix the #if 0-d out
>> code in this function, but I guess that will be obvious to whoever
>> does that.
>
> You mean the #endif part? Would an explicit err = 0 make things better?

Not really, things would still need changing later. Really the
problem is that most of the function is #if 0'd out (and never
called from anywhere); it's just unused stub code so anything
that shuts the compiler up will do, really.

-- PMM

Re: [Qemu-devel] [PATCH 2/5] sparc: fix expression with uninitialized initial value

[Blue Swirl]

On Mon, Jul 30, 2012 at 5:09 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 July 2012 17:59, Andreas Färber <afaerber@suse.de> wrote:
>> Am 30.07.2012 18:13, schrieb Peter Maydell:
>>> This will need changing again if we ever fix the #if 0-d out
>>> code in this function, but I guess that will be obvious to whoever
>>> does that.
>>
>> You mean the #endif part? Would an explicit err = 0 make things better?
>
> Not really, things would still need changing later. Really the
> problem is that most of the function is #if 0'd out (and never
> called from anywhere); it's just unused stub code so anything
> that shuts the compiler up will do, really.

I'll implement the #if 0'd part.

>
> -- PMM

Re: [Qemu-devel] [PATCH 2/5] sparc: fix expression with uninitialized initial value

[Peter Maydell]

On 30 July 2012 18:20, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 5:09 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Not really, things would still need changing later. Really the
>> problem is that most of the function is #if 0'd out (and never
>> called from anywhere); it's just unused stub code so anything
>> that shuts the compiler up will do, really.
>
> I'll implement the #if 0'd part.

That would be cool, but just to be clear, I don't in general
think we should block small cleanup/fix series on "implement
$feature first", so I'm not saying you have to do that now.

-- PMM

[Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[blauwirbel]

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(-)

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',
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Paolo Bonzini]

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',
> 

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Luiz Capitulino]

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' } }

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Blue Swirl]

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.

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Michael Roth]

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.
> 

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Blue Swirl]

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.
>>

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Michael Roth]

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.
> >>
> 

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Blue Swirl]

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

Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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?

Just change c_var() in scripts/qapi.py to have:

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8082af3..34bfbf6 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -131,6 +131,8 @@ def camel_case(name):
     return new_name
 
 def c_var(name):
+    if name in ['restrict']:
+        return 'q_restrict'
     return name.replace('-', '_').lstrip("*")
 
 def c_fun(name):

Then fixup and build failures (make sure to do a clean
first--dependencies seem to be off for qapi).

Regards,

Anthony Liguori

>
> 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',
>> 

[Qemu-devel] [PATCH 4/5] user: fix accidental AREG0 use

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

Global register AREG0 was always assumed to be usable in user-exec.c,
but this is incorrect for several targets.

Fix with #ifdeffery and by using other variables.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 user-exec.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/user-exec.c b/user-exec.c
index b2a4261..7b93fc7 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -18,7 +18,9 @@
  */
 #include "config.h"
 #include "cpu.h"
+#ifndef CONFIG_TCG_PASS_AREG0
 #include "dyngen-exec.h"
+#endif
 #include "disas.h"
 #include "tcg.h"
 
@@ -58,9 +60,11 @@ void cpu_resume_from_signal(CPUArchState *env1, void *puc)
     struct sigcontext *uc = puc;
 #endif
 
+#ifndef CONFIG_TCG_PASS_AREG0
     env = env1;
 
     /* XXX: restore cpu registers saved in host registers */
+#endif
 
     if (puc) {
         /* XXX: use siglongjmp ? */
@@ -74,8 +78,8 @@ void cpu_resume_from_signal(CPUArchState *env1, void *puc)
         sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
 #endif
     }
-    env->exception_index = -1;
-    longjmp(env->jmp_env, 1);
+    env1->exception_index = -1;
+    longjmp(env1->jmp_env, 1);
 }
 
 /* 'pc' is the host PC at which the exception was raised. 'address' is
@@ -89,9 +93,11 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     TranslationBlock *tb;
     int ret;
 
+#ifndef CONFIG_TCG_PASS_AREG0
     if (cpu_single_env) {
         env = cpu_single_env; /* XXX: find a correct solution for multithread */
     }
+#endif
 #if defined(DEBUG_SIGNAL)
     qemu_printf("qemu: SIGSEGV pc=0x%08lx address=%08lx w=%d oldset=0x%08lx\n",
                 pc, address, is_write, *(unsigned long *)old_set);
@@ -103,7 +109,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     }
 
     /* see if it is an MMU fault */
-    ret = cpu_handle_mmu_fault(env, address, is_write, MMU_USER_IDX);
+    ret = cpu_handle_mmu_fault(cpu_single_env, address, is_write,
+                               MMU_USER_IDX);
     if (ret < 0) {
         return 0; /* not an MMU fault */
     }
@@ -115,13 +122,13 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     if (tb) {
         /* the PC is inside the translated code. It means that we have
            a virtual CPU fault */
-        cpu_restore_state(tb, env, pc);
+        cpu_restore_state(tb, cpu_single_env, pc);
     }
 
     /* we restore the process signal mask as the sigreturn should
        do it (XXX: use sigsetjmp) */
     sigprocmask(SIG_SETMASK, old_set, NULL);
-    exception_action(env);
+    exception_action(cpu_single_env);
 
     /* never comes here */
     return 1;
-- 
1.7.2.5

[Qemu-devel] [PATCH 5/5] configure: disable a few Clang compiler warnings

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

Clang compiler warns about a few constructs in QEMU code. It's possible
to avoid those but that needs more work.

Suppress some warnings for Clang compiler. -Wno-unused-value would
conflict with GCC.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 configure |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index c65b5f6..e32f188 100755
--- a/configure
+++ b/configure
@@ -1154,17 +1154,20 @@ if test -z "$werror" ; then
     fi
 fi
 
+# GCC flags
 gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
 gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
 gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
 gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
+# Clang flags
+clang_flags="-Wno-initializer-overrides -Wno-self-assign -Wno-constant-conversion"
 if test "$werror" = "yes" ; then
     gcc_flags="-Werror $gcc_flags"
 fi
 cat > $TMPC << EOF
 int main(void) { return 0; }
 EOF
-for flag in $gcc_flags; do
+for flag in $gcc_flags $clang_flags; do
     if compile_prog "-Werror $flag" "" ; then
 	QEMU_CFLAGS="$QEMU_CFLAGS $flag"
     fi
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 5/5] configure: disable a few Clang compiler warnings

[Stefan Weil]

Am 30.07.2012 18:04, schrieb blauwirbel@gmail.com:
> From: Blue Swirl <blauwirbel@gmail.com>
>
> Clang compiler warns about a few constructs in QEMU code. It's possible
> to avoid those but that needs more work.
>
> Suppress some warnings for Clang compiler. -Wno-unused-value would
> conflict with GCC.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>   configure |    5 ++++-
>   1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/configure b/configure
> index c65b5f6..e32f188 100755
> --- a/configure
> +++ b/configure
> @@ -1154,17 +1154,20 @@ if test -z "$werror" ; then
>       fi
>   fi
>   
> +# GCC flags
>   gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
>   gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
>   gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>   gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
> +# Clang flags
> +clang_flags="-Wno-initializer-overrides -Wno-self-assign -Wno-constant-conversion"

I'd prefer getting these warnings (and not having them disabled)
for compilations without -Werror ("$werror" = "no").

Regards,

Stefan W.


>
>   if test "$werror" = "yes" ; then
>       gcc_flags="-Werror $gcc_flags"
>   fi
>   cat > $TMPC << EOF
>   int main(void) { return 0; }
>   EOF
> -for flag in $gcc_flags; do
> +for flag in $gcc_flags $clang_flags; do
>       if compile_prog "-Werror $flag" "" ; then
>   	QEMU_CFLAGS="$QEMU_CFLAGS $flag"
>       fi

Re: [Qemu-devel] [PATCH 5/5] configure: disable a few Clang compiler warnings

[Blue Swirl]

On Mon, Jul 30, 2012 at 4:56 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 30.07.2012 18:04, schrieb blauwirbel@gmail.com:
>
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> Clang compiler warns about a few constructs in QEMU code. It's possible
>> to avoid those but that needs more work.
>>
>> Suppress some warnings for Clang compiler. -Wno-unused-value would
>> conflict with GCC.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>   configure |    5 ++++-
>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/configure b/configure
>> index c65b5f6..e32f188 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1154,17 +1154,20 @@ if test -z "$werror" ; then
>>       fi
>>   fi
>>   +# GCC flags
>>   gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
>>   gcc_flags="-Wformat-security -Wformat-y2k -Winit-self
>> -Wignored-qualifiers $gcc_flags"
>>   gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs
>> $gcc_flags"
>>   gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
>> +# Clang flags
>> +clang_flags="-Wno-initializer-overrides -Wno-self-assign
>> -Wno-constant-conversion"
>
>
> I'd prefer getting these warnings (and not having them disabled)
> for compilations without -Werror ("$werror" = "no").

On second thought the patch makes little sense, I'll drop it. I was
trying to get everything compiled with -Werror, but that's impossible
anyway with AREG0 conversion unfinished.

>
> Regards,
>
> Stefan W.
>
>
>
>>
>>   if test "$werror" = "yes" ; then
>>       gcc_flags="-Werror $gcc_flags"
>>   fi
>>   cat > $TMPC << EOF
>>   int main(void) { return 0; }
>>   EOF
>> -for flag in $gcc_flags; do
>> +for flag in $gcc_flags $clang_flags; do
>>       if compile_prog "-Werror $flag" "" ; then
>>         QEMU_CFLAGS="$QEMU_CFLAGS $flag"
>>       fi
>
>