Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Dr. David Alan Gilbert]

* Greg Kurz (address@hidden) wrote:
> On Tue, 13 Jun 2017 16:06:31 +0800
> David Gibson <address@hidden> wrote:
> 
> > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:
> [...]
> > > > > > > +static void 
> > > > > > > pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int 
> > > > > > > i)
> > > > > > > +{
> > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > +
> > > > > > > +    g_assert(!*flag);      
> > > > > > 
> > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > >     
> > > > > 
> > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > But I agree it isn't really useful... but more because of paranoia :) 
> > > > >    
> > > > 
> > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > to hand.  Adding a data structure just for the purpose of making an
> > > > assert() later, not so much.
> > > >   
> > > 
> > > It is also passed as opaque argument to vmstate_register(), where it is
> > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > to pass numbers.  
> > 
> > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > server number makes sense to identify the vmstate, but it looks like
> > vmstate_unregister() doesn't take that.
> > 
> 
> Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> 
> Cc'ing Juan and David.

So what's the problem with a (void *)i ? It's simple, as long as you're
not actually using the opaque anywhere it's easy.

Note from a quick glance at your patch;  will this work migrating
from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
the 2.9 ?

Dave


> --
> Greg


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