#47451 add/enable/disable/remove plugins without server restart
Closed: Fixed None Opened 5 years ago by rmeggins.

Allow dynamically adding/enabling/disabling/removing plugins without requiring a server restart.

Setting up IPA is quite painful due to all of the restarts required, and plugins is one reason.


In addition, we should allow one to change the pluginConfigArea attribute for plug-ins that support it without requiring a restart.

Linked to Bugzilla bug: https://bugzilla.redhat.com/show_bug.cgi?id=994690 (''Red Hat Enterprise Linux 6'')

Replying to [comment:2 nkinder]:

In addition, we should allow one to change the pluginConfigArea attribute for plug-ins that support it without requiring a restart.

This was just completed for the RI and memberOf plugins via tickets 47525 & 47603.

Replying to [comment:7 nkinder]:

I missed this milestone change, but I'm very close to finishing this.

I'm going to have to review a chunk at a time, so please bear with me.

It looks like every plugin has to maintain its own reference counter. Can this be centralized in the plugin code e.g. add a counter to struct slapdplugin? The plugin.c code knows exactly when a plugin function is called and returns, and knows when the plugin close function is called.

Replying to [comment:10 rmeggins]:

I'm going to have to review a chunk at a time, so please bear with me.

Understood, I know its a big patch, and thanks for starting to tackle it. FYI, most of the tricky work is all in plugin.c.

It looks like every plugin has to maintain its own reference counter. Can this be centralized in the plugin code e.g. add a counter to struct slapdplugin?

I will definitely look into this. The op counter code was the very last thing I did, and I wasn't exactly thrilled about it, but we need to have something like it in place.

The rest of the code looks good.

Replying to [comment:12 rmeggins]:

The rest of the code looks good.

The new patch is attached. I added the counter to the plugin structure, so the counter init and destroy is handled when a plugin is started and stopped. There a few plugins where I could not use this: statechange, posix winsync, etc, as these use special registered callback functions that do have have access the plug-in structure. So those plugins are using there own private counters. Also, tasks do not have access to the plugin structure, so I'm using a task ref counter for those plugin task functions.

Why do the acct_policy, pam_passthru, passthru, retrocl, roles, rootdn plugins have to call slapi_plugin_op_?
Do the automember, dna, linked_attrs, member_of, mep, referint, usn, views plugins have to call slapi_plugin_op_
because they may be running as tasks? The calls are not in the functions exclusively used for tasks.

posix-winsync is a winsync plugin, not a "regular" plugin. It (and statechange, and a couple of other plugins) use the slapi api broker plugin code. Will we need to do some additional work on those types of plugins?

Replying to [comment:14 rmeggins]:

Why do the acct_policy, pam_passthru, passthru, retrocl, roles, rootdn plugins have to call slapi_plugin_op_*?

That's how the op counter is incremented/decremented.

I could move some of this code out of the individual plugin code, and into plugin_call_func() (start and finish plugin op calls), but we still need slapi_plugin_op_all_finished() in the close function - so we know when it's safe to free the resources.

Do the automember, dna, linked_attrs, member_of, mep, referint, usn, views plugins have to call slapi_plugin_op_* because they may be running as tasks?

For tasks, I also had to add a separate counter, local to the plugin. The current op counter implementation is storing the counter in the plugin entry(inside of the pblock). Tasks can not access this plugin entry and they often create their own threads, so I had use a separate global ref counter for these tasks. So there are technically two counters used, one for active operations, and one for tasks.

The same for some callbacks, they don't have access to the pblock/plugin either.

The calls are not in the functions exclusively used for tasks.

posix-winsync is a winsync plugin, not a "regular" plugin. It (and statechange, and a couple of other plugins) use the slapi api broker plugin code. Will we need to do some additional work on those types of plugins?

Well, I used a single local/separate counter for these plugins. I could try and modify the api broker code, but we would need two counters then - instead of one central one for the entire plugin.

So, at the end of day I could not use the exact same method of operation counting for all the plugins and tasks. I'm not sure we can implement a 100%, behind the scenes, global plugin operation counter(not without making some major changes). I can look into it more closely, but it would mean redoing the task code, api-broker, callbacks?, etc. Thoughts? I'm open to ideas.

Replying to [comment:15 mreynolds]:

