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

[Marek Marczykowski <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, May 20, 2026 at 02:50:57PM +0200, Szymon Acedański wrote:
> 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:

Yes, especially for this case, get_parent_handle() needs to be called
before read_file(). Other calls don't need that, but I'm not sure if having
two ways of calling read_file() would be better.

Speaking of, the dir_handle==NULL case in read_file() is unreachable
now, right? Maybe can be replaced with an assert?

> > > +                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)

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature