Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 09:29 AM, Avi Kivity wrote:
> On 02/07/2011 05:17 PM, Jan Kiszka wrote:
> > On 2011-02-07 16:13, Avi Kivity wrote:
> > >  On 02/07/2011 05:01 PM, Anthony Liguori wrote:
> > >>
> > >>  typedef struct PeriodicTimer PeriodicTimer;
> > >>
> > >>  /**
> > >>   * @accumulated_ticks:  the number of unacknowledged ticks in total
> > >>  since the creation of the timer
> > >>   **/
> > >
> > >  Outdated comment even before the code is committed.  Will be hard 
> > to beat.
> > >
> > >>  typedef void (PeriodicTimerFunc)(void *opaque);
> > >
> > >  s/void *opaque/PeriodicTimer *timer/
> > >
> > >  Down with opaques!
> >
> > What else? DeviceState?
>
> typedef void (PeriodicTimerFunc)(PeriodicTimer *timer);
>
> the callback then uses container_of() to get whatever its internal 
> data structure is from the embedded PeriodicTimer.

For the purposes of this, I think passing an opaque is better because 
the signature stays the same as the existing timer callback.  That makes 
conversion a bit friendlier.

I think it's better to avoid introducing stylistic changes with new 
interfaces.  I think they should be done separately.

Regards,

Anthony Liguori

> > >>
> > >>  PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void 
> > *opaque);
> > >>
> > >
> > >  void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc 
> > *cb);
> > >
> > >  It is better to embed than to reference.
> >
> > Likely, though this diverges from exiting QEMUTimer.
>
> That's the more modern style.  Saves allocations and dereferences, and 
> is more type safe.