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

[Anthony Liguori <anthony@codemonkey.ws>]

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.
>