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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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

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