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 <yan...@cn.fujitsu.com>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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
