From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v6 COLO 06/15] libxc/save: support COLO save Date: Tue, 9 Jun 2015 09:51:03 +0100 Message-ID: <5576A8F7.70302@citrix.com> References: <1433735159-26739-1-git-send-email-yanghy@cn.fujitsu.com> <1433735159-26739-7-git-send-email-yanghy@cn.fujitsu.com> <557592C0.6020709@citrix.com> <55765A49.9010101@cn.fujitsu.com> <557693CA.9090700@citrix.com> <5576A7A5.9020703@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5576A7A5.9020703@cn.fujitsu.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Yang Hongyang , xen-devel@lists.xen.org Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, wency@cn.fujitsu.com, guijianfeng@cn.fujitsu.com, yunhong.jiang@intel.com, eddie.dong@intel.com, rshriram@cs.ubc.ca, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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