Re: [PATCH v6 COLO 06/15] libxc/save: support COLO save

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

On 08/06/15 04:45, Yang Hongyang wrote:
> call callbacks->get_dirty_pfn() after suspend primary vm to
> get dirty pages on secondary vm, and send pages both dirty on
> primary/secondary to secondary.
>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/xc_sr_save.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
> index d63b783..cda61ed 100644
> --- a/tools/libxc/xc_sr_save.c
> +++ b/tools/libxc/xc_sr_save.c
> @@ -515,6 +515,31 @@ static int send_memory_live(struct xc_sr_context *ctx)
>      return rc;
>  }
>  
> +static int update_dirty_bitmap(uint8_t *(*get_dirty_pfn)(void *), void *data,
> +                               unsigned long p2m_size, unsigned long *bitmap)

This function should take a ctx rather than having the caller expand 3
parameters.  Also, "update_dirty_bitmap" is a little misleading, as it
isn't querying the hypervisor for the dirty bitmap.

> +{
> +    uint64_t *pfn_list;
> +    uint64_t count, i;
> +    uint64_t pfn;
> +
> +    pfn_list = (uint64_t *)get_dirty_pfn(data);

This looks like a recipe for width-errors.  The get_dirty_pfn() call
should take a pointer to a struct for it to fill.

> +    assert(pfn_list);

This should turn into an error rather than an abort().

> +
> +    count = pfn_list[0];
> +    for (i = 0; i < count; i++) {

style

> +        pfn = pfn_list[i + 1];
> +        if (pfn > p2m_size) {
> +            errno = EINVAL;
> +            return -1;
> +        }
> +
> +        set_bit(pfn, bitmap);
> +    }
> +
> +    free(pfn_list);
> +    return 0;
> +}
> +
>  /*
>   * Suspend the domain and send dirty memory.
>   * This is the last iteration of the live migration and the
> @@ -555,6 +580,19 @@ static int suspend_and_send_dirty(struct xc_sr_context *ctx)
>  
>      bitmap_or(dirty_bitmap, ctx->save.deferred_pages, ctx->save.p2m_size);
>  
> +    if ( !ctx->save.live && ctx->save.callbacks->get_dirty_pfn )
> +    {

Shouldn't get_dirty_pfn be mandatory for COLO streams (even if it is a
noop to start with) ?

~Andrew

> +        rc = update_dirty_bitmap(ctx->save.callbacks->get_dirty_pfn,
> +                                 ctx->save.callbacks->data,
> +                                 ctx->save.p2m_size,
> +                                 dirty_bitmap);
> +        if ( rc )
> +        {
> +            PERROR("Failed to get secondary vm's dirty pages");
> +            goto out;
> +        }
> +    }
> +
>      rc = send_dirty_pages(ctx, stats.dirty_count + ctx->save.nr_deferred_pages);
>      if ( rc )
>          goto out;
> @@ -784,7 +822,16 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
>              if ( rc )
>                  goto err;
>  
> -            ctx->save.callbacks->postcopy(ctx->save.callbacks->data);
> +            rc = ctx->save.callbacks->postcopy(ctx->save.callbacks->data);
> +            if ( !rc ) {
> +                if ( !errno )
> +                {
> +                    /* Postcopy request failed (without errno, using EINVAL) */
> +                    errno = EINVAL;
> +                }
> +                rc = -1;
> +                goto err;
> +            }
>  
>              rc = ctx->save.callbacks->checkpoint(ctx->save.callbacks->data);
>              if ( rc <= 0 )