Re: [Qemu-devel] [PATCH v5 26/45] Postcopy page-map-incoming (PMI) structure

[Dr. David Alan Gilbert]

* David Gibson (address@hidden) wrote:
> On Fri, Mar 13, 2015 at 01:47:53PM +0000, Dr. David Alan Gilbert wrote:
> > * David Gibson (address@hidden) wrote:
> > > On Wed, Feb 25, 2015 at 04:51:49PM +0000, Dr. David Alan Gilbert (git) 
> > > wrote:
> > > > From: "Dr. David Alan Gilbert" <address@hidden>
> [snip]
> > > > +        mis->postcopy_pmi.state0[BIT_WORD(bitmap_index)] |= 
> > > > shifted_mask;
> > > > +    } else {
> > > > +        mis->postcopy_pmi.state0[BIT_WORD(bitmap_index)] &= 
> > > > ~shifted_mask;
> > > > +    }
> > > > +    if (state & 2) {
> > > > +        mis->postcopy_pmi.state1[BIT_WORD(bitmap_index)] |= 
> > > > shifted_mask;
> > > > +    } else {
> > > > +        mis->postcopy_pmi.state1[BIT_WORD(bitmap_index)] &= 
> > > > ~shifted_mask;
> > > > +    }
> > > > +}
> > > > +
> > > > +/*
> > > > + * Retrieve the state of the given page
> > > > + * Note: This version for use by callers already holding the lock
> > > > + */
> > > > +static PostcopyPMIState postcopy_pmi_get_state_nolock(
> > > > +                            MigrationIncomingState *mis,
> > > > +                            size_t bitmap_index)
> > > > +{
> > > > +    bool b0, b1;
> > > > +
> > > > +    b0 = test_hpbits(mis, bitmap_index, mis->postcopy_pmi.state0);
> > > > +    b1 = test_hpbits(mis, bitmap_index, mis->postcopy_pmi.state1);
> > > > +
> > > > +    return (b0 ? 1 : 0) + (b1 ? 2 : 0);
> > > 
> > > Ugh.. this is a hidden dependency on the PostcopyPMIState enum
> > > elements never changing value.  Safer to code it as:
> > >       if (!b0 && !b1) {
> > >           return POSTCOPY_PMI_MISSING;
> > >       } else if (...)
> > >            ...
> > > 
> > > and let gcc sort it out.
> > 
> > Again, I was trying to make this just the interface; so it doesn't
> > know or care about the enum mapping; we can change the enum mapping to
> > the bits without changing this function (or the callers) at all.
> 
> So.. I'm not entirely clear what you mean by that.  I think what
> you're saying is that this function basically returns an arbitrary bit
> pattern derived from the state maps, and the enum provides the mapping
> from those bit patterns to meaningful states?
> 
> That's.. subtle :/.

I'm saying that I'd like everywhere to work in terms of the enum; but
since I can't store the array of enums I need to convert somewhere;
if I can keep the conversion to only being a couple of functions that know
about the bit layout and everything else uses those functions, then it
feels safe/clean.

Dave

> 
> -- 
> David Gibson                  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au        | minimalist, thank you.  NOT _the_ 
> _other_
>                               | _way_ _around_!
> http://www.ozlabs.org/~dgibson


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK