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

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





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.

Ah, my thought was use "dsps" consistently, that is all placees that
using libxl__domain_suspend_state will be renamed to dsps.(after
moving the suspend code into libxl_dom_suspend.c, dsps mostly appears
there).It did confuse one if he is familiar with the old code. But I think
it will make the code clearer for new readers. Anyway, let's wait for
Ian (J)'s comment.


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.

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