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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

~Andrew