From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH V2 04/12] Refactor read_file() so it can be shared. Date: Thu, 24 Jul 2014 08:09:19 +0100 Message-ID: <53D0CD3F02000078000255D5@mail.emea.novell.com> References: <1405989815-25236-1-git-send-email-roy.franz@linaro.org> <1405989815-25236-5-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: <1405989815-25236-5-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, linaro-uefi@lists.linaro.org, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org >>> On 22.07.14 at 02:43, wrote: > The read_file() function updated some multiboot specific data structures > as it was loading a file. These changes make read_file() more generic, > and create a load_file() wrapper for x86 that updates the multiboot > data structures. read_file() no longer does special handling of > the configuration file, as this was only needed to avoid adding > it to the multiboot structures. read_file() and load_file() return > error codes rather than directly exiting on error to facilicate > sharing. Different architectures may require different max allocation > addresses so take that as an argument. Unless you expect an architecture to pass in different values on different invocations this clearly can be a #define rather than a function parameter. > @@ -405,12 +399,21 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > > if ( what ) > { > - PrintErr(what); > - PrintErr(L" failed for "); > - PrintErrMesgExit(name, ret); > + PrintErrMesg(what, ret); > + PrintErr(L"Unable to load file"); Is it intentional to make the message less useful by dripping the printing of the file name? > + return 0; > + } > + else No need for an "else" after an unconditional "return" in the "if" branch. > @@ -470,6 +473,21 @@ static char *__init get_value(const struct file *cfg, const char *section, > } > return NULL; > } > +/* Only call with non-config files. */ Missing blank line before this comment. > +bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > + struct file *file) > +{ > + EFI_PHYSICAL_ADDRESS max = min(1UL << (32 + PAGE_SHIFT), > + HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START); > + if ( read_file(dir_handle, name, file, max) ) Missing blank line between declaration and first statement. > @@ -896,16 +921,20 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > { > microcode_set_module(mbi.mods_count); > split_value(name.s); > - read_file(dir_handle, s2w(&name), &ucode); > + load_ok = load_file(dir_handle, s2w(&name), &ucode); > efi_bs->FreePool(name.w); > + if ( !load_ok ) > + blexit(L"Unable to load ucode image."); > } > > name.s = get_value(&cfg, section.s, "xsm"); > if ( name.s ) > { > split_value(name.s); > - read_file(dir_handle, s2w(&name), &xsm); > + load_ok = load_file(dir_handle, s2w(&name), &xsm); > efi_bs->FreePool(name.w); > + if ( !load_ok ) > + blexit(L"Unable to load ucode image."); Apart from the same comment made on an earlier patch - no need for an extra message here when the called function already printed one - this is a copy'n'paste mistake: You mean XSM instead of ucode here. Jan