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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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@linaro.org> wrote:
> > >
> > > On Wed, 18 Jan 2023 at 17:12, Ilias Apalodimas
> > > <ilias.apalodimas@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.

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@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
> > > >
>