Re: [PATCH v2 for-4.22] EFI: Fix boot from a device without a file system

[Szymon Acedański <accek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, May 20, 2026, at 1:58 PM, Jan Beulich wrote:
> On 20.05.2026 12:30, Szymon Acedański wrote:
> > @@ -1526,31 +1537,33 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
> > ImageHandle,
> >  
> >          gop = efi_get_gop(&gop_handle);
> >  
> > -        /* Get the file system interface. */
> > -        dir_handle = get_parent_handle(loaded_image, &file_name);
> > -
> >          /* Read and parse the config file. */
> >          if ( read_section(loaded_image, L"config", &cfg, NULL) )
> >              PrintStr(L"Using builtin config file\r\n");
> > -        else if ( !cfg_file_name && file_name )
> > +        else
> >          {
> > -            CHAR16 *tail;
> > +            ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> >  
> > -            while ( (tail = point_tail(file_name)) != NULL )
> > +            if ( !cfg_file_name )
> >              {
> > -                wstrcpy(tail, L".cfg");
> > -                if ( read_file(dir_handle, file_name, &cfg, NULL) )
> > -                    break;
> > -                *tail = 0;
> > +                CHAR16 *tail;
> > +
> > +                while ( (tail = point_tail(file_name)) != NULL )
> > +                {
> > +                    wstrcpy(tail, L".cfg");
> > +                    if ( read_file(dir_handle, file_name, &cfg, NULL) )
> > +                        break;
> > +                    *tail = 0;
> > +                }
> > +                if ( !tail )
> > +                    blexit(L"No configuration file found.");
> > +                PrintStr(L"Using configuration file '");
> > +                PrintStr(file_name);
> > +                PrintStr(L"'\r\n");
> >              }
> > -            if ( !tail )
> > -                blexit(L"No configuration file found.");
> > -            PrintStr(L"Using configuration file '");
> > -            PrintStr(file_name);
> > -            PrintStr(L"'\r\n");
> > +            else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
> > +                blexit(L"Configuration file not found.");
> >          }
> > -        else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
> > -            blexit(L"Configuration file not found.");
> >          pre_parse(&cfg);
> >  
> >          if ( section.w )
> 
> Seeing in particular this hunk - why not have read_file() call the new 
> function?

This is because get_parent_handle not only sets dir_handle, but also sets
file_name to something like xen.efi or BOOTX64.EFI. The quoted code then
replaces .efi with .cfg to get the path to the config file to load:
> > +                while ( (tail = point_tail(file_name)) != NULL )
> > +                {
> > +                    wstrcpy(tail, L".cfg");
> > +                    if ( read_file(dir_handle, file_name, &cfg, NULL) )

I considered calling ensure_dir_handle() from read_file() for the other
call sites, but this would:
- still leave the explicit call in the quoted hunk, so it's a bit
  inconsistent (most calls implicit, one explicit)
- requires passing loaded_image to read_file + changing dir_handle
  argument to a pointer

Happy to do it in v3 if you think the call-site savings outweigh
the inconsistency and the extra argument.

> Most of the churn here would then go away.

The hunk above is the restructure of two else-if branches into a single
else block with ensure_dir_handle() on top. Most of the churn is
indentation.

Szymon

(ACK on sending new patch versions as new threads)