Re: [PATCH] libxc: reset completed flag in restore_ctx

[Jim Fehlig <jfehlig@novell.com>]

Ian Campbell wrote:
> On Tue, 2011-05-24 at 00:05 +0100, Jim Fehlig wrote:
>   
>> # HG changeset patch
>> # User Jim Fehlig <jfehlig@novell.com>
>> # 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@novell.com>

Regards,
Jim

> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # 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@citrix.com>
>
> 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;
>
>
>