[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 04/18] libxl/save: Refactor libxl__domain_suspend_state
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |