[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH Remus v2 02/10] tools/libxc: introduce setup() and cleanup() on save





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 <yanghy@xxxxxxxxxxxxxx>

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.