Replying to [comment:14 rmeggins]:

Why do the acct_policy, pam_passthru, passthru, retrocl, roles, rootdn plugins have to call slapi_plugin_op_*?

That's how the op counter is incremented/decremented.

I could move some of this code out of the individual plugin code, and into plugin_call_func() (start and finish plugin op calls),

That's what I would prefer. It is already quite difficult to write plugins correctly - this is just one more thing that will trip people up.

but we still need slapi_plugin_op_all_finished() in the close function - so we know when it's safe to free the resources.

Can't that be done in plugin_call_plugins() or wherever the plugin close function is called?

Do the automember, dna, linked_attrs, member_of, mep, referint, usn, views plugins have to call slapi_plugin_op_* because they may be running as tasks?

For tasks, I also had to add a separate counter, local to the plugin. The current op counter implementation is storing the counter in the plugin entry(inside of the pblock). Tasks can not access this plugin entry and they often create their own threads, so I had use a separate global ref counter for these tasks. So there are technically two counters used, one for active operations, and one for tasks.

The same for some callbacks, they don't have access to the pblock/plugin either.

The calls are not in the functions exclusively used for tasks.

posix-winsync is a winsync plugin, not a "regular" plugin. It (and statechange, and a couple of other plugins) use the slapi api broker plugin code. Will we need to do some additional work on those types of plugins?

Well, I used a single local/separate counter for these plugins. I could try and modify the api broker code, but we would need two counters then - instead of one central one for the entire plugin.

So, at the end of day I could not use the exact same method of operation counting for all the plugins and tasks. I'm not sure we can implement a 100%, behind the scenes, global plugin operation counter(not without making some major changes). I can look into it more closely, but it would mean redoing the task code, api-broker, callbacks?, etc. Thoughts? I'm open to ideas.

I'm not sure. But I do know that we need to make it easier to write plugins, not harder. So the more we can do behind the scenes, automatically, the better.

Replying to [comment:16 rmeggins]:

I could move some of this code out of the individual plugin code, and into plugin_call_func() (start and finish plugin op calls),

That's what I would prefer. It is already quite difficult to write plugins correctly - this is just one more thing that will trip people up.

but we still need slapi_plugin_op_all_finished() in the close function - so we know when it's safe to free the resources.

Can't that be done in plugin_call_plugins() or wherever the plugin close function is called?

Yeah I was just thinking about that too.

So, at the end of day I could not use the exact same method of operation counting for all the plugins and tasks. I'm not sure we can implement a 100%, behind the scenes, global plugin operation counter(not without making some major changes). I can look into it more closely, but it would mean redoing the task code, api-broker, callbacks?, etc. Thoughts? I'm open to ideas.

I'm not sure. But I do know that we need to make it easier to write plugins, not harder.

Agreed.

So the more we can do behind the scenes, automatically, the better.

Ok, I'll continue to work on this.

This is an impressive amount of work. I have one question regarding 'plugin_delete'. This function is called when you disable/remove a plugin.

If this plugin init function registered others plugins (slapi_register_plugin), then 'plugin_delete' removes those plugins from the plugin list based on the plugin->plg_dn. That is fine.

Now my understand is that you may pick up the wrong plugin list. In fact there is a plugin list per plugin type. There is nothing that guarantee that 'group' plugin type has the same type than the 'member' plugin type. Actually it is even common than a 'group' plugin registers 'member' plugins from different types. For example, 'pam' plugin that is 'betxnpreoperation', registers three plugins (preop, internalpreop and betxnpostoperation).

It is looking like that the type (so the plugin list) of 'member' is only defined in the plugin init function. Shouldn't be the group plugin terminaison function that should unregister the 'member' plugins ?

Replying to [comment:18 tbordaz]:

This is an impressive amount of work. I have one question regarding 'plugin_delete'. This function is called when you disable/remove a plugin.

If this plugin init function registered others plugins (slapi_register_plugin), then 'plugin_delete' removes those plugins from the plugin list based on the plugin->plg_dn. That is fine.

Now my understand is that you may pick up the wrong plugin list. In fact there is a plugin list per plugin type. There is nothing that guarantee that 'group' plugin type has the same type than the 'member' plugin type. Actually it is even common than a 'group' plugin registers 'member' plugins from different types. For example, 'pam' plugin that is 'betxnpreoperation', registers three plugins (preop, internalpreop and betxnpostoperation).

