[PATCH] efi_loader: silence 'Failed to load EFI variables' if the file is missing

[Etienne Carriere]

On Thu, 19 Jan 2023 at 14:47, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Right,
> I'll answer to myself here.
>
> On Thu, 19 Jan 2023 at 15:26, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Thu, 19 Jan 2023 at 15:15, Etienne Carriere
> > <etienne.carriere at linaro.org> wrote:
> > >
> > > On Thu, 19 Jan 2023 at 13:53, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > Hi Heinrich,
> > > > On Thu, Jan 19, 2023 at 01:42:20PM +0100, Heinrich Schuchardt wrote:
> > > > > On 1/19/23 12:45, Ilias Apalodimas wrote:
> > > > > > Hi Etienne,
> > > > > >
> > > > > > On Thu, 19 Jan 2023 at 13:17, Etienne Carriere
> > > > > > <etienne.carriere at linaro.org> wrote:
> > > > > > >
> > > > > > > On Wed, 18 Jan 2023 at 17:12, Ilias Apalodimas
> > > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > > >
> > > > > > > > When we try to load EFI variables from a file in the ESP partition and the
> > > > > > > > file is missing We print a scary error looking like
> > > > > > > > => printenv -e
> > > > > > > > ** Unable to read file ubootefi.var **
> > > > >
> > > > > This message is in line fs/fat/fat.c:1297:
> > > > > printf("** Unable to read file %s **\n", filename);
> > > > >
> > > > > The message is misplaced. If any message is written, it should be in
> > > > > fat_read_at().
> > > > >
> > > > > But I think fat_read_at() should also be quiet. It is the task of the
> > > > > calling application to shout out if a file cannot be read.
> > > > >
> > > > > I have not checked if a test in test/py/tests/test_fs/ relies on the
> > > > > error message.
> > > > >
> > > > > > > > Failed to load EFI variables
> > > > >
> > > > > If there is a writable ESP, file ubootefi.var is created on first boot.
> > > >
> > > > Are you sure?  I think it's created the moment you create an EFI variable.
> > > > So on a board that uses preseeded files you'll always see the message.
> > >
> > > It's created once EFI variables are loaded from preseed.
> > > Later boots load the variables from the file.
> >
> > Unless I am missing something, efi_set_variable_int() is the only
> > caller of efi_var_to_file() which creates that file. That only gets
> > called when a call to SetVariable is made.
> >
>
> We do set the PlatformLang on u-boot.  But as Heinrich pointed out,
> there's a check in that setvariable that shouldn't be there
> (efi_obj_list_initialized == EFI_SUCCESS).  So the file ends up not
> being created

Ok, I saw the patch in the ML.
Thanks for the details.

etienne

>
> Cheers
> /Ilias
> > Regards
> > /Ilias
> > >
> > > >
> > > > Thanks
> > > > /Ilias
> > > > > So this message should only once in the lifecycle of the board.
> > > > >
> > > > > Afterwards this message indicates that something is really wrong.
> > > > >
> > > > > If board vendors doesn't like it, they can create a dummy file.
> > > > >
> > > > > Best regards
> > > > >
> > > > > Heinrich
> > > > >
> > > > > > > >
> > > > > > > > This is not an error though since the file wasn't there to begin with.
> > > > > > > > So silence the warning by aborting the load if the file is not there,
> > > > > > > > instead of failing the load.
> > > > > > > >
> > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > > > > > > ---
> > > > > > > >   lib/efi_loader/efi_var_file.c | 6 ++++++
> > > > > > > >   1 file changed, 6 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> > > > > > > > index 62e071bd8341..7d7141473634 100644
> > > > > > > > --- a/lib/efi_loader/efi_var_file.c
> > > > > > > > +++ b/lib/efi_loader/efi_var_file.c
> > > > > > > > @@ -223,6 +223,12 @@ efi_status_t efi_var_from_file(void)
> > > > > > > >                  return EFI_OUT_OF_RESOURCES;
> > > > > > > >          }
> > > > > > > >
> > > > > > > > +       ret = efi_set_blk_dev_to_system_partition();
> > > > > > > > +       if (ret != EFI_SUCCESS)
> > > > > > > > +               goto error;
> > > > > > > > +       if (!fs_exists(EFI_VAR_FILE_NAME))
> > > > > > > > +               goto error;
> > > > > > > > +
> > > > > > > >          ret = efi_set_blk_dev_to_system_partition();
> > > > > > > >          if (ret != EFI_SUCCESS)
> > > > > > > >                  goto error;
> > > > > > >
> > > > > > > This later call to efi_set_blk_dev_to_system_partition() can be
> > > > > > > removed since already done above.
> > > > > > >
> > > > > >
> > > > > > iirc fs_exists() unconditionally calls fs_close() so we need that double call
> > > > > >
> > > > > > Cheers
> > > > > > /Ilias
> > > > > > > Etienne
> > > > > > >
> > > > > > > > --
> > > > > > > > 2.38.1
> > > > > > > >
> > > > >