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

[Ian Campbell <ian.campbell@citrix.com>]

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.

Ian.