[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 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.


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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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