Re: [Qemu-devel] Re: [PATCH 27/49] ac97: add active to the state

[malc]

On Wed, 30 Sep 2009, Anthony Liguori wrote:

> malc wrote:
> > > > > +
> > > > >  typedef struct AC97LinkState {
> > > > >      PCIDevice dev;
> > > > >      QEMUSoundCard card;
> > > > > @@ -162,6 +169,7 @@ typedef struct AC97LinkState {
> > > > >      uint8_t silence[128];
> > > > >      uint32_t base[2];
> > > > >      int bup_flag;
> > > > > +    uint8_t active[LAST_INDEX];
> > > > >         
> > > > This doesn't belong here, cause the only purpose i can see is to hack
> > > > around defficiencies of the new load/savevm APIs.
> > > >       
> > > That was supposed to be one of the features, not deficiences.  You can't
> > > sent stuff that it is not in the state. It is "by design" that you can't
> > > sent arbitrary variables.
> > >     
> > 
> > active doesn't belong in the above structure, it's not used for anything
> > other than save/loadvm. If this "design" doesn't allow this, either find
> > another way to accomplish the same or fix the "design".
> >   
> 
> Looking briefly at the code, it looks like the active[] array isn't
> technically needed in the savevm state.
> 
> I think you could basically do:
> 
> AUD_set_active_in(s->voice_pi, !!(s->bm_regs[PI_INDEX].cr & CR_RPBM));
> ..
> 
> Better yet, active[] can be dynamically built from the contents of 
> bm_regs->cr so there would be little code change.
> 
> So I think we should bump the version of the ac97 format, remove the 
> active[] array from the vmstate, and then generate it in a post function 
> that can then be passed to reset_voices().

Yes, however, what bothers me though, is that i ended up NOT doing that
when the original code was written, there must have been a reason for me
not doing that, and it completely elludes me at the moment, so no this
isn't better yet, better yet implies someone having to go through the
code and figure out whether it's safe or not.

-- 
mailto:address@hidden