qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: BALATON Zoltan
Subject: Re: [PATCH 02/14] hw/ppc/spapr.c: fail early if no firmware found in machine_init()
Date: Mon, 28 Feb 2022 20:28:39 +0100 (CET)

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 */




reply via email to

[Prev in Thread] Current Thread [Next in Thread]