When plugin is registered, nsslapd-pluginPrecedence attribute value is used to set plugin's precedence. However, if plugin registers multiple callbacks, precedence is only set into a single plugin type list.
This happens due to the fact that plugin lists are not sorted according to precedence after the plugin init function is called.
As result, nsslapd-pluginType: object plugins get default precedence for all but a single plugin function type. In my case it is not even SLAPI_PLUGIN_TYPE_OBJECT but rather SLAPI_PLUGIN_INTERNAL_POSTOPERATION. In FreeIPA we need to re-arrange schemacompat and ipa-pwd-extop plugins so that schemacompat pre-bind operation precedes ipa-pwd-extop's one. As result of this bug, both are assigned the same default precedence and ipa-pwd-extop precedes schemacompat no matter what configuration I apply since both plugins provide multiple callbacks.
My understanding is that when a plugin (with its own nsslapd-pluginprecedence value) registers others callback (with slapi_register_plugin), those callbacks do not inherit the precedence of the plugin. Instead they are loaded with PLUGIN_DEFAULT_PRECEDENCE.
Basically it would require to expose, slapi_register_plugin_ext in addition to slapi_register_plugin because slapi_register_plugin_ext allows to set the precedence.
A possibility to pass the precedence to slapi_register_plugin_ext would be to use the 'argv' argument of slapi_register_plugin. Usually argv=NULL. An option would be to search in argv[] for something like, ATTR_PLUGIN_PRECEDENCE=<value>. If it exists and value is valid, we use value for the callback precedence (call to slapi_register_plugin_ext), else we use PLUGIN_DEFAULT_PRECEDENCE.
The advantage is that it does not change the slapi-plugin interface, but would need to be documented.
I'm not sure this is how it would need to be done. Here is why:
callbacks registered when plugin's init function is called by slapi_register_plugin(). At this point the plugin is already loaded and its precedence value already known as plugin->plg_precedence.
if you call plugin's init() function, you'll get all needed plugin callbacks already set in the pblock, so after init() call you can go and re-arrange precedence in the plugin's entries in global_plugin_list[] per each registered callback type.
Ideally, the latter action (inserting plugin to a corresponding global_plugin_list[] list) should happen once the callbacks are set in the pblock, by passing plugin->plg_precedence value to the function that adds a plugin to the corresponding list because then you don't need to sort anything in addition to what already happens.
Currently there is two discussed options:
attachment 0001-Ticket-ticket47699-Propagate-plugin-precedence-to-al.patch
Thanks, this patch does the job and makes possible to re-arrange object type plugins according to the configuration.
{{{
2229 /* If the plugin belong to a group, get the precedence from the group */ 2230 if (group) { 2231 struct slapi_componentid * cid = (struct slapi_componentid *) group; 2232 if (cid->sci_plugin && 2233 (cid->sci_plugin->plg_precedence != PLUGIN_DEFAULT_PRECEDENCE) && 2234 (cid->sci_plugin->plg_precedence >= PLUGIN_MIN_PRECEDENCE) && 2235 (cid->sci_plugin->plg_precedence <= PLUGIN_MAX_PRECEDENCE)) { 2236 plugin->plg_precedence = cid->sci_plugin->plg_precedence; 2237 } 2238 }
}}}
I guess one problem would be if the "group" plugin had some precedence set explicitly, and the "member" plugin had the precedence set explicitly to the value of PLUGIN_DEFAULT_PRECEDENCE. Then the "group" precedence would override the "member" precedence, which is probably not desirable.
If precedence is >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE, would that be checked earlier when parsing the plugin entry? I mean, is it even possible, at this point in the code, for the precedence to be >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE?
What happens when the nsslapd-precedence is updated via LDAP modify?
{{{ I guess one problem would be if the "group" plugin had some precedence set explicitly, and the "member" plugin had the precedence set explicitly to the value of PLUGIN_DEFAULT_PRECEDENCE. Then the "group" precedence would override the "member" precedence, which is probably not desirable. }}}
I agree but I am unsure how it can happen. If a member plugin has a dedicated precedence, that would mean it has a config entry as a plugin. So why using slapi_register_plugin for it. slapi_register_plugin register it with DEFAULT_PRECEDENCE.
{{{ If precedence is >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE, would that be checked earlier when parsing the plugin entry? I mean, is it even possible, at this point in the code, for the precedence to be >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE? }}}
Yes that is correct, it has already been check when loading the group plugin. I will remove this
{{{ What happens when the nsslapd-precedence is updated via LDAP modify? }}} It is modified in the group config entry. I need to double check, but I believe it has no impact on the plugin list order until the group plugin is reinit at the next startup.
Replying to [comment:9 tbordaz]:
{{{ I guess one problem would be if the "group" plugin had some precedence set explicitly, and the "member" plugin had the precedence set explicitly to the value of PLUGIN_DEFAULT_PRECEDENCE. Then the "group" precedence would override the "member" precedence, which is probably not desirable. }}} I agree but I am unsure how it can happen. If a member plugin has a dedicated precedence, that would mean it has a config entry as a plugin.
I agree but I am unsure how it can happen. If a member plugin has a dedicated precedence, that would mean it has a config entry as a plugin.
This code (the proposed patch) is only called when a plugin is registered with slapi_register_plugin()?
So why using slapi_register_plugin for it. slapi_register_plugin register it with DEFAULT_PRECEDENCE.
Ok.
{{{ If precedence is >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE, would that be checked earlier when parsing the plugin entry? I mean, is it even possible, at this point in the code, for the precedence to be >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE? }}} Yes that is correct, it has already been check when loading the group plugin. I will remove this {{{ What happens when the nsslapd-precedence is updated via LDAP modify? }}} It is modified in the group config entry. I need to double check, but I believe it has no impact on the plugin list order until the group plugin is reinit at the next startup.
This may have some impact on the work that Mark R. is doing about being able to dynamically load/unload plugins.
Also, the proposed test is for ticket 47679?
Replying to [comment:10 rmeggins]:
Replying to [comment:9 tbordaz]: {{{ I guess one problem would be if the "group" plugin had some precedence set explicitly, and the "member" plugin had the precedence set explicitly to the value of PLUGIN_DEFAULT_PRECEDENCE. Then the "group" precedence would override the "member" precedence, which is probably not desirable. }}} I agree but I am unsure how it can happen. If a member plugin has a dedicated precedence, that would mean it has a config entry as a plugin. This code (the proposed patch) is only called when a plugin is registered with slapi_register_plugin()? So why using slapi_register_plugin for it. slapi_register_plugin register it with DEFAULT_PRECEDENCE. Ok. {{{ If precedence is >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE, would that be checked earlier when parsing the plugin entry? I mean, is it even possible, at this point in the code, for the precedence to be >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE? }}} Yes that is correct, it has already been check when loading the group plugin. I will remove this {{{ What happens when the nsslapd-precedence is updated via LDAP modify? }}} It is modified in the group config entry. I need to double check, but I believe it has no impact on the plugin list order until the group plugin is reinit at the next startup. This may have some impact on the work that Mark R. is doing about being able to dynamically load/unload plugins.
Not really... Currently, my plugin work does not do anything on a ldapmodify except update the dependencies(and stopping and starting the plugin). So a restart of the plugin would be required for the precedence to take effect, but I think this is something worth adding to the dynamic update functionality - so I'll work on adding it.
attachment 0001-Ticket-47699-test-case.patch
Replying to [comment:9 tbordaz]: {{{ I guess one problem would be if the "group" plugin had some precedence set explicitly, and the "member" plugin had the precedence set explicitly to the value of PLUGIN_DEFAULT_PRECEDENCE. Then the "group" precedence would override the "member" precedence, which is probably not desirable. }}} I agree but I am unsure how it can happen. If a member plugin has a dedicated precedence, that would mean it has a config entry as a plugin. This code (the proposed patch) is only called when a plugin is registered with slapi_register_plugin()?
No it is called for 'group' and 'member' plugins.
Group plugins are entries (objectClass=nsSlapdPlugin) in the config (dse.ldif). Member plugins are "virtual entries" that are created (in slapi_register_plugin_ext) in the initialization routine of the a plugin. Although a Member plugin entry has a DN, this entry does not exist in the config, it is just used to register a callback.
plugin_setup is called when loading the 'group plugins' (load_plugin_entry), it is also called if the plugin initialization routine calls slapi_register_plugin() for a 'member' plugin.
'member' plugin entry being virtual, I think only 'group' plugin can have 'precedence' and we have not 'member' plugin precedence to preserve.
So why using slapi_register_plugin for it. slapi_register_plugin register it with DEFAULT_PRECEDENCE. Ok. {{{ If precedence is >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE, would that be checked earlier when parsing the plugin entry? I mean, is it even possible, at this point in the code, for the precedence to be >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE? }}} Yes that is correct, it has already been check when loading the group plugin. I will remove this {{{ What happens when the nsslapd-precedence is updated via LDAP modify? }}} It is modified in the group config entry. I need to double check, but I believe it has no impact on the plugin list order until the group plugin is reinit at the next startup. This may have some impact on the work that Mark R. is doing about being able to dynamically load/unload plugins.
If a 'group' plugin is added and registers 'member' plugin they should "inherit" the precedence.
Now if a 'group' plugin is disabled, its 'member' plugins must removed from the plugin list. Else they could be registered with a different precedence (if the plugin is reenabled). I have not found how it was done.
Thanks Rich for looking at the test case... I changed it with the right one. sorry for the confusion
Replying to [comment:13 tbordaz]:
Replying to [comment:10 rmeggins]: Replying to [comment:9 tbordaz]: {{{ I guess one problem would be if the "group" plugin had some precedence set explicitly, and the "member" plugin had the precedence set explicitly to the value of PLUGIN_DEFAULT_PRECEDENCE. Then the "group" precedence would override the "member" precedence, which is probably not desirable. }}} I agree but I am unsure how it can happen. If a member plugin has a dedicated precedence, that would mean it has a config entry as a plugin. This code (the proposed patch) is only called when a plugin is registered with slapi_register_plugin()? No it is called for 'group' and 'member' plugins. Group plugins are entries (objectClass=nsSlapdPlugin) in the config (dse.ldif). Member plugins are "virtual entries" that are created (in slapi_register_plugin_ext) in the initialization routine of the a plugin. Although a Member plugin entry has a DN, this entry does not exist in the config, it is just used to register a callback. plugin_setup is called when loading the 'group plugins' (load_plugin_entry), it is also called if the plugin initialization routine calls slapi_register_plugin() for a 'member' plugin. 'member' plugin entry being virtual, I think only 'group' plugin can have 'precedence' and we have not 'member' plugin precedence to preserve. So why using slapi_register_plugin for it. slapi_register_plugin register it with DEFAULT_PRECEDENCE. Ok. {{{ If precedence is >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE, would that be checked earlier when parsing the plugin entry? I mean, is it even possible, at this point in the code, for the precedence to be >= PLUGIN_MIN_PRECEDENCE or <= PLUGIN_MAX_PRECEDENCE? }}} Yes that is correct, it has already been check when loading the group plugin. I will remove this {{{ What happens when the nsslapd-precedence is updated via LDAP modify? }}} It is modified in the group config entry. I need to double check, but I believe it has no impact on the plugin list order until the group plugin is reinit at the next startup. This may have some impact on the work that Mark R. is doing about being able to dynamically load/unload plugins. If a 'group' plugin is added and registers 'member' plugin they should "inherit" the precedence. Now if a 'group' plugin is disabled, its 'member' plugins must removed from the plugin list. Else they could be registered with a different precedence (if the plugin is reenabled). I have not found how it was done.
In the work I am doing this is handled correctly. You turn off the main plugin, all its registered plugins are removed from the global list - FYI.
attachment 0002-Ticket-ticket47699-Propagate-plugin-precedence-to-al.patch
Thierry, can you log a message that when the precedence is invalid, that the default precedence will be used instead? Thanks!
Replying to [comment:16 mreynolds]:
Just saw your new patch, I think you should still do the value validation, and report an error when its invalid and the default precedence is used.
Replying to [comment:17 mreynolds]:
Replying to [comment:16 mreynolds]: Thierry, can you log a message that when the precedence is invalid, that the default precedence will be used instead? Thanks! Just saw your new patch, I think you should still do the value validation, and report an error when its invalid and the default precedence is used.
Actually plugin_setup() already does the precedence validation(and rejects the plugin all together), sorry for my noise.
attachment 0003-Ticket-ticket47699-Propagate-plugin-precedence-to-al.patch
The only issue I see is that this should be added to slapi_register_plugin_ext() and not slapi_register_plugin(). Some plugins like retro changelog call the extended function directly. Looks good otherwise.
attachment 0004-Ticket-ticket47699-Propagate-plugin-precedence-to-al.patch
attachment 0004-Ticket-47699-Propagate-plugin-precedence-to-all-regi.patch
ack
Thanks to Mark and Rich for these long reviews !
git merge ticket47699 Updating bbe0a99..272bd14 Fast-forward ldap/servers/slapd/plugin.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
git push origin master
Counting objects: 11, done. Delta compression using up to 4 threads. Compressing objects: 100% (6/6), done. Writing objects: 100% (6/6), 1.29 KiB, done. Total 6 (delta 4), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git bbe0a99..272bd14 master -> master
commit 272bd14 Author: Thierry bordaz (tbordaz) tbordaz@redhat.com Date: Wed Feb 19 17:03:03 2014 +0100
== Backport to 389-ds-base-1.3.2 ==
git cherry-pick 272bd14 [389-ds-base-1.3.2 5bf85e6] Ticket 47699: Propagate plugin precedence to all registered function types 1 file changed, 16 insertions(+), 1 deletion(-)
git push origin 389-ds-base-1.3.2
Counting objects: 11, done. Delta compression using up to 4 threads. Compressing objects: 100% (6/6), done. Writing objects: 100% (6/6), 1.29 KiB, done. Total 6 (delta 4), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git 981c951..5bf85e6 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 5bf85e6 Author: Thierry bordaz (tbordaz) tbordaz@redhat.com Date: Wed Feb 19 17:03:03 2014 +0100
Metadata Update from @tbordaz: - Issue assigned to tbordaz - Issue set to the milestone: 1.3.2.12
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/1035
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.