#47699 Propagate plugin precedence to all registered function types
Closed: wontfix None Opened 10 years ago by abbra.

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:

  • make the plugin call directly slapi_register_plugin_ext (like it is already done in retroCL plugin). The drawback is that *_ext routine is not publicly documented.
  • use the slapi_register_plugin argument: group_identity. To retrieve the precedence value and set it before calling plugin_setup

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.

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.

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.

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.

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]:

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.

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.

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.

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

7 years ago

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.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

3 years ago

Login to comment on this ticket.

Metadata