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 <eich@novell.com> 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.