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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

On Fri, Mar 13, 2015 at 01:47:53PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (david@gibson.dropbear.id.au) wrote:
> > On Wed, Feb 25, 2015 at 04:51:49PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
[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 :/.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]