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: Mon, 25 Jan 2016 12:29:27 -0500
Message-ID: <20160125172927.GJ14977@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>
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: <1451442548-26974-5-git-send-email-wency@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

.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:
> +    }
> +
> +    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.

> +
> +    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:

if (type == LIBXL_DOMAIN_TYPE_HVM) ..

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; 
    }    

?