From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Hongyang Subject: Re: [PATCH v1 1/5] libxl/save: Refactor libxl__domain_suspend_state Date: Wed, 3 Jun 2015 17:50:41 +0800 Message-ID: <556ECDF1.8030306@cn.fujitsu.com> References: <1432116090-19486-1-git-send-email-yanghy@cn.fujitsu.com> <1432116090-19486-2-git-send-email-yanghy@cn.fujitsu.com> <1433256008.15036.290.camel@citrix.com> <556DD55B.1070606@cn.fujitsu.com> <1433323443.7108.31.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1433323443.7108.31.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Ian Jackson Cc: wei.liu2@citrix.com, wency@cn.fujitsu.com, andrew.cooper3@citrix.com, yunhong.jiang@intel.com, xen-devel@lists.xen.org, rshriram@cs.ubc.ca List-Id: xen-devel@lists.xenproject.org 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.