From mboxrd@z Thu Jan  1 00:00:00 1970
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH v6 04/18] libxl/save: Refactor
 libxl__domain_suspend_state
Date: Tue, 26 Jan 2016 09:32:31 -0500
Message-ID: <20160126143231.GC19666@char.us.oracle.com>
References: <1451442548-26974-1-git-send-email-wency@cn.fujitsu.com>
	<1451442548-26974-5-git-send-email-wency@cn.fujitsu.com>
	<20160125172927.GJ14977@char.us.oracle.com>
	<56A6D8B8.9030200@cn.fujitsu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <56A6D8B8.9030200@cn.fujitsu.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Wen Congyang <wency@cn.fujitsu.com>
Cc: Lars Kurth <lars.kurth@citrix.com>, Changlong Xie <xiecl.fnst@cn.fujitsu.com>, Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Jiang Yunhong <yunhong.jiang@intel.com>, Ian Jackson <ian.jackson@eu.citrix.com>, xen devel <xen-devel@lists.xen.org>, Dong Eddie <eddie.dong@intel.com>, Gui Jianfeng <guijianfeng@cn.fujitsu.com>, Shriram Rajagopalan <rshriram@cs.ubc.ca>, Yang Hongyang <hongyang.yang@easystack.cn>
List-Id: xen-devel@lists.xenproject.org

On Tue, Jan 26, 2016 at 10:23:52AM +0800, Wen Congyang wrote:
> On 01/26/2016 01:29 AM, Konrad Rzeszutek Wilk wrote:
> > .snip..
> >> --- a/tools/libxl/libxl_dom_suspend.c
> >> +++ b/tools/libxl/libxl_dom_suspend.c
> >> @@ -19,14 +19,71 @@
> >>  
> >>  /*====================== Domain suspend =======================*/
> >>  
> >> +int libxl__domain_suspend_init(libxl__egc *egc,
> >> +                               libxl__domain_suspend_state *dsps)
> >> +{
> >> +    STATE_AO_GC(dsps->ao);
> >> +    int rc = ERROR_FAIL;
> >> +    int port;
> >> +    libxl_domain_type type;
> >> +
> >> +    /* Convenience aliases */
> >> +    const uint32_t domid = dsps->domid;
> >> +
> >> +    type = libxl__domain_type(gc, domid);
> >> +    switch (type) {
> >> +    case LIBXL_DOMAIN_TYPE_HVM: {
> >> +        dsps->hvm = 1;
> >> +        break;
> >> +    }
> >> +    case LIBXL_DOMAIN_TYPE_PV:
> >> +        dsps->hvm = 0;
> >> +        break;
> >> +    default:
> >> +        goto out;
> > 
> > This will mean we return back to libxl__domain_save which will goto out which calls:
> > domain_save_done. And that will try to use the dsps->guestevtchn leading to a crash since:
> 
> Yes, thanks for pointing it out. In which case, the type is not HVM or PV?

If you call those init routines before the switch statemet - such as the
libxl__xswait_init, etc, then you can still goto out
> 
> >> +    }
> >> +
> >> +    libxl__xswait_init(&dsps->pvcontrol);
> >> +    libxl__ev_evtchn_init(&dsps->guest_evtchn);
> > 
> > we initialize them here.
> >> +    libxl__ev_xswatch_init(&dsps->guest_watch);
> >> +    libxl__ev_time_init(&dsps->guest_timeout);
> > 
> > I would instead recommend you move these initialization routines above the
> > 'type' check.
> 
> I think we should not return ERROR_FAIL when the type is not PV or HVM. We should abort the program
> like what we do in libxl__domain_save().

I would rather return - this is a library after all - so the controlling program should
do such drastic measures - not an library.

> 
> > 
> >> +
> >> +    dsps->guest_evtchn.port = -1;
> >> +    dsps->guest_evtchn_lockfd = -1;
> >> +    dsps->guest_responded = 0;
> >> +    dsps->dm_savefile = libxl__device_model_savefile(gc, domid);
> >> +
> >> +    port = xs_suspend_evtchn_port(domid);
> >> +
> >> +    if (port >= 0) {
> >> +        rc = libxl__ctx_evtchn_init(gc);
> >> +        if (rc) goto out;
> >> +
> >> +        dsps->guest_evtchn.port =
> >> +            xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
> >> +                                    domid, port, &dsps->guest_evtchn_lockfd);
> >> +
> >> +        if (dsps->guest_evtchn.port < 0) {
> >> +            LOG(WARN, "Suspend event channel initialization failed");
> >> +            rc = ERROR_FAIL;
> >> +            goto out;
> >> +        }
> >> +    }
> >> +
> >> +    rc = 0;
> >> +
> >> +out:
> >> +    return rc;
> >> +}
> >> +
> > 
> > .. snip..
> >>  struct libxl__domain_suspend_state {
> >> +    /* set by caller of libxl__domain_suspend_init */
> >> +    libxl__ao *ao;
> >> +    uint32_t domid;
> >> +
> >> +    /* private */
> >> +    int hvm;
> > 
> > How about 'is_hvm' and just use 'libxl_domain_type' type?
> > instead of having an int? You can just do:
> 
> In dss, it is 'int hvm'.
> Before this patch:
> if (dss->hvm) ...
> After this patch:
> if (dsps->hvm) ...

Right..
> 
> Thanks
> Wen Congyang
> 
> > 
> > if (type == LIBXL_DOMAIN_TYPE_HVM) ..

But what if you use that? As in dsps->type == LIBXL_DOMAIAN_TYPE_HVM for example?

> > 
> > And to check for non-conforming types - you can make  libxl__domain_suspend_init
> > do this:
> > 
> >     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> >         rc = ERROR_FAIL;
> >         goto out; 
> >     }    
> > 
> > ?
> > 
> > 
> > .
> > 
> 
> 
>