On Thu, 14 Jan 2010 07:13:01 -0500
Stephen Gallagher <sgallagh(a)redhat.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 01/13/2010 05:38 PM, Simo Sorce wrote:
> On Wed, 13 Jan 2010 16:54:54 -0500
> Stephen Gallagher <sgallagh(a)redhat.com> wrote:
>
>> It's bad practice to modify current_time in a tevent_timer handler.
>
> I agree with the nack, but I want to explain why it is really
> bad! :)
>
> There is no problem in using the tevent_timer current_time in the
> handler, it is just a copy that will die as soon as you return from
> the handler.
>
> The real problem here is that current_time is not the *actual*
> time, it can easily be (0, 0).
>
> This means that setting it to tv_sec += 1 means causing a busy loop
> that spins as fast as it can because tv_sec = 1 is always in the
> past and causes tevent to fire immediately.
>
> Simo.
>
Well, I would consider that a bug in the tevent API, then. What's the
value at all of passing a "current_time" argument to the caller if it
is not accurate. There are cases where a user may want to use this
value to examine how long after the timer was set to fire it ACTUALLY
fired.
If there are ever cases where current_time == { 0, 0 }, this should be
logged as a bug in tevent.
Setting current_time to 0,0 in some conditions is deliberate, I don't
think you can change that now as it may break existing stuff.
But this argument is tangential to this patch review.
What matters is that using it as it was used in the patch is wrong :-)
Simo.
--
Simo Sorce * Red Hat, Inc * New York