Patchwork [06/24] Remove local ram_size that hides global one

login
register
mail settings
Submitter Christian Borntraeger
Date Nov. 13, 2012, 12:11 p.m.
Message ID <1352808709-11215-1-git-send-email-borntraeger@de.ibm.com>
Download mbox | patch
Permalink /patch/198638/
State New
Headers show

Comments

Christian Borntraeger - Nov. 13, 2012, 12:11 p.m.
From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>

The global variable 'ram_size' is hidden by the local variable
declaration in s390_init()

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio.c |    1 -
 1 file changed, 1 deletion(-)
Alexander Graf - Nov. 19, 2012, 1:14 p.m.
On 13.11.2012, at 13:11, Christian Borntraeger wrote:

> From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> 
> The global variable 'ram_size' is hidden by the local variable
> declaration in s390_init()

That's the point of Eduardo's patch, no? Or do we need access to the global to change its value afterwards? If so, please write a reasonable patch description that actually explains the problem.


Alex

> 
> Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390-virtio.c |    1 -
> 1 file changed, 1 deletion(-)
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index ebec844..78477af 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -155,7 +155,6 @@ unsigned s390_del_running_cpu(CPUS390XState *env)
> static void s390_init(QEMUMachineInitArgs *args)
> {
>     ram_addr_t my_ram_size = args->ram_size;
> -    ram_addr_t ram_size = args->ram_size;
>     const char *cpu_model = args->cpu_model;
>     const char *kernel_filename = args->kernel_filename;
>     const char *kernel_cmdline = args->kernel_cmdline;
> -- 
> 1.7.10.1
>
Peter Maydell - Nov. 19, 2012, 1:20 p.m.
On 19 November 2012 13:14, Alexander Graf <agraf@suse.de> wrote:
>
> On 13.11.2012, at 13:11, Christian Borntraeger wrote:
>
>> From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
>>
>> The global variable 'ram_size' is hidden by the local variable
>> declaration in s390_init()
>
> That's the point of Eduardo's patch, no? Or do we need access
> to the global to change its value afterwards?

The function later does:
    /* lets propagate the changed ram size into the global variable. */
    ram_size = my_ram_size;

which frankly I think is pretty nasty but then we've never
had a very good mechanism for specifying board-specific
restrictions around the memory size.

-- PMM
Eduardo Habkost - Nov. 19, 2012, 1:35 p.m.
On Mon, Nov 19, 2012 at 02:14:27PM +0100, Alexander Graf wrote:
> 
> On 13.11.2012, at 13:11, Christian Borntraeger wrote:
> 
> > From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > 
> > The global variable 'ram_size' is hidden by the local variable
> > declaration in s390_init()
> 
> That's the point of Eduardo's patch, no? Or do we need access to the global to change its value afterwards? If so, please write a reasonable patch description that actually explains the problem.
> 

Actually, I wanted to keep the existing behavior (whatever it was), and
use local variables to replace the old function parameters, and change
nothing else. Most machines used a local variable named 'ram_size', but
the parameter to s390_init() was named 'my_ram_size'.

So, considering that this at least restores the previous behavior:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> 
> Alex
> 
> > 
> > Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> > hw/s390-virtio.c |    1 -
> > 1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> > index ebec844..78477af 100644
> > --- a/hw/s390-virtio.c
> > +++ b/hw/s390-virtio.c
> > @@ -155,7 +155,6 @@ unsigned s390_del_running_cpu(CPUS390XState *env)
> > static void s390_init(QEMUMachineInitArgs *args)
> > {
> >     ram_addr_t my_ram_size = args->ram_size;
> > -    ram_addr_t ram_size = args->ram_size;
> >     const char *cpu_model = args->cpu_model;
> >     const char *kernel_filename = args->kernel_filename;
> >     const char *kernel_cmdline = args->kernel_cmdline;
> > -- 
> > 1.7.10.1
> > 
>
Alexander Graf - Nov. 19, 2012, 1:46 p.m.
On 19.11.2012, at 14:35, Eduardo Habkost wrote:

> On Mon, Nov 19, 2012 at 02:14:27PM +0100, Alexander Graf wrote:
>> 
>> On 13.11.2012, at 13:11, Christian Borntraeger wrote:
>> 
>>> From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
>>> 
>>> The global variable 'ram_size' is hidden by the local variable
>>> declaration in s390_init()
>> 
>> That's the point of Eduardo's patch, no? Or do we need access to the global to change its value afterwards? If so, please write a reasonable patch description that actually explains the problem.
>> 
> 
> Actually, I wanted to keep the existing behavior (whatever it was), and
> use local variables to replace the old function parameters, and change
> nothing else. Most machines used a local variable named 'ram_size', but
> the parameter to s390_init() was named 'my_ram_size'.
> 
> So, considering that this at least restores the previous behavior:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Yup, Heinz, please resend with a patch description that actually tells us what the problem is.


Alex

Patch

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index ebec844..78477af 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -155,7 +155,6 @@  unsigned s390_del_running_cpu(CPUS390XState *env)
 static void s390_init(QEMUMachineInitArgs *args)
 {
     ram_addr_t my_ram_size = args->ram_size;
-    ram_addr_t ram_size = args->ram_size;
     const char *cpu_model = args->cpu_model;
     const char *kernel_filename = args->kernel_filename;
     const char *kernel_cmdline = args->kernel_cmdline;