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

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

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.


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