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

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-05-11 at 09:20 +0800, Hongyang Yang wrote:
> 
> 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.

If anything I think I'd prefer for the if to move inside the
xc_hypercall_buffer_free_pages macro.

Ian.