From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Hongyang Subject: Re: [PATCH v8 --for 4.6 COLO 06/25] tools/libxl: write colo_context records into the stream Date: Thu, 16 Jul 2015 15:24:38 +0800 Message-ID: <55A75C36.8010108@cn.fujitsu.com> References: <1436951936-3447-1-git-send-email-yanghy@cn.fujitsu.com> <1436951936-3447-7-git-send-email-yanghy@cn.fujitsu.com> <55A699CE.6020808@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A699CE.6020808@citrix.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: Andrew Cooper , 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 07/16/2015 01:35 AM, Andrew Cooper wrote: > On 15/07/15 10:18, Yang Hongyang wrote: >> write colo_context records into the stream, used by both >> primary and secondary to send colo context. >> >> Signed-off-by: Yang Hongyang >> Signed-off-by: Wen Congyang >> --- >> tools/libxl/libxl_internal.h | 5 +++ >> tools/libxl/libxl_stream_write.c | 87 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 92 insertions(+) >> >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index a83d6a5..2634836 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -3000,6 +3000,7 @@ struct libxl__stream_write_state { >> int rc; >> bool running; >> bool in_checkpoint; >> + bool in_colo_context; >> libxl__save_helper_state shs; >> >> /* Main stream-writing data. */ >> @@ -3019,6 +3020,10 @@ _hidden void libxl__stream_write_start(libxl__egc *egc, >> _hidden void >> libxl__stream_write_start_checkpoint(libxl__egc *egc, >> libxl__stream_write_state *stream); >> +_hidden void >> +libxl__stream_write_colo_context(libxl__egc *egc, >> + libxl__stream_write_state *stream, >> + libxl_sr_colo_context *colo_context); >> _hidden void libxl__stream_write_abort(libxl__egc *egc, >> libxl__stream_write_state *stream, >> int rc); >> diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c >> index df55277..e7a32c4 100644 >> --- a/tools/libxl/libxl_stream_write.c >> +++ b/tools/libxl/libxl_stream_write.c >> @@ -96,6 +96,16 @@ static void write_checkpoint_end_record(libxl__egc *egc, >> static void checkpoint_end_record_done(libxl__egc *egc, >> libxl__stream_write_state *stream); >> >> +/* COLO context */ >> +static void write_colo_context(libxl__egc *egc, >> + libxl__stream_write_state *stream, >> + libxl_sr_colo_context *colo_context); >> +static void write_colo_context_done(libxl__egc *egc, >> + libxl__datacopier_state *dc, >> + int rc, int onwrite, int errnoval); >> +static void colo_context_done(libxl__egc *egc, >> + libxl__stream_write_state *stream, int rc); >> + >> /*----- Helpers -----*/ >> >> static void write_done(libxl__egc *egc, >> @@ -500,6 +510,11 @@ static void stream_complete(libxl__egc *egc, >> return; >> } >> >> + if (stream->in_colo_context) { >> + colo_context_done(egc, stream, rc); >> + return; >> + } > > Please follow the same style as stream->in_checkpoint, asserting(rc) and > explaining how the error comes back around. > >> + >> if (!stream->rc) >> stream->rc = rc; >> stream_done(egc, stream); >> @@ -555,6 +570,78 @@ static void check_all_finished(libxl__egc *egc, >> stream->completion_callback(egc, stream, stream->rc); >> } >> >> +/*----- COLO context -----*/ >> +void libxl__stream_write_colo_context(libxl__egc *egc, >> + libxl__stream_write_state *stream, >> + libxl_sr_colo_context *colo_context) >> +{ >> + assert(stream->running); >> + assert(!stream->in_checkpoint); >> + assert(!stream->in_colo_context); >> + stream->in_colo_context = true; >> + >> + write_colo_context(egc, stream, colo_context); > > Use setup_write() here, which will remove most of the rest of this > patch, and cover all your error handling for your. setup_write() was introduced from your v4 series I think? so I missed that part when rebasing... > > You want a small pair of functions such as the write_checkpoint_end() > pair. See write_toolstack_record() for an example using setup_write() > with a body. > > Also, to preempt Ian Jacksons review, use FILLZERO() over "= { ... }" Ok, thank you for the kindly explaination, will address this in the next version. > > ~Andrew > . > -- Thanks, Yang.