On Tue, Jun 13, 2017 at 12:05:52PM +0200, Greg Kurz wrote:
> On Tue, 13 Jun 2017 10:55:50 +0100
> "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
>
> > * Greg Kurz (gr...@kaod.org) wrote:
> > > On Tue, 13 Jun 2017 10:00:03 +0100
> > > "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> > >
> > > > * Greg Kurz (gr...@kaod.org) wrote:
> > > > > 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.
> > > >
> > > > So what's the problem with a (void *)i ?
> > >
> > > https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> > >
> > > > It's simple, as long as you're
> > > > not actually using the opaque anywhere it's easy.
> > > >
> > >
> > > but as you say, since the opaque isn't used anywhere, it is probably
> > > okay to pass (void *) i.
> >
> > Yes, I don't think we're ever casting back from the (void *) to an int
> > so it feels pretty safe to me.
> >
>
> Just one nit, gcc doesn't like it on 64-bit hosts:
>
> hw/ppc/spapr.c: In function ‘pre_2_10_vmstate_register_dummy_icp’:
> hw/ppc/spapr.c:148:60: error: cast to pointer from integer of different
> size [-Werror=int-to-pointer-cast]
> vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, (void *) i);
>
> I need to cast to (long) first.
No, you need to cast to uintptr_t first.
--
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
signature.asc
Description: PGP signature
