Re: [U-Boot] [PATCH v2 03/16] efi: Add error checking for efi_init_obj_list()

Hi Heinrich,

On 4 December 2017 at 15:21, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 12/04/2017 10:28 PM, Simon Glass wrote:

...

This function calls a function which can fail. Print a message in this case and abort the boot, rather than silently continuing to boot, which will certainly fail.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Change return type of efi_init_obj_list() to efi_status_t

  cmd/bootefi.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 17b26e6f4e..a2138f6075 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized; static struct efi_device_path *bootefi_image_path; static struct efi_device_path *bootefi_device_path; -/* Initialize and populate EFI object list */ -static void efi_init_obj_list(void) +/**

• • efi_init_obj_list() - Initialize and populate EFI object list
• • @return 0 if OK, -ve on error (in which case it prints a message)
• */

+static efi_status_t efi_init_obj_list(void) {

•   efi_status_t ret;
  

•   if (efi_obj_list_initalized)
  

•           return;
  

•           return 0;
    efi_obj_list_initalized = 1;
    efi_console_register();
  

@@ -43,12 +49,19 @@ static void efi_init_obj_list(void) efi_net_register();

This function can also fail. Same is true for efi_gop_register.

...

#endif #ifdef CONFIG_GENERATE_SMBIOS_TABLE

•   efi_smbios_register();
  

•   ret = efi_smbios_register();
  

•   if (ret)
  

if (ret != EFI_SUCCESS)

Please can we avoid this obfuscation? It really pains me to see what I consider to be stupidity in the code. Can we just agree that success is 0 in U-Boot/?

...

•           goto error;
  

  #endif /* Initialize EFI runtime services */ efi_reset_system_init(); efi_get_time_init();

•   return EFI_SUCCESS;
  

+error:

•   printf("Error: Cannot set up EFI object list (err=%d)\n", ret);
  

Now we have some objects initialized and others not. Before returning we should clean up. We should free all allocated objects.

OK let me take another look at this.

Best regards

Heinrich

...

•   return ret;
  

  } static void *copy_fdt(void *fdt)

@@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, ulong fdt_pages, fdt_size, fdt_start, fdt_end; const efi_guid_t fdt_guid = EFI_FDT_GUID; bootm_headers_t img = { 0 };

•   int ret;
    /*
     * Special case for efi payload not loaded from disk, such as
  

@@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, "{ro,boot}(blob)0000000000000000"); /* Initialize and populate EFI object list */

•   efi_init_obj_list();
  

•   ret = efi_init_obj_list();
  

•   if (ret)
  

•           return ret;
    /* Call our payload! */
    debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__,
  

(long)entry); @@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) */ efi_save_gd(); /* Initialize and populate EFI object list */

•           if (!efi_obj_list_initalized)
  

•                   efi_init_obj_list();
  

•           if (!efi_obj_list_initalized && efi_init_obj_list())
  

•                   return CMD_RET_FAILURE;
  

•           loaded_image_info.device_handle = bootefi_device_path;
            loaded_image_info.file_path = bootefi_image_path;
  

•           return efi_selftest(&loaded_image_info, &systab);
    } else
  

  #endif

Regards, Simon