Re: [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation

[BALATON Zoltan]

On Fri, 14 Oct 2022, Mark Cave-Ayland wrote:
> On 03/10/2022 21:13, BALATON Zoltan wrote:
>
> > By slight reorganisation we can avoid an else branch and some code
> > duplication which makes it easier to follow the code.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >   hw/ppc/mac_newworld.c | 6 +++---
> >   hw/ppc/mac_oldworld.c | 7 +++----
> >   2 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > index 6bc3bd19be..73b01e8c6d 100644
> > --- a/hw/ppc/mac_newworld.c
> > +++ b/hw/ppc/mac_newworld.c
> > @@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
> >                            machine->kernel_filename);
> >               exit(1);
> >           }
> > +        cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> > +                                         KERNEL_GAP);
> >           /* load initrd */
> >           if (machine->initrd_filename) {
> > -            initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + 
> > KERNEL_GAP);
> > +            initrd_base = cmdline_base;
> >               initrd_size = load_image_targphys(machine->initrd_filename,
> >                                                 initrd_base,
> >                                                 machine->ram_size - 
> > initrd_base);
> > @@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
> >                   exit(1);
> >               }
> >               cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
> > -        } else {
> > -            cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + 
> > KERNEL_GAP);
> >           }
> >           ppc_boot_device = 'm';
> >       } else {
> > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > index cb67e44081..b424729a39 100644
> > --- a/hw/ppc/mac_oldworld.c
> > +++ b/hw/ppc/mac_oldworld.c
> > @@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
> >                            machine->kernel_filename);
> >               exit(1);
> >           }
> > +        cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> > +                                         KERNEL_GAP);
> >           /* load initrd */
> >           if (machine->initrd_filename) {
> > -            initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> > -                                            KERNEL_GAP);
> > +            initrd_base = cmdline_base;
> >               initrd_size = load_image_targphys(machine->initrd_filename,
> >                                                 initrd_base,
> >                                                 machine->ram_size - 
> > initrd_base);
> > @@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
> >                   exit(1);
> >               }
> >               cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
> > -        } else {
> > -            cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + 
> > KERNEL_GAP);
> >           }
> >           ppc_boot_device = 'm';
> >       } else {
>
> Is there any particular reason why you would want to avoid the else branch? I

It avoids code duplication and to me it's easier to follow than an else 
branch. With this patch cmdline_base calculation is clearly tied to 
kernel_base and kernel_size if only kernel is used and initrd_base 
initrd_size when initrd is used. With the else branch it's less obvious 
because it's set much later in the else branch while initrd_base 
duplicates it above. So avoiding this duplication makes the code easier to 
read and less prone to errors if the calculation is ever modified.

> don't feel this patch is an improvement since by always setting cmdline_base 
> to a non-zero value, as a reviewer I then have to check all other uses of 
> cmdline_base in the file to ensure that this doesn't cause any issues.

There aren't that many uses that it's difficult to check and this only 
need to be done once.

> I much prefer the existing version since setting the values of cmdline_base 
> and initrd_base is very clearly scoped within the if statement.

What can I say, it's hard to argue with preferences but avoiding code 
duplication is worth the effort reviewing this patch in my opinion.

Regards,
BALATON Zoltan