On Tue, 13 Jun 2017 16:06:31 +0800 David Gibson <da...@gibson.dropbear.id.au> 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.
--
Greg
pgpXEq2VKcUXk.pgp
Description: OpenPGP digital signature
