Re: [PATCH 02/14] hw/ppc/spapr.c: fail early if no firmware found in machine_init()

[BALATON Zoltan]

On Mon, 28 Feb 2022, Daniel Henrique Barboza wrote:
> The firmware check consists on a file search (qemu_find_file) and load
> it via load_imag_targphys(). This validation is not dependent on any
> other machine state but it currently being done at the end of
> spapr_machine_init(). This means that we can do a lot of stuff and end
> up failing at the end for something that we can verify right out of the
> gate.
>
> Move this validation to the start of spapr_machine_init() to fail
> earlier.  While we're at it, use g_autofree in the 'filename' pointer.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/spapr.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c74543ace3..4cc204f90d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2707,15 +2707,25 @@ static void spapr_machine_init(MachineState *machine)
>     MachineClass *mc = MACHINE_GET_CLASS(machine);
>     const char *bios_default = spapr->vof ? FW_FILE_NAME_VOF : FW_FILE_NAME;
>     const char *bios_name = machine->firmware ?: bios_default;
> +    g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>     const char *kernel_filename = machine->kernel_filename;
>     const char *initrd_filename = machine->initrd_filename;
>     PCIHostState *phb;
>     int i;
>     MemoryRegion *sysmem = get_system_memory();
>     long load_limit, fw_size;
> -    char *filename;
>     Error *resize_hpt_err = NULL;

Keeping it close to where it's used, i.e. right here at the end of the 
declarations would be easier to read, considering that it also inits it 
right away so checking the value in the next line is more straight 
forward.

Grouping with the simliar filename variables is also reasonable but unless 
the value of those change later just removing them and using the 
machine->{kernel,initrd}_filename directly may be simpler. I did that 
before to mac machines to simplify code.

By the way those are declared const char *, does this filename needs to be 
const too? Maybe moving the bios_* vars here too makes more sense to keep 
them together.

Regards,
BALATON Zoltan

> +    if (!filename) {
> +        error_report("Could not find LPAR firmware '%s'", bios_name);
> +        exit(1);
> +    }
> +    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
> +    if (fw_size <= 0) {
> +        error_report("Could not load LPAR firmware '%s'", filename);
> +        exit(1);
> +    }
> +
>     /*
>      * if Secure VM (PEF) support is configured, then initialize it
>      */
> @@ -2996,18 +3006,6 @@ static void spapr_machine_init(MachineState *machine)
>         }
>     }
>
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> -    if (!filename) {
> -        error_report("Could not find LPAR firmware '%s'", bios_name);
> -        exit(1);
> -    }
> -    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
> -    if (fw_size <= 0) {
> -        error_report("Could not load LPAR firmware '%s'", filename);
> -        exit(1);
> -    }
> -    g_free(filename);
> -
>     /* FIXME: Should register things through the MachineState's qdev
>      * interface, this is a legacy from the sPAPREnvironment structure
>      * which predated MachineState but had a similar function */