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

[Wen Congyang <wency@xxxxxxxxxxxxxx>]

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?

>> +    }
>> +
>> +    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().

> 
>> +
>> +    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) ...

Thanks
Wen Congyang

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




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