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

[Yang Hongyang <yanghy@xxxxxxxxxxxxxx>]

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.

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