Simo Sorce wrote:
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 :-)
Well, it is a little bit confusing. What is current_time used for, then?
Martin