On Tue, 2011-05-24 at 00:05 +0100, Jim Fehlig wrote:
> # HG changeset patch
> # User Jim Fehlig <jfehlig@xxxxxxxxxx>
> # Date 1306191873 21600
> # Node ID f94242f20cdaee81d28f68df38d5a98f8fd9947d
> # Parent fb517cc27adef3a7ad548e7397e02e1414132ead
> libxc: reset completed flag in restore_ctx
>
> Long running libxl/libxc apps such as libvirt segfault when
> attempting multiple restores. The completed flag in restore_ctx
> structure is set at completion of first restore. Subsequent
> restores do not load any pages and result in the segfault.
>
> Reset completed flag at start of restore.
Seems reasonable. However, we have:
static struct restore_ctx _ctx = {
.live_p2m = NULL,
.p2m = NULL,
};
static struct restore_ctx *ctx = &_ctx;
[...]
/* For info only */
ctx->nr_pfns = 0;
i.e. we initialise the different subsets of the fields in two separate
places. Perhaps we should just add a memset?
BTW, those static variables are pretty disgusting and are going to cause
trouble for users which deal with >1 domain at a time.
It's not clear that there is any reason for it to be a static variable
anyway. It comes from 20545:cc7d66ba0dad which says: "pass the
restore_context through function and allocate the context on the restore
function stack." but, presumably by mistake, retains the static
modifiers from the global variable. The same is true of the save side.
So how about this instead (untested but seemingly sane...):
# HG changeset patch
# User Ian Campbell <ian.campbell@xxxxxxxxxx>
# Date 1306224654 -3600
# Node ID 13599c2c82d8bdbfc73611fed9a1bc2aebfa275c
# Parent 9cf8d38260606de37826b76334028114feff6131
libxc: xc_domain_{save,restore}: remove static variables
20544:ad9d75d74bd5 and 20545:cc7d66ba0dad seemingly intended to change these
global static variables into stack variables but didn't remove the static
qualifier.
Also zero the entire struct once with memset rather than clearing fields
piecemeal in two different places.
Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
diff -r 9cf8d3826060 -r 13599c2c82d8 tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c Tue May 24 09:02:05 2011 +0100
+++ b/tools/libxc/xc_domain_restore.c Tue May 24 09:10:54 2011 +0100
@@ -1133,23 +1133,19 @@ int xc_domain_restore(xc_interface *xch,
int orig_io_fd_flags;
- static struct restore_ctx _ctx = {
- .live_p2m = NULL,
- .p2m = NULL,
- };
- static struct restore_ctx *ctx = &_ctx;
+ struct restore_ctx _ctx;
+ struct restore_ctx *ctx = &_ctx;
struct domain_info_context *dinfo = &ctx->dinfo;
pagebuf_init(&pagebuf);
memset(&tailbuf, 0, sizeof(tailbuf));
tailbuf.ishvm = hvm;
- /* For info only */
- ctx->nr_pfns = 0;
-
if ( superpages )
return 1;
+ memset(ctx, 0, sizeof(*ctx));
+
ctxt = xc_hypercall_buffer_alloc(xch, ctxt, sizeof(*ctxt));
if ( ctxt == NULL )
diff -r 9cf8d3826060 -r 13599c2c82d8 tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c Tue May 24 09:02:05 2011 +0100
+++ b/tools/libxc/xc_domain_save.c Tue May 24 09:10:54 2011 +0100
@@ -958,11 +958,8 @@ int xc_domain_save(xc_interface *xch, in
unsigned long mfn;
struct outbuf ob;
- static struct save_ctx _ctx = {
- .live_p2m = NULL,
- .live_m2p = NULL,
- };
- static struct save_ctx *ctx = &_ctx;
+ struct save_ctx _ctx;
+ struct save_ctx *ctx = &_ctx;
struct domain_info_context *dinfo = &ctx->dinfo;
int completed = 0;
@@ -976,6 +973,8 @@ int xc_domain_save(xc_interface *xch, in
outbuf_init(xch, &ob, OUTBUF_SIZE);
+ memset(ctx, 0, sizeof(*ctx));
+
/* If no explicit control parameters given, use defaults */
max_iters = max_iters ? : DEF_MAX_ITERS;
max_factor = max_factor ? : DEF_MAX_FACTOR;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home