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

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