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

[Hongyang Yang <yanghy@xxxxxxxxxxxxxx>]

On 05/08/2015 06:08 PM, Andrew Cooper wrote:
> On 08/05/15 10:59, Hongyang Yang wrote:
> > > 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...
> Ah, in which case you would be better using
> xc__hypercall_buffer_free_pages() and not creating the local shadow in
> the first place.
I thought we'd better use those MACROs which described in the comments...
If it is OK to use xc__hypercall_buffer_free_pages(), I will fix it in
the next version.

> ~Andrew
> .
--
Thanks,
Yang.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel