Re: [PATCH] libxc: refactor memory allocation functions

[Juergen Gross <jgross@suse.com>]

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 <wei.liu2@citrix.com>
>>>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>>> ---
>>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> Cc: Juergen Gross <jgross@suse.com>
>>>>
>>>> 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