Re: [Xen-devel] [PATCH V2 06/12] add read_config_file() function for XEN EFI config file

[Roy Franz <roy.franz@xxxxxxxxxx>]

On Thu, Jul 24, 2014 at 12:32 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 22.07.14 at 02:43, <roy.franz@xxxxxxxxxx> wrote:
>> Move open-coded reading of the XEN EFI configuration file into a shared
>> fuction read_config_file().
>
> If the function is shared, why is it being placed in the x86 file instead
> of the shared one (with again no declaration added to the shared
> header)?
>
>> +bool_t __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
>> +                             struct file *cfg, CHAR16 *cfg_file_name,
>> +                             union string *section,
>> +                             CHAR16 *xen_file_name)
>> +{
>> +    /*
>> +     * This allocation is internal to the EFI stub, so any address is
>> +     * fine.
>> +     */
>> +    EFI_PHYSICAL_ADDRESS max = ~0;
>
> Ah, okay, here comes the answer to the question I asked in an
> earlier patch. However, using AllocateMaxAddress with an
> unlimited address seems kind of bogus. I'd prefer this to be done
> cleanly by passing a boolean into the function and having that one
> use AllocateMaxAddress or AllocateAnyPages.
>

This will be going away since Ian's patch resolves the problem this was
addressing.

>> +
>> +    /* Read and parse the config file. */
>> +    if ( !cfg_file_name )
>> +    {
>> +        CHAR16 *tail;
>> +
>> +        while ( (tail = point_tail(xen_file_name)) != NULL )
>> +        {
>> +            wstrcpy(tail, L".cfg");
>> +            if ( read_file(*cfg_dir_handle, xen_file_name, cfg, max) )
>> +                break;
>> +            *tail = 0;
>> +        }
>> +        if ( !tail )
>> +            return 0;
>> +        PrintStr(L"Using configuration file '");
>> +        PrintStr(xen_file_name);
>> +        PrintStr(L"'\r\n");
>> +    }
>> +    else if ( !read_file(*cfg_dir_handle, cfg_file_name, cfg, max) )
>> +        return 0;
>> +    pre_parse(cfg);
>> +
>> +    if ( section->w )
>> +        w2s(section);
>> +    else
>> +        section->s = get_value(cfg, "global", "default");
>> +
>> +
>> +    for ( ; ; )
>> +    {
>> +        union string dom0_kernel_name;
>> +        dom0_kernel_name.s = get_value(cfg, section->s, "kernel");
>> +        if ( dom0_kernel_name.s )
>> +            break;
>> +        dom0_kernel_name.s = get_value(cfg, "global", "chain");
>
> Please name the variable differently if it is used for other than the
> purpose its current name implies.

OK


>
> Also there are again blank line issues above - I'm not going to
> repeat respective comments made in an earlier patch, implying
> that you'll take care of these issues throughout the series.

Yes, I will review the changes again.
>
>> @@ -855,53 +917,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>      if ( EFI_ERROR(status) )
>>          gop = NULL;
>>
>> -    /* Read and parse the config file. */
>> -    if ( !cfg_file_name )
>> -    {
>> -        CHAR16 *tail;
>> +    if ( !read_config_file(&dir_handle, &cfg, cfg_file_name, &section,
>> +                           file_name) )
>> +        blexit(L"Unable to read configuration file.");
>>
>> -        while ( (tail = point_tail(file_name)) != NULL )
>> -        {
>> -            wstrcpy(tail, L".cfg");
>> -            if ( read_file(dir_handle, file_name, &cfg, max_addr) )
>> -                break;
>> -            *tail = 0;
>> -        }
>> -        if ( !tail )
>> -            blexit(L"No configuration file found.");
>> -        PrintStr(L"Using configuration file '");
>> -        PrintStr(file_name);
>> -        PrintStr(L"'\r\n");
>> -    }
>> -    else if ( !read_file(dir_handle, cfg_file_name, &cfg, max_addr) )
>> -        blexit(L"Configuration file not found.");
>> -    pre_parse(&cfg);
>> -
>> -    if ( section.w )
>> -        w2s(&section);
>> -    else
>> -        section.s = get_value(&cfg, "global", "default");
>> -
>> -    for ( ; ; )
>> -    {
>> -        name.s = get_value(&cfg, section.s, "kernel");
>> -        if ( name.s )
>> -            break;
>> -        name.s = get_value(&cfg, "global", "chain");
>> -        if ( !name.s )
>> -            break;
>> -        efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>> -        cfg.addr = 0;
>> -        if ( !read_file(dir_handle, s2w(&name), &cfg, max_addr) )
>> -        {
>> -            PrintStr(L"Chained configuration file '");
>> -            PrintStr(name.w);
>> -            efi_bs->FreePool(name.w);
>> -            blexit(L"'not found.");
>> -        }
>> -        pre_parse(&cfg);
>> -        efi_bs->FreePool(name.w);
>> -    }
>> +    name.s = get_value(&cfg, section.s, "kernel");
>>      if ( !name.s )
>>          blexit(L"No Dom0 kernel image specified.");
>
> This redundant lookup can be avoided by having the function return
> not just a bool_t.

Yup, I'll change this.

Roy

>
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel