Ian Campbell wrote:
> 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.
>
Heh, I was thinking about this on my way to the office today ...
> 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...):
>
Yes, it looks sane to me and has now been tested. I did several
save/restore of both pv and hvm domains through libvirt libxl driver.
Tested-by: Jim Fehlig <jfehlig@xxxxxxxxxx>
Regards,
Jim
> # 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