On 26.06.2025 15:41, Frediano Ziglio wrote:
> On Thu, Jun 26, 2025 at 2:31 PM Jan Beulich <jbeul...@suse.com> 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:
'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.'
Jan
