it's probably worth a PR_ASSERT here that timespec is 0 on both fields, and that rc is 0 as well so that in dev we catch any time we pass in un-init memory IMO.

it's probably worth a PR_ASSERT here that timespec is 0 on both fields, and that rc is 0 as well so that in dev we catch any time we pass in un-init memory IMO.

Sounds good to me... done.

Yep, this is much better I think, as it directly targets the problem in the csn gen, and prevents invalid memory being provided. Ack from me, but worth checking with @tbordaz too just in case I think :)

The acquisition can be moved later, for example when delta is computed

so there is no need to release the lock hear

return code should also be tested

return code should also be tested

Actually, it doesn't "need" to be checked :-) This is only for logging a message. If csngen_new_csn() fails then the time is set to zero. Then the rest of code will handle this and log it as a zero CSN. We will have already logged a message saying the time was not obtained. I don't think we need to keep logging how it failed.

The acquisition can be moved later, for example when delta is computed

Well I was debating this actually, but I figured since we are getting the "time", and could be waiting on that lock, that we should wait until we have the lock and then take the time. The time would technically be more accurate.

so there is no need to release the lock hear

If I keep the locking the same, then yes we do.

I'm not saying "no" to any of your suggestions, but I'd like to hear what you have to say in regards to my comments above :-) Thanks!

Regarding replica_generate_next_csn:csngen_new_csn, this part of code is not only for logging. It adjust the csn generator then retry to generate a greater csn (than the one before adjustment). With that branch of code, replica_generate_next_csn will succeeds even if csngen_new_csn fails and opcsn is let unchanged.

Regarding the lock, only one thread can generate a csn at a time, so I think the contention is on the big lock rather then the csn generator lock. So for a performance pov the fix is valid.
I just realized that the structure protected by the lock was accessed after the getttimeofday syscall so it would be better to acquire the lock later.

@tbordaz, thanks for the feedback, I'll adjust the patch..

The patch looks good to me. ACK

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4162

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.