Re: [PATCH v4 3/3] xen/efi: Update error flow for read_file function

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2025 16:57, Frediano Ziglio wrote:
> On Thu, Jun 26, 2025 at 3:50 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 26.06.2025 15:41, Frediano Ziglio wrote:
>>> On Thu, Jun 26, 2025 at 2:31 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>
>>>> On 26.06.2025 15:10, Frediano Ziglio wrote:
>>>>> --- a/xen/common/efi/boot.c
>>>>> +++ b/xen/common/efi/boot.c
>>>>> @@ -792,6 +792,8 @@ static bool __init read_file(EFI_FILE_HANDLE 
>>>>> dir_handle, CHAR16 *name,
>>>>>
>>>>>      if ( !name )
>>>>>          PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
>>>>> +
>>>>> +    what = L"Open";
>>>>>      if ( dir_handle )
>>>>>          ret = dir_handle->Open(dir_handle, &FileHandle, name,
>>>>>                                 EFI_FILE_MODE_READ, 0);
>>>>> @@ -800,54 +802,58 @@ static bool __init read_file(EFI_FILE_HANDLE 
>>>>> dir_handle, CHAR16 *name,
>>>>>      if ( file == &cfg && ret == EFI_NOT_FOUND )
>>>>>          return false;
>>>>>      if ( EFI_ERROR(ret) )
>>>>> -        what = L"Open";
>>>>> -    else
>>>>> -        ret = FileHandle->SetPosition(FileHandle, -1);
>>>>> +        goto fail;
>>>>> +
>>>>> +    what = L"Seek";
>>>>> +    ret = FileHandle->SetPosition(FileHandle, -1);
>>>>>      if ( EFI_ERROR(ret) )
>>>>> -        what = what ?: L"Seek";
>>>>> -    else
>>>>> -        ret = FileHandle->GetPosition(FileHandle, &size);
>>>>> +        goto fail;
>>>>> +
>>>>> +    what = L"Get size";
>>>>> +    ret = FileHandle->GetPosition(FileHandle, &size);
>>>>>      if ( EFI_ERROR(ret) )
>>>>> -        what = what ?: L"Get size";
>>>>> -    else
>>>>> -        ret = FileHandle->SetPosition(FileHandle, 0);
>>>>> +        goto fail;
>>>>> +
>>>>> +    what = L"Seek";
>>>>> +    ret = FileHandle->SetPosition(FileHandle, 0);
>>>>>      if ( EFI_ERROR(ret) )
>>>>> -        what = what ?: L"Seek";
>>>>> -    else
>>>>> -    {
>>>>> -        file->addr = min(1UL << (32 + PAGE_SHIFT),
>>>>> -                         HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>>>>> -        ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>>>>> -                                    PFN_UP(size), &file->addr);
>>>>> -    }
>>>>> +        goto fail;
>>>>> +
>>>>> +    what = L"Allocation";
>>>>> +    file->addr = min(1UL << (32 + PAGE_SHIFT),
>>>>> +                     HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>>>>> +    ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>>>>> +                                PFN_UP(size), &file->addr);
>>>>>      if ( EFI_ERROR(ret) )
>>>>> -        what = what ?: L"Allocation";
>>>>> -    else
>>>>> -    {
>>>>> -        file->need_to_free = true;
>>>>> -        file->size = size;
>>>>> -        handle_file_info(name, file, options);
>>>>> +        goto fail;
>>>>>
>>>>> -        ret = FileHandle->Read(FileHandle, &file->size, file->str);
>>>>> -        if ( !EFI_ERROR(ret) && file->size != size )
>>>>> -            ret = EFI_ABORTED;
>>>>> -        if ( EFI_ERROR(ret) )
>>>>> -            what = L"Read";
>>>>> -    }
>>>>> +    file->need_to_free = true;
>>>>> +    file->size = size;
>>>>> +    handle_file_info(name, file, options);
>>>>>
>>>>> -    if ( FileHandle )
>>>>> -        FileHandle->Close(FileHandle);
>>>>> +    what = L"Read";
>>>>> +    ret = FileHandle->Read(FileHandle, &file->size, file->str);
>>>>> +    if ( !EFI_ERROR(ret) && file->size != size )
>>>>> +        ret = EFI_ABORTED;
>>>>> +    if ( EFI_ERROR(ret) )
>>>>> +        goto fail;
>>>>>
>>>>> -    if ( what )
>>>>> -    {
>>>>> -        PrintErr(what);
>>>>> -        PrintErr(L" failed for ");
>>>>> -        PrintErrMesg(name, ret);
>>>>> -    }
>>>>> +    FileHandle->Close(FileHandle);
>>>>>
>>>>>      efi_arch_flush_dcache_area(file->ptr, file->size);
>>>>>
>>>>>      return true;
>>>>> +
>>>>> +fail:
>>>>
>>>> Nit: Style (see ./CODING_STYLE).
>>>>
>>>
>>> What specifically? I checked the indentation and it's 4 spaces. if-s
>>> are spaced correctly. About labels I didn't find much on CODING_STYLE
>>> so I opened 3/4 files and most of them are indented with no spaces
>>> (they start at column 1).
>>
>> You didn't search for the word "label" then, did you? Quote:
>>
> 
> I did, I probably mis-typed it.
> 
>> 'Due to the behavior of GNU diffutils "diff -p", labels should be
>>  indented by at least one blank.  Non-case labels inside switch() bodies
>>  are preferred to be indented the same as the block's case labels.'
> 
> I suppose labels should be indented less than the code they refer to,
> so in this case from 1 to 3 spaces. I supposed 2 would be the best
> option.

Except that I think 1 is what we commonly use (levaing aside the many bad
examples that we still have).

Jan