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

...

...

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