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

[Roy Franz <roy.franz@linaro.org>]

On Thu, Jul 24, 2014 at 12:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> 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.
I'll remove the argument - Ian's module freeing patch removes the need
for this.

>
>> @@ -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?

No, I have fixed this.
>
>> +        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.

Fixed this and above issues

>
>> @@ -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.

Fixed, and I'll review redundant messages.
>
> Jan
>