1. Patchew
  2. Xen
  3. View series

[PATCH v5] xen/efi: Update error flow for read_file function

Frediano Ziglio posted 1 patch 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20250628064630.2222323-1-frediano.ziglio@cloud.com
xen/common/efi/boot.c | 80 +++++++++++++++++++++++--------------------
1 file changed, 43 insertions(+), 37 deletions(-)
[PATCH v5] xen/efi: Update error flow for read_file function
Posted by Frediano Ziglio 4 months ago 
Use more explicit goto statements to handle common error code
path instead of a lot of if/else.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Change since v4:
- fixed label indentation.
---
 xen/common/efi/boot.c | 80 +++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 9306dc8953..1019de6950 100644
--- 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:
+    if ( FileHandle )
+        FileHandle->Close(FileHandle);
+
+    PrintErr(what);
+    PrintErr(L" failed for ");
+    PrintErrMesg(name, ret);
+
+    /* not reached */
+    return false;
 }
 
 static bool __init read_section(const EFI_LOADED_IMAGE *image,
-- 
2.43.0
Re: [PATCH v5] xen/efi: Update error flow for read_file function
Posted by Marek Marczykowski-Górecki 3 months, 2 weeks ago 
On Sat, Jun 28, 2025 at 07:46:18AM +0100, Frediano Ziglio wrote:
> Use more explicit goto statements to handle common error code
> path instead of a lot of if/else.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>


> ---
> Change since v4:
> - fixed label indentation.
> ---
>  xen/common/efi/boot.c | 80 +++++++++++++++++++++++--------------------
>  1 file changed, 43 insertions(+), 37 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 9306dc8953..1019de6950 100644
> --- 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:
> +    if ( FileHandle )
> +        FileHandle->Close(FileHandle);
> +
> +    PrintErr(what);
> +    PrintErr(L" failed for ");
> +    PrintErrMesg(name, ret);
> +
> +    /* not reached */
> +    return false;
>  }
>  
>  static bool __init read_section(const EFI_LOADED_IMAGE *image,
> -- 
> 2.43.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH v5] xen/efi: Update error flow for read_file function
Posted by Frediano Ziglio 3 months, 3 weeks ago 
On Sat, Jun 28, 2025 at 7:46 AM Frediano Ziglio
<frediano.ziglio@cloud.com> wrote:
>
> Use more explicit goto statements to handle common error code
> path instead of a lot of if/else.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
> Change since v4:
> - fixed label indentation.
> ---
>  xen/common/efi/boot.c | 80 +++++++++++++++++++++++--------------------
>  1 file changed, 43 insertions(+), 37 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 9306dc8953..1019de6950 100644
> --- 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:
> +    if ( FileHandle )
> +        FileHandle->Close(FileHandle);
> +
> +    PrintErr(what);
> +    PrintErr(L" failed for ");
> +    PrintErrMesg(name, ret);
> +
> +    /* not reached */
> +    return false;
>  }
>
>  static bool __init read_section(const EFI_LOADED_IMAGE *image,

Comments on this?

Frediano

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.