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

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

On 09/06/15 09:45, Yang Hongyang wrote:
>
>>> Even if there are no dirty pages on secondary, pfn_list shouldn't be
>>> NULL, it's just that pfn_list[0] will be 0. if pfn_list is NULL,
>>> there might be unexpected error happened.
>>
>> get_dirty_pfn() should be declared alongside a
>>
>> struct pfn_data
>> {
>>      uint64_t count;
>>      uint64_t *pfns;
>> };
>>
>> and this function here should create one of these on the stack and pass
>> it by pointer to get_dirty_pfn().  I might also be tempted to rename
>> this to get_remote_logdirty() or similar, to indicate that it is a
>> source of logdirty data from something other than the current
>> hypervisor.
>
> This is a callback, I can't find a way to pass pointer from libxc to
> libxl,
> libxl can not access the pointer data...The struct can be used for
> represent
> the data however.

Right - my point is that it should be the implementation of
get_remote_logdirty() (i.e. in libxl_save_helper) which is responsible
for unpackaging the data from whatever RPC method is used, rather than
the caller.

>
>>>> Shouldn't get_dirty_pfn be mandatory for COLO streams (even if it is a
>>>> noop to start with) ?
>>>
>>> It should be mandatory, it shouldn't be noop under COLO. perhaps we
>>> should
>>> add sanity check at the beginning. But problem is save side do not
>>> have a param
>>> passed from libxl to indicate the stream type(like
>>> checkpointed_stream in
>>> restore side). So we may need to add another XCFLAGS? Currently
>>> there is
>>> XCFLAGS_CHECKPOINTED which represents Remus, we might need to change
>>> this to
>>> XCFLAGS_STREAM_REMUS
>>> XCFLAGS_STREAM_COLO
>>> so that we can know what kind of stream we are handling?
>>
>> checkpointed_stream started out as a bugfix for a legacy stream
>> migration breakage.  Really, this information should have been passed
>> right from the start.
>
> Did I miss the bugfix? is it not in upstream?

c/s 7051d5c

~Andrew