qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Gibson
Subject: Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU
Date: Tue, 13 Jun 2017 18:01:46 +0800
User-agent: Mutt/1.8.0 (2017-02-23)

On Tue, Jun 13, 2017 at 11:21:50AM +0200, Greg Kurz wrote:
> On Tue, 13 Jun 2017 10:00:03 +0100
> "Dr. David Alan Gilbert" <address@hidden> wrote:
> 
> > * 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 ?
> 
> 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.

Right, that's probably the right short term approach.


Incidentally, for a somewhat safer-than-standard approach to this
common problem, see https://ccodearchive.net/info/ptrint.html

-- 
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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]