[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


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

That makes sense.

I still don't understand why libxl__remus_device_state is not part of

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 {
    const char *dm_savefile;
    int interval; /* checkpoint interval (for Remus) */
+   libxl__ev_time checkpoint_timeout; /* used for remus checkpoint */

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.



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.