It is looking like that the type (so the plugin list) of 'member' is only defined in the plugin init function. Shouldn't be the group plugin terminaison function that should unregister the 'member' plugins ?

Actually plugin_remove_plugins() scans all the plugin type lists for member functions. This was something I ran into very early on in my work. So yes, for example a preop plugins can register, internalpreop, extendop, etc, functions, and this will correctly identify those plugins and remove them.

If I understand this correctly, any change to any plugin parameter, from plugin path to include/exclude subtree attributes to log access/audit, will cause the plugin to be deleted then re-added back. This seems a bit heavyweight. For some changes e.g. plugin path, initfn, type, etc. there really is no other way. But for example, if I just wanted to add a subtree to the exclusion list, or enable audit logging for a plugin, I probably don't want to delete and add the plugin, which will wipe out all of the data in memory stored by the plugin. On the other hand, maybe this is good enough for now, and we can separate the handling of the parameters which cause a plugin restart from the others in a separate ticket.

The dse callbacks are problematic. We are taking away the dse callback argument. I don't think we can do that. I think we need a new function e.g. slapi_config_register_callback_plugin which must be used for any plugin that wants to be dynamically deleted/added. This function is exactly the same as slapi_config_register_callback except that it has a *pb argument which is used to associated the dse callback with the slapi plugin. Then, I think DSE_FLAG_PLUGIN won't be needed.

Replying to [comment:22 rmeggins]:

If I understand this correctly, any change to any plugin parameter, from plugin path to include/exclude subtree attributes to log access/audit, will cause the plugin to be deleted then re-added back. This seems a bit heavyweight. For some changes e.g. plugin path, initfn, type, etc. there really is no other way. But for example, if I just wanted to add a subtree to the exclusion list, or enable audit logging for a plugin, I probably don't want to delete and add the plugin, which will wipe out all of the data in memory stored by the plugin. On the other hand, maybe this is good enough for now, and we can separate the handling of the parameters which cause a plugin restart from the others in a separate ticket.

We could definitely separate it into two types of config attributes, but I would like to do it in a separate ticket as this will require a non-trivial change, and this patch is large enough as it is.

The dse callbacks are problematic. We are taking away the dse callback argument. I don't think we can do that.

Well the callback argument wasn't really being used, so I thought I could take advantage of that. The reason I did that was to reduce the impact on existing plugins, and for writers of new plugins. I already added some new API functions, was just trying to keep it to a minimum. Anyway, no problem, I can add the new function.

I think we need a new function e.g. slapi_config_register_callback_plugin which must be used for any > plugin that wants to be dynamically deleted/added. This function is exactly the same as >slapi_config_register_callback except that it has a *pb argument which is used to associated the dse >callback with the slapi plugin. Then, I think DSE_FLAG_PLUGIN won't be needed.

True, we can move it all behind the scenes with a new function.

Replying to [comment:23 mreynolds]:

Replying to [comment:22 rmeggins]:

If I understand this correctly, any change to any plugin parameter, from plugin path to include/exclude subtree attributes to log access/audit, will cause the plugin to be deleted then re-added back. This seems a bit heavyweight. For some changes e.g. plugin path, initfn, type, etc. there really is no other way. But for example, if I just wanted to add a subtree to the exclusion list, or enable audit logging for a plugin, I probably don't want to delete and add the plugin, which will wipe out all of the data in memory stored by the plugin. On the other hand, maybe this is good enough for now, and we can separate the handling of the parameters which cause a plugin restart from the others in a separate ticket.

Yes, agreed.

We could definitely separate it into two types of config attributes, but I would like to do it in a separate ticket as this will require a non-trivial change, and this patch is large enough as it is.

The dse callbacks are problematic. We are taking away the dse callback argument. I don't think we can do that.

Well the callback argument wasn't really being used, so I thought I could take advantage of that.

They are being used, and in addition, since they are part of the public API, someone could be using them, and we would break that.

The reason I did that was to reduce the impact on existing plugins, and for writers of new plugins.

There is already going to be an impact on plugins that use DSE callbacks - there is no way around that - unless there is some tricky way to associate a DSE callback with a plugin, but I don't see how that is possible.

