Re: [PATCH V5 10/15] Add arch specific module handling to read_file()

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

On Tue, Sep 23, 2014 at 6:23 PM, Roy Franz <roy.franz@linaro.org> wrote:
> On Mon, Sep 22, 2014 at 6:57 PM, Roy Franz <roy.franz@linaro.org> wrote:
>> On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 19.09.14 at 00:50, <roy.franz@linaro.org> wrote:
>>>> --- a/xen/common/efi/boot.c
>>>> +++ b/xen/common/efi/boot.c
>>>> @@ -56,12 +56,13 @@ static void noreturn blexit(const CHAR16 *str);
>>>>  static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
>>>>  static char *get_value(const struct file *cfg, const char *section,
>>>>                                const char *item);
>>>> -static void  split_value(char *s);
>>>> +static char *split_string(char *s);
>>>>  static CHAR16 *s2w(union string *str);
>>>>  static char *w2s(const union string *str);
>>>> -static bool_t  read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>>> -                               struct file *file);
>>>> +static size_t wstrlen(const CHAR16 * s);
>>>>  static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
>>>> +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file,
>>>> +                               CHAR16 *filename, char *options);
>>>
>>> Please don't needlessly move this declaration down.
>>>
>> I'll fix this.
>>
>>>> @@ -115,6 +116,15 @@ static void __init DisplayUint(UINT64 Val, INTN Width)
>>>>      PrintStr(PrintString);
>>>>  }
>>>>
>>>> +static size_t __init wstrlen(const CHAR16 * s)
>>>> +{
>>>> +     const CHAR16 *sc;
>>>> +
>>>> +     for (sc = s; *sc != L'\0'; ++sc)
>>>> +             /* nothing */;
>>>> +     return sc - s;
>>>> +}
>>>
>>> Coding style (many issues).
>>
>> static size_t __init wstrlen(const CHAR16 *s)
>> {
>>      const CHAR16 *sc;
>>
>>      for ( sc = s; *sc != L'\0'; ++sc )
>>             ;
>>      return sc - s;
>> }
>>
>> Is the above OK?  Not sure what else to change here...
>>
>>>
>>>> @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn)
>>>>              break;
>>>>          }
>>>>  }
>>>> +/*
>>>> + * Truncate string at first space, and return pointer
>>>> + * to remainder of string.
>>>
>>> ... (if any).
>>>
>>
>> Sure.
>>
>>
>>>> + */
>>>> +static char * __init split_string(char *s)
>>>> +{
>>>> +    while ( *s && !isspace(*s) )
>>>> +        ++s;
>>>> +    if ( *s )
>>>> +    {
>>>> +        *s = 0;
>>>> +        return(s + 1);
>>>
>>> No parentheses here please.
>>
>> sure
>>
>>
>>>
>>>> +    }
>>>> +    return NULL;
>>>> +}
>>>>
>>>> -static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>>> -                               struct file *file)
>>>> +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file,
>>>> +                               CHAR16 *filename, char *options)
>>>
>>> Is the renaming from "name" to "filename" really necessary/useful?
>>> And the flipping of the order of pre-existing parameters?
>>
>> I'll fix this.
>>
>>>
>>>> @@ -659,18 +678,19 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
>>>> *SystemTable)
>>>>      EFI_LOADED_IMAGE *loaded_image;
>>>>      EFI_STATUS status;
>>>>      unsigned int i, argc;
>>>> -    CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
>>>> +    CHAR16 **argv, *options = NULL;
>>>>      UINTN cols, rows, depth, size, info_size, gop_mode = ~0;
>>>>      EFI_HANDLE *handles = NULL;
>>>>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>>>>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>>>>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>>>>      EFI_FILE_HANDLE dir_handle;
>>>> -    union string section = { NULL }, name;
>>>> +    union string section = { NULL }, name, file_name, cfg_file_name = {NULL};
>>>
>>> Your addition should be consistent with existing code (blanks around
>>> NULL). But - why are you changing the types of the two variables in
>>> the first place? Afaics all references to them now are using the .w
>>> member access, suggesting that this is just a remnant of your earlier
>>> version changes.
>>>
>>
>> Yes, this is leftover type changes from a previous version.   Both
>> these should be able to to be
>> reverted to simple CHAR16 *
>>>> @@ -274,8 +275,8 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect
>>>>      if ( name.s )
>>>>      {
>>>>          microcode_set_module(mbi.mods_count);
>>>> -        split_value(name.s);
>>>> -        read_file(dir_handle, s2w(&name), &ucode);
>>>> +        options = split_string(name.s);
>>>> +        read_file(dir_handle, &ucode, s2w(&name), options);
>>>
>>> Perhaps rather NULL instead of options?
>>
>> Yup.
>>
> OK, I looked at this a little more, and the original code would take
> anything after the ucode filename
> and put it into the mbi.string field for the ucode module.  All the
> modules where treated the same in this
> regard - they could all have options.
>
> If ucode never has options, I can remove this, but this would be a
> change in behavior.

The EFI config file documentation specifies just the filename, so I
have simplified this
in the next version.

>
>
>>>
>>>> @@ -575,3 +576,30 @@ static void __init efi_arch_memory(void)
>>>>      l3_bootmap[l3_table_offset(xen_phys_start + (8 << L2_PAGETABLE_SHIFT) - 1)] =
>>>>          l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR);
>>>>  }
>>>> +
>>>> +static void __init efi_arch_handle_module(struct file *file, const CHAR16 *name,
>>>> +                                          char *options)
>>>> +{
>>>> +    union string local_name;
>>>> +    void *ptr;
>>>> +
>>>> +    /*
>>>> +     * Make a copy, as conversion is destructive, and caller still wants
>>>> +     * wide string available after this call returns.
>>>> +     */
>>>> +    if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * sizeof(*name),
>>>> +                              &ptr) != EFI_SUCCESS )
>>>> +        blexit(L"ERROR Unable to allocate string buffer\r\n");
>>>
>>> Iirc I said this before, but just in case: No explicit newline on strings
>>> passed to blexit() please.
>> Yes.
>>>
>>> Jan