Re: [PATCH v15 2/7] remus: introduce remus device

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

On 07/11/2014 10:12 AM, Hongyang Yang wrote:
>
>
> On 07/11/2014 01:34 AM, Ian Jackson wrote:
>> Hongyang Yang writes ("Re: [PATCH v15 2/7] remus: introduce remus device"):
>>> On 07/10/2014 01:26 AM, Ian Jackson wrote:
>>>> Sorry to mention this now, but I think it would be clearer if all
>>>> these libxl__remus_device_* functions which manipulate _all_ the
>>>> devices for a domain used the plural of "device", ie
>>>> libxl__remus_devices_setup, etc.
>>>
>>> Ok
>>
>> Thanks.  You may find git-filter-branch helpful to do this easily.
>> (Be careful not to blow your leg off.)
>
> Thanks for the tip.
>
>>
>>>>> +    rds->num_devices++;
>>>>> +    /*
>>>>> +     * we add devices that have been setuped to the array no matter
>>>>> +     * the setup process succeed or failed because we need to ensure
>>>>> +     * the device been teardown while setup failed. If any of the
>>>>> +     * device setup failed, we will quit remus, but before we exit,
>>>>> +     * we will teardown the devices that have been added to **dev
>>>>> +     */
>>>>> +    rds->dev[rds->num_set_up++] = dev;
>>>>> +    if (rc) {
>>>>> +        /* setup failed */
>>>>> +        rs->saved_rc = ERROR_FAIL;
>>>>> +    }
>>>>
>>>> This doesn't look right.  If the setup fails, presumably we shouldn't
>>>> put the device in the array ?  If we do it will presumably be torn
>>>> down later.
>>>
>>> the netbuf script was designed as below:
>>> 1. when setup failed, the script won't teardown the device itself.
>>> 2. the teardown op is ok to be excute many times.
>>
>> Aha.  Right.
>>
>> I think you should state exactly that, probably in a comment here and
>> also in the script itself.  This can replace the comment you have
>> above, which is rather vague.
>>
>>> In the remus device layer, if one device setup failed(whether script
>>> exit or the script get killed or something), we will quit
>>> remus, but before that, we will teardown all device that has been set
>>> up, whether it's correctly set up or not. So if we don't put the
>>> device in the array, we will get a leaked device, that has not been
>>> teardown.
>>
>> That makes sense.
>>
>>>> I still don't understand why libxl__remus_device_state is not part of
>>>> libxl__remus_state.
>>>
>>> libxl__remus_device_state is part of libxl__remus_state:
>>> +struct libxl__remus_state {
>> ...
>>> +    libxl__remus_device_state dev_state;
>>>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> I meant: why is it a separate structure, rather than the contents
>> simply being included there ?
>>
>
> Err, I thought I've replied on the last thread about this, but I will
> reply here.
> I intend to use libxl__remus_state on upper layer, that is, in the
> main flow of libxl layer, and use libxl__remus_device_state in both
> remus abstract layer and concrete layer. I thought that will make
> things more clear. But yes, there still some libxl__remus_state use
> in remus abstract layer and concrete layer, I will fix it up in the
> next version.

I reconsidered about this case, most part of libxl__remus_state can be
merged into libxl__remus_device_state, there's only one left:
libxl__ev_time timeout; /* used for checkpoint */
but I think this can be moved to libxl__domain_suspend_state just like
interval for remus:
struct libxl__domain_suspend_state {
...snip...
     const char *dm_savefile;
     int interval; /* checkpoint interval (for Remus) */
+   libxl__ev_time checkpoint_timeout; /* used for remus checkpoint */
...snip...
};

So, I'll merge libxl__remus_state into libxl__remus_device_state.

>
>>
>> Thanks for your other replies.  You don't seem to have replied to
>> everything I said, including some questions I asked.  Do you intend
>> to, later, perhaps ?
>
> Sorry, I might have missed some of your comments or questions, but
> that's not what I was intend to...I was trying to reply every question
> you've pointed out. I will go back and go through your comments carefully.
>
>>
>> Thanks,
>> Ian.
>> .
>>
>

-- 
Thanks,
Yang.