Re: [Xen-devel] [PATCH V2 04/12] Refactor read_file() so it can be shared.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.07.14 at 02:43, <roy.franz@xxxxxxxxxx> wrote:
> The read_file() function updated some multiboot specific data structures
> as it was loading a file.  These changes make read_file() more generic,
> and create a load_file() wrapper for x86 that updates the multiboot
> data structures.  read_file() no longer does special handling of
> the configuration file, as this was only needed to avoid adding
> it to the multiboot structures.  read_file() and load_file() return
> error codes rather than directly exiting on error to facilicate
> sharing.  Different architectures may require different max allocation
> addresses so take that as an argument.

Unless you expect an architecture to pass in different values on
different invocations this clearly can be a #define rather than a
function parameter.

> @@ -405,12 +399,21 @@ static bool_t __init read_file(EFI_FILE_HANDLE 
> dir_handle, CHAR16 *name,
>  
>      if ( what )
>      {
> -        PrintErr(what);
> -        PrintErr(L" failed for ");
> -        PrintErrMesgExit(name, ret);
> +        PrintErrMesg(what, ret);
> +        PrintErr(L"Unable to load file");

Is it intentional to make the message less useful by dripping the
printing of the file name?

> +        return 0;
> +    }
> +    else

No need for an "else" after an unconditional "return" in the "if"
branch.

> @@ -470,6 +473,21 @@ static char *__init get_value(const struct file *cfg, 
> const char *section,
>      }
>      return NULL;
>  }
> +/* Only call with non-config files. */

Missing blank line before this comment.

> +bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> +                               struct file *file)
> +{
> +    EFI_PHYSICAL_ADDRESS max = min(1UL << (32 + PAGE_SHIFT),
> +                                   HYPERVISOR_VIRT_END - 
> DIRECTMAP_VIRT_START);
> +    if ( read_file(dir_handle, name, file, max) )

Missing blank line between declaration and first statement.

> @@ -896,16 +921,20 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>      {
>          microcode_set_module(mbi.mods_count);
>          split_value(name.s);
> -        read_file(dir_handle, s2w(&name), &ucode);
> +        load_ok = load_file(dir_handle, s2w(&name), &ucode);
>          efi_bs->FreePool(name.w);
> +        if ( !load_ok )
> +            blexit(L"Unable to load ucode image.");
>      }
>  
>      name.s = get_value(&cfg, section.s, "xsm");
>      if ( name.s )
>      {
>          split_value(name.s);
> -        read_file(dir_handle, s2w(&name), &xsm);
> +        load_ok = load_file(dir_handle, s2w(&name), &xsm);
>          efi_bs->FreePool(name.w);
> +        if ( !load_ok )
> +            blexit(L"Unable to load ucode image.");

Apart from the same comment made on an earlier patch - no need
for an extra message here when the called function already printed
one - this is a copy'n'paste mistake: You mean XSM instead of ucode
here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel