From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH for-4.5 v2] libxl: Initialise CTX->xce in domain suspend Date: Mon, 5 Jan 2015 10:32:16 -0500 Message-ID: <20150105153216.GC11661@l.oracle.com> References: <1419233604-859-1-git-send-email-yanghy@cn.fujitsu.com> <21674.41273.269354.954668@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <21674.41273.269354.954668@mariner.uk.xensource.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: Ian Jackson Cc: Yang Hongyang , Wei Liu , xen-devel@lists.xensource.com, Ian Campbell List-Id: xen-devel@lists.xenproject.org On Mon, Jan 05, 2015 at 02:35:37PM +0000, Ian Jackson wrote: > Yang Hongyang writes ("[PATCH] xl/libxl: fix migrate/Remus regression (core dumped)"): > > When excuting xl migrate/Remus, the following error occurd: > > [root@master xen]# xl migrate 5 slaver > > migration target: Ready to receive domain. > > Saving to migration stream new xl format (info 0x1/0x0/1225) > > Loading new save file (new xl fmt info 0x1/0x0/1225) > > Savefile contains xl domain config in JSON format > > Parsing config from > > Segmentation fault (core dumped) > > > > This is because CTX->xce is used without been initialized. > > The bug was introduced by commit 2ffeb5d7f5d8 > > libxl: events: Deregister evtchn fd when not needed > > which remove the initialization of xce from libxl__ctx_alloc. > > > > This patch initialze the CTX->xce before use it. > > Thanks. This patch goes in the right direction, but isn't quite > correct because it doesn't check the return value from > libxl__ctx_evtchn_init. > > Looking at this it is clear that following the on-demand > initialisation of CTX->xce, it is normally necessary for any evtchn > user in libxl to call libxl__ctx_evtchn_init, since they will need the > xce for finding the right port number to pass to > libxl__ev_evtchn_wait. > > Sorry for not noticing this when I made my earlier change. > > I have therefore: > * In the patch below, added changes to the comments to document this. > * Done git grep '\bxce\b' tools/libxl and checked the other uses. > * Consequently, verified that the rest of the code in libxl_dom.c > avoids using xce unless guest_evtchn.port>=0, and properly > initialises .port to -1, so that there is no need for further calls > to libxl__ctx_evtchn_init. > > I have compiled but not executed this patch. Yang Hongyang: can you > please test that it fixes the bug for you ? > > Konrad: this should go in 4.5 because it is a bugfix without which > libxl may dereference NULL. OK. Release-Acked-by: Konrad Rzeszutek Wilk > > (I have also somewhat improved the English grammar in the commit > message.) > > Thanks, > Ian. > > commit 9d1cb27f5e961fd9db1c7d8381af18e33510f924 > Author: Ian Jackson > Date: Mon Jan 5 14:31:00 2015 +0000 > > libxl: Initialise CTX->xce in domain suspend, as needed > > When excuting xl migrate/Remus, the following error can occur: > [root@master xen]# xl migrate 5 slaver > migration target: Ready to receive domain. > Saving to migration stream new xl format (info 0x1/0x0/1225) > Loading new save file (new xl fmt info 0x1/0x0/12\ > ) > Savefile contains xl domain config in JSON format > Parsing config from > Segmentation fault (core dumped) > > This is because CTX->xce is used without been initialized. > The bug was introduced by commit 2ffeb5d7f5d8 > libxl: events: Deregister evtchn fd when not needed > which removed the initialization of xce from libxl__ctx_alloc. > > In this patch we initialise the CTX->xce before using it. Also, we > adjust the doc comment for libxl__ev_evtchn_* to mention the need to > do so. > > Signed-off-by: Yang Hongyang > Signed-off-by: Ian Jackson > Cc: Ian Campbell > Cc: Konrad Rzeszutek Wilk > Cc: Wei Liu > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index 74ea84b..94ae818 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -1824,6 +1824,9 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss) > port = xs_suspend_evtchn_port(dss->domid); > > if (port >= 0) { > + rc = libxl__ctx_evtchn_init(gc); > + if (rc) goto out; > + > dss->guest_evtchn.port = > xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce, > dss->domid, port, &dss->guest_evtchn_lockfd); > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 9695f18..6dac0f8 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -800,8 +800,10 @@ static inline int libxl__ev_xswatch_isregistered(const libxl__ev_xswatch *xw) > > /* > * The evtchn facility is one-shot per call to libxl__ev_evtchn_wait. > - * You should call some suitable xc bind function on (or to obtain) > - * the port, then libxl__ev_evtchn_wait. > + * You should: > + * Use libxl__ctx_evtchn_init to make sure CTX->xce is valid; > + * Call some suitable xc bind function on (or to obtain) the port; > + * Then call libxl__ev_evtchn_wait. > * > * When the event is signaled then the callback will be made, once. > * Then you must call libxl__ev_evtchn_wait again, if desired.