Re: [PATCH v3 2/5] remus: resume immediately if libxl__xc_domain_save_done() completes

From: Ian Campbell <ian.campbell@citrix.com>
To: Wen Congyang <wency@cn.fujitsu.com>,
	xen devel <xen-devel@lists.xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Wei Liu <wei.liu2@citrix.com>,
	Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v3 2/5] remus: resume immediately if libxl__xc_domain_save_done() completes
Date: Fri, 15 Jan 2016 09:48:43 +0000	[thread overview]
Message-ID: <1452851323.32341.42.camel@citrix.com> (raw)
In-Reply-To: <56988731.5000206@cn.fujitsu.com>

On Fri, 2016-01-15 at 13:44 +0800, Wen Congyang wrote:
> On 01/14/2016 06:21 PM, Ian Campbell wrote:
> > On Tue, 2016-01-12 at 09:40 +0800, Wen Congyang wrote:
> > > On 01/09/2016 12:27 AM, Ian Campbell wrote:
> > > > On Fri, 2016-01-08 at 14:38 +0800, Wen Congyang wrote:
> > > > > For example: if the secondary host is down, and we fail to send
> > > > > the
> > > > > data
> > > > > to
> > > > > the secondary host. xc_domain_save() returns 0.
> > > > 
> > > > Just to be check: On failure in this way xc_domain_save() returns 0
> > > > (i.e.
> > > > success)?
> > > 
> > > Yes, it returns 0. I am not sure the return value is right.
> > > 
> > > > 
> > > > >  So in the function
> > > > > libxl__xc_domain_save_done(), rc is 0(the helper program exits
> > > > > normally),
> > > > > and retval is 0(it is xc_domain_save()'s return value). In such
> > > > > case,
> > > > > we
> > > > > just need to complete the stream.
> > > > > 
> > > > > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > > > > ---
> > > > >  tools/libxl/libxl_stream_write.c | 13 +++++++++++--
> > > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tools/libxl/libxl_stream_write.c
> > > > > b/tools/libxl/libxl_stream_write.c
> > > > > index 80d9208..82e7719 100644
> > > > > --- a/tools/libxl/libxl_stream_write.c
> > > > > +++ b/tools/libxl/libxl_stream_write.c
> > > > > @@ -354,8 +354,17 @@ void libxl__xc_domain_save_done(libxl__egc
> > > > > *egc,
> > > > > void *dss_void,
> > > > >       * alive, and check_all_finished() may have torn it down
> > > > > around
> > > > > us.
> > > > >       * If the stream is not still alive, we must not continue
> > > > > any
> > > > > work.
> > > > >       */
> > > > > -    if (libxl__stream_write_inuse(stream))
> > > > > -        write_emulator_xenstore_record(egc, stream);
> > > > > +    if (libxl__stream_write_inuse(stream)) {
> > > > > +        if (dss->remus)
> > > > > +            /*
> > > > > +             * For remus, if libxl__xc_domain_save_done()
> > > > > completes,
> > > > > +             * there was an error sending data to the secondary.
> > > > > +             * Resume the primary ASAP.
> > > > > +             */
> > > > > +            stream_complete(egc, stream, 0);
> > > > 
> > > > Is there an indication to the caller that things have failed in
> > > > this
> > > > way?
> > > > Would that information be of use to the caller?
> > > 
> > > For remus, when we come here, something is wrong regardless of the
> > > return
> > > value.
> > 
> > But does the caller know this? Can it tell.
> > 
> > > 
> > > > 
> > > > Or does the called infer this has happened because
> > > > otherwise libxl_domain_remus_start is not supposed to return?
> > > 
> > > Yes, libxl_domain_remus_start() should not return unless somethins is
> > > wrong.
> > 
> > This really ought to be documented somewhere.
> 
> libxl_domain_remus_start():
>     /* Point of no return */
>     libxl__remus_setup(egc, dss);
>     return AO_INPROGRESS;

This is (obviously) not documentation.

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