From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roy Franz Subject: Re: [PATCH V2 05/12] replace split_value() with truncate_string() Date: Wed, 6 Aug 2014 15:37:37 -0700 Message-ID: References: <1405989815-25236-1-git-send-email-roy.franz@linaro.org> <1405989815-25236-6-git-send-email-roy.franz@linaro.org> <53D0CFAD02000078000255F2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53D0CFAD02000078000255F2@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir , Ian Campbell , tim , xen-devel , Stefano Stabellini , linaro-uefi , Fu Wei List-Id: xen-devel@lists.xenproject.org On Thu, Jul 24, 2014 at 12:19 AM, Jan Beulich wrote: >>>> On 22.07.14 at 02:43, 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 >