From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH] libxc: refactor memory allocation functions Date: Tue, 1 Dec 2015 15:08:43 +0100 Message-ID: <565DA9EB.20601@suse.com> References: <1448969956-26310-1-git-send-email-wei.liu2@citrix.com> <1448971124.15768.117.camel@citrix.com> <565D8CBE.9020001@suse.com> <20151201121236.GQ21588@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a3lbq-0006BO-Lj for xen-devel@lists.xenproject.org; Tue, 01 Dec 2015 14:08:46 +0000 In-Reply-To: <20151201121236.GQ21588@citrix.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: Wei Liu Cc: Xen-devel , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org On 01/12/15 13:12, Wei Liu wrote: > On Tue, Dec 01, 2015 at 01:04:14PM +0100, Juergen Gross wrote: >> On 01/12/15 12:58, Ian Campbell wrote: >>> On Tue, 2015-12-01 at 11:39 +0000, Wei Liu wrote: >>>> There were some problems with the original memory allocation functions: >>>> 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling >>>> xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything. >>>> 2. xc_dom_alloc_pad didn't call dom->allocate. >>>> >>>> Refactor the code so that: >>>> 1. xc_dom_alloc_{segment,pad,page} end up calling >>>> xc_dom_chk_alloc_pages. >>>> 2. xc_dom_chk_alloc_pages calls dom->allocate. >>>> >>>> This way we avoid scattering dom->allocate over multiple locations and >>>> open-coding. >>>> >>>> Also change the return type of xc_dom_alloc_page to xen_pfn_t and return >>>> an invalid pfn when xc_dom_chk_alloc_pages fails. >>> >>> Given this presumably the handful of callers ought to gain some error >>> handling in a followup patch? >> >> I could do that. Wei? >> > > You're welcome to pick that up. > >>> >>> xc_dom_chk_alloc_pages does log, so at least the callers needn't bother >>> with that. >>> >>>> Signed-off-by: Wei Liu >>> >>> Acked-by: Ian Campbell >>> >>>> --- >>>> Cc: Ian Campbell >>>> Cc: Ian Jackson >>>> Cc: Juergen Gross >>>> >>>> We don't have INVALID_PFN, maybe we need one? >>> >>> We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the >>> bill (or if not how it is distinct from the former). >> >> What about merging all of them into INVALID_FRAME? >> > > Note that INVALID_MFN is in public header (xenctrl.h) while > INVALID_P2M_ENTRY is not. INVALID_P2M_ENTRY in libxc is defined as (xen_pfn_t)-1. I think it would make sense to modify that to INVALID_PFN. > In fact (xen_pfn_t)-1 is part of guest ABI so we should properly export > and document it. The current situation is not ideal. Not sure how much > yak shaving is required though. As this relates to the return value checking of the domain builder memory allocating I'll have a try, too. Juergen