Re: [PATCH for-4.5 V7 11/14] Add arch specific module handling to read_file()

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

On Thu, Sep 25, 2014 at 11:52 AM, Roy Franz <roy.franz@linaro.org> wrote:
> On Thu, Sep 25, 2014 at 9:44 AM, Roy Franz <roy.franz@linaro.org> wrote:
>> On Thu, Sep 25, 2014 at 3:34 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 25.09.14 at 03:42, <roy.franz@linaro.org> wrote:
>>>> +    /*
>>>> +     * 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");
>>>
>>> Kind of disappointing: You said you'd drop these ERROR prefixes,
>>> but this is the second one I come across. I'm ditching them in
>>> preparation for committing.
>>>
>>>> +
>>>> +    local_name.w = ptr;
>>>> +    wstrcpy(local_name.w, name);
>>>> +    w2s(&local_name);
>>>> +
>>>> +    place_string(&mb_modules[mbi.mods_count].string, options);
>>>> +    place_string(&mb_modules[mbi.mods_count].string, "");
>>>
>>> Hmm, this one's still here, and I don't recall having seen an
>>> explanation for it. I guess I need to cut off committing at this
>>> patch then...
>>>
>
> The original code put the entire line from the config file into the
> mb_modules[i].string field,
> which consists of the filename and module options, space separated.
> This was done at the
> same time the filename was extracted to pass to read_file().
>
> My refactored version of read_file() takes a filename and option
> string as separate arguments,
> and then after the file is loaded and address known, the module info is updated.
> The efi_arch_handle_module() gets the filename and options separately,
> and the above place_string()
> commands reconstruct the the single string of filename followed by
> options that is placed
> in the mb_modules[i].string field.
>
> I will add a comment to this effect, and also only add the options if
> present, since for
> several module types options will be NULL.
>

Reviewing this in more detail, the line:

place_string(&mb_modules[mbi.mods_count].string, "");

is not needed and has been removed.  I have verified the same kernel commandline
in dom0 (/proc/cmdline) before and after the change.


>
>>
>> I apologize - I missed adding the above to items to my checklist, and
>> was likely too focused
>> on getting an updated set out yesterday so these items got missed.
>>
>>>> @@ -861,16 +882,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>>>      name.s = get_value(&cfg, section.s, "ramdisk");
>>>>      if ( name.s )
>>>>      {
>>>> -        split_value(name.s);
>>>> -        read_file(dir_handle, s2w(&name), &ramdisk);
>>>> +        option_str = split_string(name.s);
>>>> +        read_file(dir_handle, s2w(&name), &ramdisk, option_str);
>>>
>>> As said before, this should be NULL as not having and never going to
>>> have a consumer. Whether you keep the XSM one below is - as also
>>> said before - up to you.
>>>
>>> Jan
>>
>> I'll drop the options for ramdisk and xsm.  I addresses the options on
>> the ucode module,
>> which is what you had previously commented on.
>>
>> I will rebase on staging ( or master if they get pushed there today)
>> to get the patches you have already committed.
>>
>> Roy
>>
>>>
>>>>          efi_bs->FreePool(name.w);
>>>>      }
>>>>
>>>>      name.s = get_value(&cfg, section.s, "xsm");
>>>>      if ( name.s )
>>>>      {
>>>> -        split_value(name.s);
>>>> -        read_file(dir_handle, s2w(&name), &xsm);
>>>> +        option_str = split_string(name.s);
>>>> +        read_file(dir_handle, s2w(&name), &xsm, option_str);
>>>>          efi_bs->FreePool(name.w);
>>>>      }
>>>>
>>>
>>>