LGTM!

While you're touching it, it could be worth changing "long" to int32_t here (similar with retVal), but not necessary

Hey,

I think this is the kind of change that requires a test case to show that it hasn't broken the existing functions (Which I think exists), but also that we add a test case for the 0 case.

I also commented that there is a possible code cleanup, but it's not required.

Thanks,

Hey,
I think this is the kind of change that requires a test case to show that it hasn't broken the existing functions (Which I think exists), but also that we add a test case for the 0 case.

Agreed, I'll work on that tomorrow, and the code cleanup!

I also commented that there is a possible code cleanup, but it's not required.
Thanks,

Thank you!

@mreynolds just a dummy remark. pw_inhistory is used as the number of password kept in the history array (update_pw_history). Have you tested that when it is set to 0, the all history is not cleared ?

@mreynolds, rereading the ticket description I realize that clear of the history array was the not a concern. My understanding is that the ticket is to have the ability to reject a password update if the new value/current value are the same.
A concern is that we can not have both: ability to reject update if new_value=current_value AND new_value exist in history array. I feel I am missing something here.

@mreynolds just a dummy remark. pw_inhistory is used as the number of password kept in the history array (update_pw_history). Have you tested that when it is set to 0, the all history is not cleared ?

I have it works fine

@mreynolds, rereading the ticket description I realize that clear of the history array was the not a concern. My understanding is that the ticket is to have the ability to reject a password update if the new value/current value are the same.
A concern is that we can not have both: ability to reject update if new_value=current_value AND new_value exist in history array. I feel I am missing something here.

If you set passwordinhistry to "0", it only checks the current password, if you set it to "1" it checks the last password and the current password, etc. The only behavior change is when you use "0", in that case it never checks the entry's attributes, just the current password.

Changes made, and CI test updated. Please review...

I think we should use UserAccounts for all of these operations...

There is no need in these try-except blocks because the test case fails it will report all of the information to the output including a stack trace.

I think we should use UserAccounts for all of these operations...

Done, also had to add a new function to Accounts class to rebind on the same connection. Please review

Ohhh as a follow up, when you write the test, run the code with asan too please :)

Ohhh as a follow up, when you write the test, run the code with asan too please :)

Test was written up, but I can not test it anymore because setup-ds.pl is currently broken. I'll test it once that all gets straightened out. Do you still require a ASAN run for this patch (the CI was passing)?

@mreynolds Anytime I touch the C code, I run it with ASAN. It's caught so many mistakes of my own that it's invaluable when used with testing. I think it would be great for you to run it with ASAN as tthat would help give you more confidence in the code changes :)

ASAN tests pass, merging...

Thank you!

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/3215

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.