From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH V2 06/12] add read_config_file() function
 for XEN EFI config file
Date: Thu, 24 Jul 2014 08:32:27 +0100
Message-ID: <53D0D2AB020000780002560B@mail.emea.novell.com>
References: <1405989815-25236-1-git-send-email-roy.franz@linaro.org>
	<1405989815-25236-7-git-send-email-roy.franz@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1405989815-25236-7-git-send-email-roy.franz@linaro.org>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Roy Franz <roy.franz@linaro.org>
Cc: keir@xen.org, ian.campbell@citrix.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, linaro-uefi@lists.linaro.org, fu.wei@linaro.org
List-Id: xen-devel@lists.xenproject.org

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

> +
> +    /* 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.

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.

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

Jan