Re: [PATCH v11 16/20] cryptodisk: Fallback to passphrase

[Patrick Colp]

On 2024-04-12 14:13, Stefan Berger wrote:
> On 4/12/24 04:39, Gary Lin via Grub-devel wrote:
> > From: Patrick Colp <patrick.colp@oracle.com>
> >
> > If a protector is specified, but it fails to unlock the disk, fall back
> > to asking for the passphrase. However, an error was set indicating that
> > the protector(s) failed. Later code (e.g., LUKS code) fails as
> > `grub_errno` is now set. Print the existing errors out first, before
>
> Though you are resetting grub_errno now so that LUKS code then does 
> not fail unnecessarily? It's not quite clear what the above wants to 
> convey.

Yeah, that's it. If we do nothing with `grub_errno` here, then code will 
eventually execute that checks if `grub_errno` is set. It'll see that it 
is and fail, even though there was no actual failure, it was just set 
before from when the protector failed. I could change it to read 
something like:

If `grub_errno` was previously set, print out the error and clear 
`grub_errno`.

> > proceeding with the passphrase.
>
>
> > Signed-off-by: Patrick Colp <patrick.colp@oracle.com>
> > Signed-off-by: Gary Lin <glin@suse.com>
> > ---
> >   grub-core/disk/cryptodisk.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 0ca1a5c4d..0dbf601c4 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1191,11 +1191,16 @@ grub_cryptodisk_scan_device_real (const char 
> > *name,
> >             source->name, source->partition != NULL ? "," : "",
> >             part != NULL ? part : N_("UNKNOWN"), dev->uuid);
> >         grub_free (part);
> > -      goto error;
> >       }
> >       if (!cargs->key_len)
> >       {
> > +      if (grub_errno)
> > +    {
> > +      grub_print_error ();
> > +      grub_errno = GRUB_ERR_NONE;
> > +    }
> > +
> >         /* Get the passphrase from the user, if no key data. */
> >         askpass = 1;
> >         part = grub_partition_get_name (source->partition);