Re: [PATCH v1 1/5] libxl/save: Refactor libxl__domain_suspend_state

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

On 06/02/2015 10:40 PM, Ian Campbell wrote:
> On Wed, 2015-05-20 at 18:01 +0800, Yang Hongyang wrote:
>> @@ -1762,16 +1762,18 @@ static void libxl__domain_suspend_callback(void *data)
>>   {
>>       libxl__save_helper_state *shs = data;
>>       libxl__egc *egc = shs->egc;
>> -    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
>> +    libxl__domain_save_state *dss = CONTAINER_OF(shs, *dss, shs);
>> +    libxl__domain_suspend_state *dss2 = &dss->dss;
>
> With dss now being ambiguously save vs suspend I don't think adding a 2
> suffix to one of the usages is the right answer.
>
> I think in contexts where you are dealing with both that *save_state and
> *suspend_state are the way to go for the local variables. I'm afraid
> this will make the change noisier, sorry.
>
> I'm afraid I think that the dss member of struct
> libxl__domain_save_state will therefore also need to be called
> suspend_state too.
>
> I think we can tolerate using dss in contexts where there is only one of
> the two structs in active use, just to avoid even more noise.
>
> Alternatively if there is another name for either "save" or "suspend"
> which doesn't start with an s (or conflict with some other type) perhaps
> we could go with that. I can't think of one off hand.
>
> Another name might also help because the semantic difference between
> suspend and save is something I have to think about every time. Is there
> a split along live/dead lines which we could use here perhaps?
>
>
>>   static void domain_suspend_callback_common_done(libxl__egc *egc,
>>                                   libxl__domain_suspend_state *dss, int ok)
>>   {
>> -    libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, ok);
>> +    libxl__domain_save_state *dsvs = CONTAINER_OF(dss, *dsvs, dss);
>
> I suppose dsvs is a bit better then dss2. Maybe that's the answer, if
> used consistently.

If use dsvs to represent save_state consistently, the modification of the
code will be too much. I'm thinking of using "dsps" stands for
domain_suspend_state, is it ok?

>
> Ian.
>
> .
>

-- 
Thanks,
Yang.