Database plugins, like chaining, handle ldap operation on their own. So they never call any pre/post op plugins(or backend txn pre/post op plugins for that matter). This can break existing deployments that were using chaining before 389 switched over to using backend txn plugins across the board.
In the case of chaining, the RI plugin is a possible chaining component, but since it is now a backend txn plugin it will never get called for delete operations on a chaining backend.
Also, the RI plugin, just like the attribute uniqueness plugin, will have to know to look at all possible sub-suffixes/backends to properly handle the group cleanup (See ticket 47777).
cleanup code before working on fix 0001-Ticket-47792-code-cleanup.patch
ok, but note that, in general, large whitespace only commits will make cherry-picking/backporting harder
Code cleanup
git merge ticket47792 Updating 2b98dca..d341b77 Fast-forward ldap/servers/plugins/chainingdb/cb_add.c | 128 +++++++++++++-------------- ldap/servers/plugins/chainingdb/cb_delete.c | 133 ++++++++++++++-------------- ldap/servers/plugins/chainingdb/cb_modify.c | 172 +++++++++++++++++------------------- ldap/servers/plugins/chainingdb/cb_modrdn.c | 158 +++++++++++++++++---------------- ldap/servers/plugins/chainingdb/cb_search.c | 385 +++++++++++++++++++++++++++++++++++++++------------------------------------------
2b98dca..d341b77 master -> master commit d341b77
9cf2aa3..65c5e58 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit 65c5e58
f94bfa1..ea1b127 389-ds-base-1.3.1 -> 389-ds-base-1.3.1 commit ea1b127112efd3e962d363f12fcbae6f3b18c06a
The function names are misleading e.g. slapi_plugin_call_preop_betxn_plugins calls both the be and betxn plugins. Maybe just slapi_plugin_call_preop_be_plugins? Not sure about a good name.
{{{ 165 if ( NULL != ctrls) 166 ldap_controls_free(ctrls); }}} It is ok to call ldap_controls_free with NULL.
I do not like the way the calls to be_txn plugins are made public. In the cb case you probably call the plugins at the right place, but if a function to call thes plugins is public it could be used to call be txn plugins at any time.
Replying to [comment:7 lkrispen]:
I do not like the way the calls to be_txn plugins are made public. In the cb case you probably call the plugins at the right place, but if a function to call these plugins is public it could be used to call be txn plugins at any time.
I see your point, but database plugins need to be able to call these be/betxn plugins(starting in 1.3.1 most plugins are now betxn plugins). Someone writing their own plugin is already doing so at their own risk. We can't stop them from crashing the server or introducing memory leaks, and I think the same applies in this case. I think what we need here is proper documentation on how to use these functions.
Replying to [comment:6 rmeggins]:
Sure, I'll work on a better name.
I just checked mozldap and openldap, and yes it is safe to call this function if the controls are NULL. Maybe in the past it wasn't, as most places in the server code do this NULL check.
revised - new functions are now private 0001-Ticket-47792-database-plugins-need-a-way-to-call-bet.patch
Moved the functions into slapi-private.h, later we can easily move them to the public api if there is a need to do so.
New patch attached.
git merge ticket47792 Updating 087356f..9929b43 Fast-forward ldap/servers/plugins/chainingdb/cb_add.c | 76 ++++++++++++++++++++++++++++++++++++++++++---------------------------------- ldap/servers/plugins/chainingdb/cb_close.c | 4 ++-- ldap/servers/plugins/chainingdb/cb_delete.c | 58 ++++++++++++++++++++++++++++++---------------------------- ldap/servers/plugins/chainingdb/cb_modify.c | 60 +++++++++++++++++++++++++++++++----------------------------- ldap/servers/plugins/chainingdb/cb_modrdn.c | 61 ++++++++++++++++++++++++++++++++----------------------------- ldap/servers/plugins/chainingdb/cb_search.c | 44 ++++++++++++++++---------------------------- ldap/servers/slapd/plugin.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ldap/servers/slapd/slapi-plugin.h | 6 ++++++ ldap/servers/slapd/slapi-private.h | 34 ++++++++++++++++++++++++++++++++++ 9 files changed, 286 insertions(+), 150 deletions(-)
git push origin master 087356f..9929b43 master -> master
commit 9929b43 Author: Mark Reynolds mreynolds@redhat.com Date: Wed May 7 12:29:03 2014 -0400
1.3.2
2132875..8eeb738 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit 8eeb738
1.3.1
40e86e7..ba53b57 389-ds-base-1.3.1 -> 389-ds-base-1.3.1 commit ba53b5761f37074454723e83c70e5ec0ebd6ab6f
Metadata Update from @rmeggins: - Issue assigned to mreynolds - Issue set to the milestone: 1.3.1.23
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 issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1123
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.