| Message ID | 1352808709-11215-1-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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 > > >
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
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;