Shouldn't we translate the 'result' here? like with ldap_err2string

If I run py.test -s -v dirsrvtests/tests/suites/plugins/acceptance_test.py::test_automember - the instance crashes here.
In the test case, it is 467 line - task = plugin.fixup(branch2.dn, 'objectclass=top')

I still didn't investigate the automember code base enough, but can it be that the crash happens because of config->inclusive_rules == NULL at that point of the code path?

Also, I have 'ERR - auto-membership-plugin - automember_update_member_value - result: 16' - at some point (LDAP_NO_SUCH_ATTRIBUTE). I don't know if it's related

If I run py.test -s -v dirsrvtests/tests/suites/plugins/acceptance_test.py::test_automember - the instance crashes here.
In the test case, it is 467 line - task = plugin.fixup(branch2.dn, 'objectclass=top')
I still didn't investigate the automember code base enough, but can it be that the crash happens because of config->inclusive_rules == NULL at that point of the code path?

Investigating...

Also, I have 'ERR - auto-membership-plugin - automember_update_member_value - result: 16' - at some point (LDAP_NO_SUCH_ATTRIBUTE). I don't know if it's related

It's not related, but that logging line was for my debugging, and I will remove it completely.

Issues are fixed and tests pass. Please review again...

Lots of this block of code seems applicable to the add case too, chance to make a function for membership check/modification?

int32_t ? size_t ?

ii should be size_t as it's used in an array deref

!= PLUGIN_SUCCESS? Depends what error results we could possibly return, but != is defensive.

modrdn case is already defined I guess? What about if deleting an attribute would now cause the entry to be part of the group?

I agree with William regarding 'int' issues and regarding possibly another test case option.
The rest looks good to me.

!= PLUGIN_SUCCESS? Depends what error results we could possibly return, but != is defensive.

I think the idea was to have these three 'rc' in automember_update_membership():

* Return SLAPI_PLUGIN_FAILURE for failures, or
*        SLAPI_PLUGIN_SUCCESS for success (no memberships updated), or
*        MEMBERSHIP_UPDATED   for success (memberships updated)

We don't return anything else so the logic depends on checking if it's success, failure or membership_updated.
But I agree that the != PLUGIN_SUCCESS logic is a more straightforward approach in general.

modrdn case is already defined I guess? What about if deleting an attribute would now cause the entry to be part of the group?

The post_op modify handles this case - it doesn't really matter what the mod ops are (add attr/delete attr) because we are looking at the entry as a whole after all mods are applied. Basically it takes a snap shot of the pre and post entry automember groups, and then the group differences are cleaned up from the post entry. The CI test essentially covers this scenario.

LGTM!

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

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.