Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables

[BALATON Zoltan]

On Sun, 25 Sep 2022, BALATON Zoltan wrote:
> On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
> > On 17/09/2022 00:07, BALATON Zoltan wrote:
> > > Some lines can be dropped making the code flow simpler and easier to
> > > follow by setting default values at variable declaration for some
> > > variables in both mac_oldworld.c and mac_newworld.c.
> > >
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > >   hw/ppc/mac_newworld.c | 28 +++++-----------------------
> > >   hw/ppc/mac_oldworld.c | 27 +++++----------------------
> > >   2 files changed, 10 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > > index 27e4e8d136..6bc3bd19be 100644
> > > --- a/hw/ppc/mac_newworld.c
> > > +++ b/hw/ppc/mac_newworld.c
> > > @@ -111,11 +111,11 @@ static void ppc_core99_init(MachineState *machine)
> > >       CPUPPCState *env = NULL;
> > >       char *filename;
> > >       IrqLines *openpic_irqs;
> > > -    int i, j, k, ppc_boot_device, machine_arch, bios_size;
> > > +    int i, j, k, ppc_boot_device, machine_arch, bios_size = -1;
> > >       const char *bios_name = machine->firmware ?: PROM_FILENAME;
> > >       MemoryRegion *bios = g_new(MemoryRegion, 1);
> > > -    hwaddr kernel_base, initrd_base, cmdline_base = 0;
> > > -    long kernel_size, initrd_size;
> > > +    hwaddr kernel_base = 0, initrd_base = 0, cmdline_base = 0;
> > > +    long kernel_size = 0, initrd_size = 0;
> > >       UNINHostState *uninorth_pci;
> > >       PCIBus *pci_bus;
> > >       PCIDevice *macio;
> > > @@ -130,7 +130,7 @@ static void ppc_core99_init(MachineState *machine)
> > >       DeviceState *dev, *pic_dev;
> > >       DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
> > >       hwaddr nvram_addr = 0xFFF04000;
> > > -    uint64_t tbfreq;
> > > +    uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
> > >         /* init CPUs */
> > >       for (i = 0; i < machine->smp.cpus; i++) {
> > > @@ -165,8 +165,6 @@ static void ppc_core99_init(MachineState *machine)
> > >               bios_size = load_image_targphys(filename, PROM_BASE, 
> > > PROM_SIZE);
> > >           }
> > >           g_free(filename);
> > > -    } else {
> > > -        bios_size = -1;
> > >       }
> > >       if (bios_size < 0 || bios_size > PROM_SIZE) {
> > >           error_report("could not load PowerPC bios '%s'", bios_name);
> > > @@ -174,15 +172,12 @@ static void ppc_core99_init(MachineState *machine)
> > >       }
> > >         if (machine->kernel_filename) {
> > > -        int bswap_needed;
> > > +        int bswap_needed = 0;
> > >     #ifdef BSWAP_NEEDED
> > >           bswap_needed = 1;
> > > -#else
> > > -        bswap_needed = 0;
> > >   #endif
> > >           kernel_base = KERNEL_LOAD_ADDR;
> > > -
> > >           kernel_size = load_elf(machine->kernel_filename, NULL,
> > >                                  translate_kernel_address, NULL, NULL, 
> > > NULL,
> > >                                  NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
> > > @@ -212,16 +207,10 @@ static void ppc_core99_init(MachineState *machine)
> > >               }
> > >               cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
> > >           } else {
> > > -            initrd_base = 0;
> > > -            initrd_size = 0;
> >
> > This bit seems a bit odd...
> >
> > >               cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + 
> > > KERNEL_GAP);
> > >           }
> > >           ppc_boot_device = 'm';
> > >       } else {
> > > -        kernel_base = 0;
> > > -        kernel_size = 0;
> > > -        initrd_base = 0;
> > > -        initrd_size = 0;
> >
> > and also here. The only reason I can think that someone would explicitly 
> > set these variables back to 0 would be if there are cases in the load_*() 
> > functions where non-zero values could be returned for failure. It's worth 
> > having a look to confirm this and see if this also needs some additional 
> > tweaks to the logic flow here.
>
> They aren't set back to 0 but set here the first time. Nothing touches these 
> variables before this if-else do this patch just moves the zero init to the 
> variable declaration and only leaves the cases which set a value different 
> than zero here which I think is easier to follow.

Checked again in the original before this patch to make sure but these are 
in else branches of ifs setting the same variable to some value so I 
think it's either set to value or 0 and these lines settin 0 can'r run if 
the other part setting a value has run so these can't set it back, these 
are just the default 0 values so setting that at the beginning can drop 
these lines. What am I missing? How can these set a value back to 0?

Regards,
BALATON Zoltan