From: Howard Chu Sent: Friday, May 23, 2014 5:30 PM
you have completely changed the semantics of the timestamp. The code explicitly used "now" - the current time when
the
operation completed, and you have changed it to use op->o_time, which is the time when the operation was first received. You said all you wanted to
do
was add microseconds, but this patch changes much more than that.
Yes, I almost went into more detail on that when I posted, I suppose I probably should have.
During my testing, the seconds in op->o_time were never different than now was set to by a call to time(). Assuming an operation never takes more than one second to complete, if you are working with a one second granularity, the start time and the stop time are roughly equivalent, never differing by more than one second (ie, start at 10.8, stop at 11.2, use 11 rather than 10). What is the worst case amount of time it would take to perform a bind operation? The change made was in ppolicy_bind_response, which unless I misunderstand, is only called for the bind operation, so the change in semantic time definition would only apply to binds, and impact the timestamp stored in pwdFailureTime, whether or not a given pwdFailureTime value was recent enough to consider, and whether or not a password was considered expired.
Further, while the semantics did change, is the new definition just different, or worse than the previous? Was there a specific reason it was decided to mark the time of failure as when the bind operation completed as opposed to when it began? What benefit does it provide?
I originally did intend to add microseconds to the existing call to time, but after reviewing other uses of fractional seconds, such as in the accesslog module, it seemed more convenient to use the op->o_tincr, which while not time-based also allows you to distinguish operations at a subsecond granularity. For the purpose of keeping track of the failures, it seemed not so important you actually logged the exact microsecond the failure occurred, but simply that you are able to distinguish multiple failures within one second. I considered keeping the call to time(), and simply adding op->o_tincr on to that, but then I would potentially be mixing time intervals.
If you want to keep the same semantics of the time being at the end of the operation rather than at the start, is it acceptable to call ldap_pvt_gettime from this module, and then call lutil_tm2time to get a lutil_timet containing both seconds and microseconds? ldap_pvt_gettime involves a mutex, and seemed like would be less efficient to use than the existing time and operation increment that was already present, given that a second or two delta didn't really seem to make a difference for the use that was made of the time.
Aside from that, there's no reason to make a 2nd redundant call to slap_timestamp - just copy the result from timestr to failtimestr.
You're right, I did not consider that. I will make that change.
Rejecting this patch.
Thank you for the feedback.