[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.