I already added some new API functions, was just trying to keep it to a minimum. Anyway, no problem, I can add the new function.

I think we need a new function e.g. slapi_config_register_callback_plugin which must be used for any > plugin that wants to be dynamically deleted/added. This function is exactly the same as >slapi_config_register_callback except that it has a *pb argument which is used to associated the dse >callback with the slapi plugin. Then, I think DSE_FLAG_PLUGIN won't be needed.

True, we can move it all behind the scenes with a new function.

New patch attached. The patch is still in the coverity queue, but there are 12 jobs ahead of it. Not sure when its actually going to complete, so while I'm waiting I figured it would be best to get this patch out for review in the meantime.

Looking good. One minor nit:
{{{
3385 PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,"Plugin delete failed: dependency error "
3386 "prevents the plugin from being deleted/disabled.");
}}}
text should say which plugin it is. same with
{{{
3161 LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3162 "by other plugins, it can not be dynamically changed at this time. This could "
3163 "cause issues when the server is restarted.\n",0,0,0);
}}}
and
{{{
LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3166 "by other plugins, it can not be disabled/moved at this time.\n",0,0,0);
}}}

Replying to [comment:26 rmeggins]:

Looking good. One minor nit:
{{{
3385 PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,"Plugin delete failed: dependency error "
3386 "prevents the plugin from being deleted/disabled.");
}}}
text should say which plugin it is. same with
{{{
3161 LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3162 "by other plugins, it can not be dynamically changed at this time. This could "
3163 "cause issues when the server is restarted.\n",0,0,0);
}}}
and
{{{
LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3166 "by other plugins, it can not be disabled/moved at this time.\n",0,0,0);
}}}

Okay the error message (error log and client returntext) will list all the plugins that rely on this plugin type when the dependency check fails New patch is attached...

Thanks Rich!

