Re: [PATCH] efi: remove unreachable code in read_file()

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 8/14/25 11:23, Jan Beulich wrote:
> On 12.08.2025 21:17, Dmytro Prokopchuk1 wrote:
>> MISRA C Rule 2.1 states: "A project shall not contain unreachable code."
>>
>> Function `PrintErrMesg(const CHAR16*, EFI_STATUS)` isn't intended to return
>> control to its caller. At the end, it calls `blexit()`, which, in turn,
>> invokes the `__builtin_unreachable()` function, making subsequent return
>> statements in `read_file()` unreachable:
>>
>>      PrintErrMesg(name, ret);
>>      /* not reached */
>>      return false;
>>
>> This also causes unreachability of the code meant to handle `read_file()`
>> errors, as seen in these examples:
>>
>> In this block:
>>      if ( read_file(dir_handle, file_name, &cfg, NULL) )
>>          break;
>>      *tail = 0;
>>      }
>>      if ( !tail )
>>          blexit(L"No configuration file found.");
>>
>> And here:
>>      else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
>>          blexit(L"Configuration file not found.");
>>
>> And here:
>>      if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
>>      {
>>          PrintStr(L"Chained configuration file '");
>>          PrintStr(name.w);
>>          efi_bs->FreePool(name.w);
>>          blexit(L"'not found.");
>>      }
>>
>> The issue arises because when an error occurs inside `read_file()`, it calls
>> `PrintErrMesg()` and does not return to the caller.
>>
>> To address this the following changes are applied:
>> 1. Remove `PrintErrMesg(name, ret);` from the `read_file()` function.
>> 2. Replaced it with `PrintErr(name);`, which prints the file name and returns
>>     control to the caller.
>> 3. Change the `read_file()` return type from `bool` to `EFI_STATUS`, allowing
>>     file operation result codes to be returned to the caller.
>> 4. Properly handle error codes returned from the `read_file()` function in 
>> the
>>     relevant areas of the code.
>> 5. Replace `blexit()` calls with informative error codes using 
>> `PrintErrMesg()`
>>     where appropriate.
>>
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
>> ---
>> Test CI pipeline:
>> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1980590118
>> ---
>>   xen/common/efi/boot.c | 57 ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 40 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index 50ff1d1bd2..ddbafb2f9c 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -132,7 +132,7 @@ struct file {
>>       };
>>   };
>>
>> -static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>> +static EFI_STATUS read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>                         struct file *file, const char *options);
>>   static bool read_section(const EFI_LOADED_IMAGE *image, const CHAR16 *name,
>>                            struct file *file, const char *options);
>> @@ -782,7 +782,7 @@ static void __init handle_file_info(const CHAR16 *name,
>>       efi_arch_handle_module(file, name, options);
>>   }
>>
>> -static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>> +static EFI_STATUS __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>                                struct file *file, const char *options)
>>   {
>>       EFI_FILE_HANDLE FileHandle = NULL;
>> @@ -791,7 +791,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
>> CHAR16 *name,
>>       const CHAR16 *what = NULL;
>>
>>       if ( !name )
>> -        PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
>> +        return EFI_INVALID_PARAMETER;
>
> Why the change in error code? EFI_OUT_OF_RESOURCES() was used deliberately for
> cases where the result of s2w() is passed directly into here.
>
> Between this hunk and ...
>
>> @@ -842,7 +842,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
>> CHAR16 *name,
>>
>>       efi_arch_flush_dcache_area(file->ptr, file->size);
>>
>> -    return true;
>> +    return ret;
>>
>>    fail:
>>       if ( FileHandle )
>
> ... this one there's at least one "return false" which you leave untouched, 
> thus
> wrongly reporting EFI_SUCCESS now to the caller.
>
>> @@ -850,10 +850,9 @@ static bool __init read_file(EFI_FILE_HANDLE 
>> dir_handle, CHAR16 *name,
>>
>>       PrintErr(what);
>>       PrintErr(L" failed for ");
>> -    PrintErrMesg(name, ret);
>> +    PrintErr(name);
>>
>> -    /* not reached */
>> -    return false;
>> +    return ret;
>>   }
>
> With the comment here - possibly adjusted to become a SAF one - all should be
> fine with no other changes? Because of the other "return false" callers simply
> can't assume the function would never indicate failure back to them. (New
> "return false" could in principle also appear, which is why I think the base
> structure wants keeping as is, including in the callers.)
>
> Jan

Yes, probably the deviation is better in such case, that changing code
and introducing new errors.

Dmytro.