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

On Thu, Jul 24, 2014 at 12:32 AM, Jan Beulich JBeulich@suse.com wrote:

...

...

...

On 22.07.14 at 02:43, roy.franz@linaro.org 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