git merge dynamic-plugin
Updating e4a1de6..dbfab50
Fast-forward
ldap/ldif/template-dse.ldif.in | 1 +
ldap/schema/01core389.ldif | 1 +
.../plugins/acct_usability/acct_usability.c | 9 +-
ldap/servers/plugins/acctpolicy/acct_config.c | 9 +
ldap/servers/plugins/acctpolicy/acct_init.c | 19 +
ldap/servers/plugins/acctpolicy/acctpolicy.h | 1 +
ldap/servers/plugins/acl/acl.h | 1 +
ldap/servers/plugins/acl/acl_ext.c | 16 +-
ldap/servers/plugins/acl/aclgroup.c | 4 +-
ldap/servers/plugins/acl/aclplugin.c | 1 +
ldap/servers/plugins/automember/automember.c | 135 +--
ldap/servers/plugins/chainingdb/cb.h | 7 +-
ldap/servers/plugins/chainingdb/cb_close.c | 76 +-
ldap/servers/plugins/chainingdb/cb_instance.c | 60 +-
ldap/servers/plugins/cos/cos.c | 3 +-
ldap/servers/plugins/cos/cos_cache.c | 14 +-
ldap/servers/plugins/deref/deref.c | 9 +-
ldap/servers/plugins/dna/dna.c | 70 +-
ldap/servers/plugins/linkedattrs/fixup_task.c | 25 +-
ldap/servers/plugins/linkedattrs/linked_attrs.c | 70 +-
ldap/servers/plugins/linkedattrs/linked_attrs.h | 3 +
ldap/servers/plugins/memberof/memberof.c | 40 +-
ldap/servers/plugins/memberof/memberof.h | 4 +-
ldap/servers/plugins/memberof/memberof_config.c | 47 +-
ldap/servers/plugins/mep/mep.c | 63 +-
ldap/servers/plugins/pam_passthru/pam_passthru.h | 3 +-
ldap/servers/plugins/pam_passthru/pam_ptconfig.c | 9 +
ldap/servers/plugins/pam_passthru/pam_ptimpl.c | 9 +
ldap/servers/plugins/pam_passthru/pam_ptpreop.c | 41 +-
ldap/servers/plugins/passthru/passthru.h | 5 +
ldap/servers/plugins/passthru/ptconfig.c | 483 ++++----
ldap/servers/plugins/passthru/ptpreop.c | 15 +-
.../plugins/posix-winsync/posix-group-func.c | 7 +
.../plugins/posix-winsync/posix-group-func.h | 1 +
.../plugins/posix-winsync/posix-winsync-config.c | 23 +
ldap/servers/plugins/posix-winsync/posix-winsync.c | 128 ++-
.../plugins/posix-winsync/posix-wsp-ident.h | 5 +
ldap/servers/plugins/referint/referint.c | 23 +-
ldap/servers/plugins/retrocl/retrocl.c | 32 +-
ldap/servers/plugins/retrocl/retrocl_po.c | 13 +-
ldap/servers/plugins/roles/roles_cache.c | 12 +-
ldap/servers/plugins/roles/roles_plugin.c | 20 +-
ldap/servers/plugins/rootdn_access/rootdn_access.c | 27 +-
ldap/servers/plugins/schema_reload/schema_reload.c | 35 +-
ldap/servers/plugins/statechange/statechange.c | 48 +-
ldap/servers/plugins/sync/sync.h | 1 +
ldap/servers/plugins/sync/sync_init.c | 5 +-
ldap/servers/plugins/sync/sync_persist.c | 43 +-
ldap/servers/plugins/sync/sync_refresh.c | 11 +
ldap/servers/plugins/usn/usn.c | 27 +-
ldap/servers/plugins/usn/usn.h | 1 -
ldap/servers/plugins/usn/usn_cleanup.c | 13 +-
ldap/servers/plugins/views/views.c | 111 ++-
ldap/servers/plugins/whoami/whoami.c | 5 +-
ldap/servers/slapd/apibroker.c | 2 +
ldap/servers/slapd/config.c | 11 +-
ldap/servers/slapd/configdse.c | 2 +-
ldap/servers/slapd/dse.c | 287 ++++-
ldap/servers/slapd/factory.c | 210 ++--
ldap/servers/slapd/fedse.c | 48 +-
ldap/servers/slapd/libglobs.c | 31 +
ldap/servers/slapd/main.c | 8 +-
ldap/servers/slapd/plugin.c | 1426 +++++++++++++++++---
ldap/servers/slapd/proto-slap.h | 14 +-
ldap/servers/slapd/result.c | 1 -
ldap/servers/slapd/schema.c | 10 +-
ldap/servers/slapd/slap.h | 11 +
ldap/servers/slapd/slapi-plugin.h | 55 +
ldap/servers/slapd/task.c | 52 +-
ldap/servers/slapd/thread_data.c | 56 +-
ldap/servers/slapd/tools/pwenc.c | 7 +-
71 files changed, 2914 insertions(+), 1161 deletions(-)

git push origin master
e4a1de6..dbfab50 master -> master

commit dbfab50
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Mar 20 16:09:43 2014 -0400

git merge ticket47451
Updating 819cbfd..3c66c73
Fast-forward
ldap/servers/plugins/linkedattrs/fixup_task.c | 5 -----
ldap/servers/plugins/linkedattrs/linked_attrs.h | 7 -------
2 files changed, 12 deletions(-)

git push origin master
819cbfd..3c66c73 master -> master

commit 3c66c73
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed May 21 14:14:06 2014 -0400

Plugins that register tasks need to unregister them in the close function. Otherwise, repeatedly starting and stopping a plugin that registers tasks will corrupt the dse callback linked list and lead to a crash.

Hi Mark,
You fixed the tasks in these plug-ins.
ldap/servers/plugins/automember/automember.c | 8 ++++++++
ldap/servers/plugins/linkedattrs/linked_attrs.c | 2 ++
ldap/servers/plugins/memberof/memberof.c | 1 +
ldap/servers/plugins/schema_reload/schema_reload.c | 2 ++
ldap/servers/plugins/usn/usn.c | 1 +
ldap/servers/plugins/usn/usn.h | 1 +
ldap/servers/plugins/usn/usn_cleanup.c | 8 ++++++++
ldap/servers/slapd/slapi-plugin.h | 1 +
ldap/servers/slapd/task.c | 19 +++++++++++++++++++

I grepped task in the plugins directory, I see these, which contain extra... I'm curious they are okay to ignore?
automember/automember.c
linkedattrs/fixup_task.c
linkedattrs/linked_attrs.c
memberof/memberof.c
posix-winsync/posix-group-task.c
posix-winsync/posix-winsync-config.c
replication/repl5_replica.c
replication/repl5_replica_config.c
replication/repl_extop.c
schema_reload/schema_reload.c
syntaxes/validate_task.c
usn/usn_cleanup.c

