From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH V5 10/15] Add arch specific module handling to read_file() Date: Mon, 22 Sep 2014 13:44:02 +0100 Message-ID: <542035B20200007800036ED3@mail.emea.novell.com> References: <1411080607-32365-1-git-send-email-roy.franz@linaro.org> <1411080607-32365-11-git-send-email-roy.franz@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1411080607-32365-11-git-send-email-roy.franz@linaro.org> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Roy Franz Cc: keir@xen.org, ian.campbell@citrix.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org >>> On 19.09.14 at 00:50, wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -56,12 +56,13 @@ static void noreturn blexit(const CHAR16 *str); > static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode); > static char *get_value(const struct file *cfg, const char *section, > const char *item); > -static void split_value(char *s); > +static char *split_string(char *s); > static CHAR16 *s2w(union string *str); > static char *w2s(const union string *str); > -static bool_t read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > - struct file *file); > +static size_t wstrlen(const CHAR16 * s); > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file, > + CHAR16 *filename, char *options); Please don't needlessly move this declaration down. > @@ -115,6 +116,15 @@ static void __init DisplayUint(UINT64 Val, INTN Width) > PrintStr(PrintString); > } > > +static size_t __init wstrlen(const CHAR16 * s) > +{ > + const CHAR16 *sc; > + > + for (sc = s; *sc != L'\0'; ++sc) > + /* nothing */; > + return sc - s; > +} Coding style (many issues). > @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn) > break; > } > } > +/* > + * Truncate string at first space, and return pointer > + * to remainder of string. ... (if any). > + */ > +static char * __init split_string(char *s) > +{ > + while ( *s && !isspace(*s) ) > + ++s; > + if ( *s ) > + { > + *s = 0; > + return(s + 1); No parentheses here please. > + } > + return NULL; > +} > > -static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > - struct file *file) > +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file, > + CHAR16 *filename, char *options) Is the renaming from "name" to "filename" really necessary/useful? And the flipping of the order of pre-existing parameters? > @@ -659,18 +678,19 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > EFI_LOADED_IMAGE *loaded_image; > EFI_STATUS status; > unsigned int i, argc; > - CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; > + CHAR16 **argv, *options = NULL; > UINTN cols, rows, depth, size, info_size, gop_mode = ~0; > EFI_HANDLE *handles = NULL; > EFI_SHIM_LOCK_PROTOCOL *shim_lock; > EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; > EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info; > EFI_FILE_HANDLE dir_handle; > - union string section = { NULL }, name; > + union string section = { NULL }, name, file_name, cfg_file_name = {NULL}; Your addition should be consistent with existing code (blanks around NULL). But - why are you changing the types of the two variables in the first place? Afaics all references to them now are using the .w member access, suggesting that this is just a remnant of your earlier version changes. > @@ -274,8 +275,8 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect > if ( name.s ) > { > microcode_set_module(mbi.mods_count); > - split_value(name.s); > - read_file(dir_handle, s2w(&name), &ucode); > + options = split_string(name.s); > + read_file(dir_handle, &ucode, s2w(&name), options); Perhaps rather NULL instead of options? > @@ -575,3 +576,30 @@ static void __init efi_arch_memory(void) > l3_bootmap[l3_table_offset(xen_phys_start + (8 << L2_PAGETABLE_SHIFT) - 1)] = > l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR); > } > + > +static void __init efi_arch_handle_module(struct file *file, const CHAR16 *name, > + char *options) > +{ > + union string local_name; > + void *ptr; > + > + /* > + * Make a copy, as conversion is destructive, and caller still wants > + * wide string available after this call returns. > + */ > + if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * sizeof(*name), > + &ptr) != EFI_SUCCESS ) > + blexit(L"ERROR Unable to allocate string buffer\r\n"); Iirc I said this before, but just in case: No explicit newline on strings passed to blexit() please. Jan