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: Fri, 8 May 2015 17:59:19 +0800 Message-ID: <554C88F7.2030908@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <554C85A7.5000103@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, ian.jackson@eu.citrix.com, yunhong.jiang@intel.com, eddie.dong@intel.com, rshriram@cs.ubc.ca List-Id: xen-devel@lists.xenproject.org On 05/08/2015 05:45 PM, Andrew Cooper wrote: > On 08/05/15 10:33, Yang Hongyang wrote: >> introduce setup() and cleanup() which subsume the >> ctx->save.ops.{setup,cleanup}() calls and also do allocate/free >> necessary memory. >> The SHADOW_OP_OFF hypercall also included in the cleanup(). >> >> Signed-off-by: Yang Hongyang > > I would suggest swapping this with patch 1, to save introducing new > code, just to move it again in the next patch. OK > > 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... > >> + free(ctx->save.deferred_pages); >> + free(ctx->save.batch_pfns); >> +} >> + >> +/* >> + * Save a domain. >> + */ >> +static int save(struct xc_sr_context *ctx, uint16_t guest_type) >> +{ >> + xc_interface *xch = ctx->xch; >> + int rc; >> + >> + rc = setup(ctx); >> + if ( rc ) >> + goto err; >> + >> + IPRINTF("Saving domain %d, type %s", >> + ctx->domid, dhdr_type_to_str(guest_type)); >> + >> xc_report_progress_single(xch, "Start of stream"); >> >> rc = write_headers(ctx, guest_type); >> @@ -679,29 +714,10 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type) >> goto done; >> >> err: >> - saved_errno = errno; >> - saved_rc = rc; >> PERROR("Save failed"); >> >> done: >> - xc_shadow_control(xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_OFF, >> - NULL, 0, NULL, 0, NULL); >> - >> - rc = ctx->save.ops.cleanup(ctx); >> - if ( rc ) >> - PERROR("Failed to clean up"); >> - >> - xc_hypercall_buffer_free_pages(xch, dirty_bitmap, >> - NRPAGES(bitmap_size(ctx->save.p2m_size))); >> - free(ctx->save.deferred_pages); >> - free(ctx->save.batch_pfns); >> - >> - if ( saved_rc ) >> - { >> - rc = saved_rc; >> - errno = saved_errno; >> - } > > You must keep saved_{rc,errno}, so that the cleanup doesn't clobber the > errno semantically relevant to why the save failed. > OK > ~Andrew > >> - >> + cleanup(ctx); >> return rc; >> }; >> > > . > -- Thanks, Yang.