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

["Jan Beulich" <JBeulich@suse.com>]

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

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

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.

Jan