Re: [PATCH V5 06/15] Add efi_arch_handle_cmdline() for processing commandline

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

On Mon, Sep 22, 2014 at 5:17 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote:
>> Add arch function for processing the Xen commandline and
>> updating internal structures.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>
> With two small requests for adjustments below
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
>> @@ -256,7 +257,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
>>  }
>>
>>  static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>> -                                    CHAR16 *cmdline, UINTN cmdsize)
>> +                                    CHAR16 *cmdline, UINTN cmdsize,
>> +                                    CHAR16 **options)
>
> I think this would better be named "dom0_options" or "dom0_opts" or
> some such (also in the caller).

I think xen_options would be better, or else I'm really confused :)
This string is combined with the Xen options from the
config file into mbi.cmdline. Dom0 arguments come from the "kernel"
line in the configuration tile.

>
>> @@ -701,14 +701,14 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      dir_handle = get_parent_handle(loaded_image, &file_name);
>>
>>      argc = get_argv(0, NULL, loaded_image->LoadOptions,
>> -                    loaded_image->LoadOptionsSize);
>> +                    loaded_image->LoadOptionsSize, &options);
>
> I don't see why you don't pass NULL here, the more that you add
> a respective check to get_argv().

I don't either - I will change this.
>
> Jan
>