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

[Hongyang Yang <yanghy@xxxxxxxxxxxxxx>]

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