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

[BALATON Zoltan]

On Thu, 29 Sep 2022, Mark Cave-Ayland wrote:
> On 25/09/2022 10:16, 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.
>
> Okay - in that case if you can test with a non-kernel ELF to verify this, and 
> add a note confirming that everything still works for the error paths then 
> that will be fine.

I've originally added non-elf loading to be able to use -bios macrom.bin 
which I've now verified that it still works so this should be OK. I've 
also split this patch up to more parts for easier review in the later 
versions of the series but what it does is basically instead of

int x;
if (cond) {
  x = a;
} else {
  x = 0;
}

we do

int x = 0;
if (cond) {
  x = a;
}

which I thought would be simple to review.

Regards,
BALATON Zoltan