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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.

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.

Ian.


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