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

[Yang Hongyang <yanghy@cn.fujitsu.com>]

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.