https://bugzilla.novell.com/show_bug.cgi?id=472046
User eich@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=472046#c3
--- Comment #3 from Egbert Eich 2009-02-09 02:55:22 MST ---
I've got severe issues with this patch:
1. (minor)
+ timeout = min(timeout, XSyncValueLow32 (value));
and
+ timeout = min(timeout, 0);
why min()? It's a define, timeout and XSyncValueLow32() are unsigned,
in the first case it's initialized to -1 in the latter to some value,
therefore the result is always clear.
This doesn't change the behavior of the function.
2. if (pIdleTimeValueLess &&
XSyncValueLessOrEqual (idle, *pIdleTimeValueLess))
{
- AdjustWaitForDelay (wt, 0);
....
+ if (trig->CheckTrigger(trig, old_idle)) {
+ AdjustWaitForDelay(wt, 0);
+ break;
+ }
}
else if (pIdleTimeValueGreater)
{
...
}
changes the function: AdjustWaitForDelay(wt, 0) in the original form was
called whenever the first condition was true. Therefore the second test for
pIdleTimeValueGreater was not required.
However in the new version it may not have been called while the test for
pIdleTimeValueGreater may still succeed. Thus we may end up not calling it
at all although it should have been called.
This the else should be removed and the 'break' after the
AdjustWaitForDelay(wt, 0); should be replaced by:
IdleTimeCounter->value = old_idle; return;
3. I don't see why we need to check for the trigger in this function. The
greater and less brackets should have been set by
SyncComputeBracketValues()
so that djustWaitForDelay() only gets called when CheckTrigger for at least
one Trigger would return true. I think the point of of having these
brackets
is exactly to not have to loop over all triggers every time
IdleTimeBlockHandle() is called.
I suspect the bug to be elsewhere however the investigation wasn't done
thoroughly enough.
--
Configure bugmail: https://bugzilla.novell.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.