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



>
> 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);
>>>      }
>>>
>>
>>