Re: [PATCH v4 --for 4.6 COLOPre 07/25] libxl/remus: init checkpoint_callback in Remus checkpoint callback

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

On 07/16/2015 06:32 PM, Ian Campbell wrote:
> On Wed, 2015-07-15 at 20:35 +0800, Yang Hongyang wrote:
>>
>> On 07/15/2015 08:02 PM, Ian Campbell wrote:
>>> On Wed, 2015-07-15 at 15:45 +0800, Yang Hongyang wrote:
>>>> init stream {read/write} state checkpoint_callback in Remus
>>>> checkpoint callback.
>>>
>>> Why? Is this earlier or later than previously? Seems later?
>>
>> There's no functional change, it's just refactoring so that we can move
>> all remus code into one file.
>
> That answers the why, thanks.
>
> But, it would be a no functional change if the initialisation was moving
> from e.g. the very start of a function to the caller of that function
> right before the call or from the end of a function to the caller right
> after the call.
>
> But AFAICT this movement is a bit more than that, e.g. the init of
> dcs->srs.checkpoint_callback has moved from near the end of
> domcreate_bootloader_done to
> libxl__remus_domain_restore_checkpoint_callback before a call to
> libxl__stream_read_start_checkpoint, which doesn't have the property I
> describe above, at least not in an obvious way.
>
> So there is either a span of time where the callback is no longer
> initialised when it was before, or it is initialised for a larger span
> that it was before (with the former having the larger potential for
> issues).
>
> It's also not entirely clear that the new location of the initialisation
> is traversed on all the same paths as before, or if it happens on fewer
> paths that those are exactly the ones which matter.
>
> Lastly it seems odd to split out the initialisation of only one member
> of dcs->srs and dcs->sws into a different location to all the others,
> especially putting it into the checkpoint callback (which is called
> repeatedly).
>
> Perhaps what is really needed here is a new function to initialise a
> dcs->srs for remus, and another for dcs->sws to be called exactly where
> the init happens today?

The checkpoint_callback() only used by remus, you can see from the
initialisation line:
dss->sws.checkpoint_callback = remus_checkpoint_stream_written;
dcs->srs.checkpoint_callback = remus_checkpoint_stream_done;

These two functions remus_checkpoint_stream_written &
remus_checkpoint_stream_done only called when libxc call
chaeckpoint() callback, so there should be no problem
with the move. Given the fact it's only used by Remus, init
it in previous place is not a good idea IMO.

>
>
> .
>

-- 
Thanks,
Yang.