Re: [PATCH V2 05/12] replace split_value() with truncate_string()

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

On Thu, Jul 24, 2014 at 12:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
>> --- a/xen/arch/x86/efi/boot.c
>> +++ b/xen/arch/x86/efi/boot.c
>> @@ -466,7 +466,13 @@ static char *__init get_value(const struct file *cfg, const char *section,
>>              break;
>>          default:
>>              if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' )
>> -                return ptr + ilen + 1;
>> +            {
>> +                ptr += ilen + 1;
>> +                /* strip off any leading spaces */
>
> Coding style.
>
>> +                while ( *ptr && isspace(*ptr) )
>> +                    ptr++;
>> +                return ptr;
>> +            }
>
> It's unclear how this whole hunk is related to the patch subject.
>
>> @@ -489,14 +495,19 @@ bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>      return 0;
>>  }
>>
>> -static void __init split_value(char *s)
>> +/* Truncate string at first space, and return pointer
>> + * to remainder of string.
>> + */
>
> Coding style again.
>
>> +char * __init truncate_string(char *s)
>
> Non-static function without declaration in any header.
>
>>  {
>> -    while ( *s && isspace(*s) )
>> -        ++s;
>> -    place_string(&mb_modules[mbi.mods_count].string, s);
>>      while ( *s && !isspace(*s) )
>>          ++s;
>> -    *s = 0;
>> +    if (*s)
>> +    {
>> +        *s = 0;
>> +        return(s + 1);
>> +    }
>> +    return(NULL);
>
> None of the callers uses the return value - why is the function return
> type not "void"? Also, if there is a reason, then no parentheses around
> the return expression please.

I am making these changes to make the basic functionality share-able
with the arm64
stub.  The users of this function's return value come in when the arm
stub is added.
The declaration is added to efi-shared.h when it is moved - I can make
it static here until then.
I very deliberately am separating code refactoring from code movement
patches which causes
the separation between the creation of a function with a return value
that is not currently used.

>
>> @@ -893,7 +904,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      }
>>      if ( !name.s )
>>          blexit(L"No Dom0 kernel image specified.");
>> -    split_value(name.s);
>> +    place_string(&mb_modules[mbi.mods_count].string, name.s);
>> +    truncate_string(name.s);
>>      load_ok = load_file(dir_handle, s2w(&name), &kernel);
>>      efi_bs->FreePool(name.w);
>>      if ( !load_ok )
>> @@ -907,7 +919,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      name.s = get_value(&cfg, section.s, "ramdisk");
>>      if ( name.s )
>>      {
>> -        split_value(name.s);
>> +        place_string(&mb_modules[mbi.mods_count].string, name.s);
>> +        truncate_string(name.s);
>>          load_ok = load_file(dir_handle, s2w(&name), &ramdisk);
>>          efi_bs->FreePool(name.w);
>>          if ( !load_ok )
>> @@ -920,7 +933,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      if ( name.s )
>>      {
>>          microcode_set_module(mbi.mods_count);
>> -        split_value(name.s);
>> +        place_string(&mb_modules[mbi.mods_count].string, name.s);
>> +        truncate_string(name.s);
>>          load_ok = load_file(dir_handle, s2w(&name), &ucode);
>>          efi_bs->FreePool(name.w);
>>          if ( !load_ok )
>> @@ -930,7 +944,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      name.s = get_value(&cfg, section.s, "xsm");
>>      if ( name.s )
>>      {
>> -        split_value(name.s);
>> +        place_string(&mb_modules[mbi.mods_count].string, name.s);
>> +        truncate_string(name.s);
>>          load_ok = load_file(dir_handle, s2w(&name), &xsm);
>>          efi_bs->FreePool(name.w);
>>          if ( !load_ok )
>
> Looking at all these I wonder why you didn't retain split_value() as a
> simple wrapper.
I should be able to do that.
>
> Furthermore splitting out the place_string() doesn't seem very
> efficient, as imo the goal ought to be for efi_start() to become
> common code (or at least the module loading part of fit), i.e.
> there's no win at all from the change you're doing here.

I don't think that combining the x86 and arm efi_start() will work out
that cleanly.  Arm is using device tree from getting information from
GRUB and/or the firmware, so I think you'd end up with a lot of conditional
code.

>
> Jan
>