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

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

On 06/03/2015 05:24 PM, Ian Campbell wrote:
> On Wed, 2015-06-03 at 00:10 +0800, Yang Hongyang wrote:
>> 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?
>
> So in some contexts you would have "dss" (existing save state) and
> "dsps" (new suspend state), which is potentially confusing so I think it
> would be worth renaming dss to dsvs in at least those places, it's
> mechanical so although noisy it's not too bad.
>
> Ian (J) is the main author if this code (and therefore chose the
> existing names), it might be worth giving him a chance to object or
> suggest an alternative.

OK, thanks

>
> Ian.
>
> .
>

-- 
Thanks,
Yang.