Replying to [comment:35 nhosoi]:

I grepped task in the plugins directory, I see these, which contain extra... I'm curious they are okay to ignore?
automember/automember.c
linkedattrs/fixup_task.c
linkedattrs/linked_attrs.c
memberof/memberof.c
posix-winsync/posix-group-task.c
posix-winsync/posix-winsync-config.c

posix-winsync plugin needs a lot work...

replication/repl5_replica.c
replication/repl5_replica_config.c
replication/repl_extop.c

Replication plugin can not be restarted, so it is not affected by this.

schema_reload/schema_reload.c
syntaxes/validate_task.c

Syntax plugin can not be restarted, so it is not affected either.

usn/usn_cleanup.c

Actually posix winsync wasn't that bad. New patch attached.

Replying to [comment:37 mreynolds]:

Actually posix winsync wasn't that bad. New patch attached.

Indeed. Thanks, Mark!

ad7885e..005c4c9 master -> master
commit 005c4c9
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Nov 13 17:22:24 2014 -0500

3e7321b..fb7eef1 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit fb7eef1

Some shared config entries are not dynamically reloaded. Plugins that need checking:

Automember
Managed Entry
MemberOf
Referential Integrity
Pam Passthru

d8e8119..0e0848a master -> master
commit 0e0848a
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Nov 26 16:23:00 2014 -0500

ae06f96..d34b0ce 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit d34b0ce

Fix various issues, as well as a regression from previous patch
0001-Ticket-47451-Dynamic-Plugin-various-fixes.patch

6c43c22..ff023a4 master -> master
commit ff023a4
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Dec 16 15:25:35 2014 -0500

251888e..f17159e 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit f17159e

ff023a4..3e1d976 master -> master
commit 3e1d976

f17159e..15e1cdd 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 15e1cdd

Thanks Noriko!

6207253..4e39dbb master -> master
commit 4e39dbb

a2977b4..2ace013 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 2ace013

Fixed various issues(crashes, memory leaks, and race conditions), and improved the CI test suite

4ae6794..14e5422 master -> master
commit 14e5422
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Dec 23 21:40:00 2014 -0500

f634732..7be03f8 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 7be03f8

Reopening... Found crash when setting an invalid value for a plugin attribute (the crash happens during the "revert back to the old plugin on error" phase)

Might be a paranoiac question :), in case "else" of the condition at line 1924 and 1925, rc and returncode are both 0. That is considered as SLAPI_DSE_CALLBACK_DO_NOT_APPLY at the following switch (rc). Is it okay or is it to be reset to SLAPI_DSE_CALLBACK_OK?

slapi-plugin.h:#define SLAPI_DSE_CALLBACK_OK (1)
slapi-plugin.h:#define SLAPI_DSE_CALLBACK_ERROR (-1)
slapi-plugin.h:#define SLAPI_DSE_CALLBACK_DO_NOT_APPLY (0)

{{{
1924 if(config_get_dynamic_plugins() &&
1925 slapi_entry_attr_hasvalue(ec, SLAPI_ATTR_OBJECTCLASS, "nsSlapdPlugin") )
1926 {
....
1926 1954 }
}}}

Other than this question, your patch looks good to me.

Replying to [comment:56 nhosoi]:

Might be a paranoiac question :), in case "else" of the condition at line 1924 and 1925, rc and returncode are both 0. That is considered as SLAPI_DSE_CALLBACK_DO_NOT_APPLY at the following switch (rc). Is it okay or is it to be reset to SLAPI_DSE_CALLBACK_OK?

Yes it should be reset, but I'm not sure its possible for the plugin_start to fail, and not the plugin_restart. Regardless, I changed it :-) New patched attached.

Thanks, Mark. I feel better now. :)

9300e96..d7be25e master -> master
commit d7be25e
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Feb 9 16:23:04 2015 -0500

411a8e7..a61c3cb 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit a61c3cb

Metadata Update from @tbordaz:
- Issue assigned to mreynolds
- Issue set to the milestone: 1.3.3 backlog

2 years ago

Login to comment on this ticket.

Metadata