From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hongyang Yang Subject: Re: [PATCH Remus v2 02/10] tools/libxc: introduce setup() and cleanup() on save Date: Mon, 11 May 2015 09:20:30 +0800 Message-ID: <555003DE.9090104@cn.fujitsu.com> References: <1431077610-3366-1-git-send-email-yanghy@cn.fujitsu.com> <1431077610-3366-3-git-send-email-yanghy@cn.fujitsu.com> <554C85A7.5000103@citrix.com> <554C88F7.2030908@cn.fujitsu.com> <554C8B08.6020409@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <554C8B08.6020409@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: Andrew Cooper , xen-devel@lists.xen.org Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, wency@cn.fujitsu.com, eddie.dong@intel.com, yunhong.jiang@intel.com, ian.jackson@eu.citrix.com, rshriram@cs.ubc.ca List-Id: xen-devel@lists.xenproject.org On 05/08/2015 06:08 PM, Andrew Cooper wrote: > On 08/05/15 10:59, Hongyang Yang wrote: >> >>> >>> In general, a good change, but some comments... >>> >>>> --- >>>> tools/libxc/xc_sr_save.c | 72 >>>> +++++++++++++++++++++++++++++------------------- >>>> 1 file changed, 44 insertions(+), 28 deletions(-) >>>> >>>> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c >>>> index cc3e6b1..2394bc4 100644 >>>> --- a/tools/libxc/xc_sr_save.c >>>> +++ b/tools/libxc/xc_sr_save.c >>>> @@ -607,13 +607,10 @@ static int send_domain_memory_nonlive(struct >>>> xc_sr_context *ctx) >>>> return rc; >>>> } >>>> >>>> -/* >>>> - * Save a domain. >>>> - */ >>>> -static int save(struct xc_sr_context *ctx, uint16_t guest_type) >>>> +static int setup(struct xc_sr_context *ctx) >>>> { >>>> xc_interface *xch = ctx->xch; >>>> - int rc, saved_rc = 0, saved_errno = 0; >>>> + int rc; >>>> DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, >>>> (&ctx->save.dirty_bitmap_hbuf)); >>>> >>>> @@ -632,13 +629,51 @@ static int save(struct xc_sr_context *ctx, >>>> uint16_t guest_type) >>>> goto err; >>>> } >>>> >>>> - IPRINTF("Saving domain %d, type %s", >>>> - ctx->domid, dhdr_type_to_str(guest_type)); >>>> - >>>> rc = ctx->save.ops.setup(ctx); >>>> if ( rc ) >>>> goto err; >>>> >>>> + rc = 0; >>>> + >>>> + err: >>>> + return rc; >>>> + >>>> +} >>>> + >>>> +static void cleanup(struct xc_sr_context *ctx) >>>> +{ >>>> + xc_interface *xch = ctx->xch; >>>> + DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, >>>> + (&ctx->save.dirty_bitmap_hbuf)); >>>> + >>>> + xc_shadow_control(xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_OFF, >>>> + NULL, 0, NULL, 0, NULL); >>>> + >>>> + if ( ctx->save.ops.cleanup(ctx) ) >>>> + PERROR("Failed to clean up"); >>>> + >>>> + if ( dirty_bitmap ) >>>> + xc_hypercall_buffer_free_pages(xch, dirty_bitmap, >>>> + >>>> NRPAGES(bitmap_size(ctx->save.p2m_size))); >>> >>> xc_hypercall_buffer_free_pages() if fine dealing with NULL, just like >>> free() is. You can drop the conditional. >> >> Actually this is another trick that I need to deal with those >> hypercall macros. >> DECLARE_HYPERCALL_BUFFER_SHADOW will define a user pointer "dirty_bitmap" >> and a shadow buffer, although xc_hypercall_buffer_free_pages takes >> "dirty_bitmap" as an augument, but it is also a MACRO, without >> "if ( dirty_bitmap )", the compiler will report "dirty_bitmap" unused >> error... > > Ah, in which case you would be better using > xc__hypercall_buffer_free_pages() and not creating the local shadow in > the first place. I thought we'd better use those MACROs which described in the comments... If it is OK to use xc__hypercall_buffer_free_pages(), I will fix it in the next version. > > ~Andrew > . > -- Thanks, Yang.