Re: [Xen-devel] [PATCH v3 COLOPre 06/26] libxl/save: Refactor libxl__domain_suspend_state

[Yang Hongyang <yanghy@xxxxxxxxxxxxxx>]

On 06/30/2015 12:01 AM, Ian Campbell wrote:
> On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
> [...]
> >       switch (type) {
> >       case LIBXL_DOMAIN_TYPE_HVM: {
> >           dss->hvm = 1;
> > +        dsps->hvm = 1;
> >           break;
> >       }
> >       case LIBXL_DOMAIN_TYPE_PV:
> >           dss->hvm = 0;
> > +        dsps->hvm = 0;
> >           break;
> [...]
> > @@ -2913,9 +2914,27 @@ typedef struct libxl__logdirty_switch {
> >   } libxl__logdirty_switch;
> >
> >   struct libxl__domain_suspend_state {
> > +    /* set by caller of domain_suspend_callback_common */
> > +    libxl__ao *ao;
> > +
> > +    uint32_t domid;
> > +    int hvm;
> > +    /* private */
> > +    libxl__ev_evtchn guest_evtchn;
> > +    int guest_evtchn_lockfd;
> > +    int guest_responded;
> > +    libxl__xswait_state pvcontrol;
> > +    libxl__ev_xswatch guest_watch;
> > +    libxl__ev_time guest_timeout;
> > +    const char *dm_savefile;
> > +    void (*callback_common_done)(libxl__egc*,
> > +                                 struct libxl__domain_suspend_state*, int ok);
> > +};
> > +
> > +struct libxl__domain_save_state {
> >       /* set by caller of libxl__domain_suspend */
> >       libxl__ao *ao;
> > -    libxl__domain_suspend_cb *callback;
> > +    libxl__domain_save_cb *callback;
> >
> >       uint32_t domid;
> >       int fd;
> > @@ -2924,22 +2943,14 @@ struct libxl__domain_suspend_state {
> >       int debug;
> >       const libxl_domain_remus_info *remus;
> >       /* private */
> > -    libxl__ev_evtchn guest_evtchn;
> > -    int guest_evtchn_lockfd;
> > +    libxl__domain_suspend_state dsps;
> >       int hvm;
> I wonder if, given that any domain suspend must necessarily be contained
> within a domain save it would be preferable to have "suspend" code
> upcast the suspend_state to the containing save_state in order to look
> at ->hvm, rather than duplicating it. Likewise domid.
The reason we include hvm in suspend state is that we need a more common
suspend function we will use on restore side (with COLO). It should not touch
the upper struct, because it will be used on both save/restore side.

> For ao I can imagine that the suspend and save might actually have
> separate ao lifetimes, in which case these do of course need to remain
> different.
>
> Would that make any sense at all?
>
> FYI it was the dual initialisation of hvm quoted above which lead me to
> think along these lines.
>
> Alternatively perhaps suspend_state should a separate init function to
> logically separate it from the save_state initialiser. Perhaps taking
> the latter as an argument? Or possibly just the relevant fields.
Yes, the latter should be better. I will separate the init of the
suspend state.

> Ian.
>
> .
--
Thanks,
Yang.

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