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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-06-30 at 17:43 +0800, Yang Hongyang wrote:
> 
> 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.

OK makes sense, thanks. You may as well mention this in the commit
message so I don't forget next time ;-)

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

Thanks.



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