Re: [PATCH v6 04/18] libxl/save: Refactor libxl__domain_suspend_state

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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

?