Re: [PATCH v3 COLOPre 17/26] tools/libx{l, c}: introduce should_checkpoint callback

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 06/30/2015 06:19 PM, Ian Campbell wrote:
> On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
>> Under COLO, we are doing checkpoint on demand, if this
>> callback returns 1, we will take another checkpoint.
>> 0 indicates unexpected error.
>
> Is this checkpoint therefore expected to be blocking until another
> checkpoint is desired (or an error occurs)?

Right.

>
>>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>   tools/libxc/include/xenguest.h     | 18 ++++++++++++++++++
>>   tools/libxl/libxl_save_msgs_gen.pl |  7 ++++---
>>   2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
>> index 3e48a97..e804a1d 100644
>> --- a/tools/libxc/include/xenguest.h
>> +++ b/tools/libxc/include/xenguest.h
>> @@ -63,6 +63,15 @@ struct save_callbacks {
>>        * 1: take another checkpoint */
>>       int (*checkpoint)(void* data);
>>
>> +    /*
>> +     * Called after the checkpoint callback.
>> +     *
>> +     * returns:
>> +     * 0: terminate checkpointing gracefully
>
> The commit message describes this as an "unexpected error" which this
> seems more like the desired response to such an error.
>
> And what would gracefully involve in this context, resuming on sender or
> receiver?

It depends on the error. If it indicates a failover on restore side, we will
resuming the receiver. I will make the comment clearer next version.

>
>> +     * 1: take another checkpoint
>> +     */
>> +    int (*should_checkpoint)(void* data);
>> +
>>       /* Enable qemu-dm logging dirty pages to xen */
>>       int (*switch_qemu_logdirty)(int domid, unsigned enable, void *data); /* HVM only */
>>
>> @@ -108,6 +117,15 @@ struct restore_callbacks {
>>       /* A checkpoint record has been found in the stream */
>>       int (*checkpoint)(void* data);
>>
>> +    /*
>> +     * Called after the checkpoint callback.
>> +     *
>> +     * returns:
>> +     * 0: terminate checkpointing gracefully
>
> Again.
>
>> +     * 1: take another checkpoint
>> +     */
>> +    int (*should_checkpoint)(void* data);
>> +
>>       /* to be provided as the last argument to each callback function */
>>       void* data;
>>   };
>> diff --git a/tools/libxl/libxl_save_msgs_gen.pl b/tools/libxl/libxl_save_msgs_gen.pl
>> index dc17c6b..7284975 100755
>> --- a/tools/libxl/libxl_save_msgs_gen.pl
>> +++ b/tools/libxl/libxl_save_msgs_gen.pl
>> @@ -26,11 +26,12 @@ our @msgs = (
>>       [  3, 'scxA',   "suspend", [] ],
>>       [  4, 'scxA',   "postcopy", [] ],
>>       [  5, 'srcxA',   "checkpoint", [] ],
>> -    [  6, 'scxA',   "switch_qemu_logdirty",  [qw(int domid
>> +    [  6, 'srcxA',  "should_checkpoint", [] ],
>> +    [  7, 'scxA',   "switch_qemu_logdirty",  [qw(int domid
>>                                                 unsigned enable)] ],
>> -    [  7, 'r',      "restore_results",       ['unsigned long', 'store_mfn',
>> +    [  8, 'r',      "restore_results",       ['unsigned long', 'store_mfn',
>>                                                 'unsigned long', 'console_mfn'] ],
>> -    [  8, 'srW',    "complete",              [qw(int retval
>> +    [  9, 'srW',    "complete",              [qw(int retval
>>                                                    int errnoval)] ],
>>   );
>>
>
>
> .
>

-- 
Thanks,
Yang.