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_prokopch...@epam.com>
>> ---
>> 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.
