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

[Simon Glass]

Hi Heinrich,

On 17 September 2017 at 22:33, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 09/18/2017 12:59 AM, 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 at chromium.org>
> > ---
> >
> >  cmd/bootefi.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 2c9d31c5eb..9aa588eb1b 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -106,11 +106,17 @@ static struct efi_object bootefi_device_obj = {
> >       },
> >  };
> >
> > -/* 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 int efi_init_obj_list(void)
>
> Use efi_status_t as return type.
>
> >  {
> > +     int ret;
> > +
> >       if (efi_obj_list_initalized)
> > -             return;
> > +             return 0;
> >       efi_obj_list_initalized = 1;
> >
> >       list_add_tail(&loaded_image_info_obj.link, &efi_obj_list);
> > @@ -132,12 +138,19 @@ static void efi_init_obj_list(void)
> >               loaded_image_info.device_handle = bootefi_device_path;
> >  #endif
> >  #ifdef CONFIG_GENERATE_SMBIOS_TABLE
> > -     efi_smbios_register();
> > +     ret = efi_smbios_register();
> > +     if (ret)
> > +             goto error;
> >  #endif
> >
> >       /* Initialize EFI runtime services */
> >       efi_reset_system_init();
> >       efi_get_time_init();
> > +
> > +     return 0;
> > +error:
> > +     printf("Error: Cannot set up EFI object list (err=%d)\n", ret);
> > +     return ret;
> >  }
> >
> >  static void *copy_fdt(void *fdt)
> > @@ -219,6 +232,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;
> >
> >       /*
> >        * gd lives in a fixed register which may get clobbered while we execute
> > @@ -258,7 +272,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt)
> >               return -ENOENT;
> >
> >       /* 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);
> > @@ -312,7 +328,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >                */
> >               efi_save_gd();
> >               /* Initialize and populate EFI object list */
> > -             efi_init_obj_list();
> > +             if (efi_init_obj_list())
> > +                     return CMD_RET_FAILURE;
> > +
>
> We are duplicating code here.
>
> efi_save_gd and efi_init_obj_list should have been moved to the start of
> do_bootefi_exec in my patch
> efi_selftest: provide unit test for event services
>

Yes I see that but I wonder if we should wait until your series is
applied before figuring this out?

>
> >               loaded_image_info.device_handle = bootefi_device_path;
> >               loaded_image_info.file_path = bootefi_image_path;
> >               return efi_selftest(&loaded_image_info, &systab);
> >
>

Regards,
Simon