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);
}}}

revision - Improved error logging
0001-Ticket-47451-add-enable-disable-remove-plugins-witho.patch

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...

ack!

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 dbfab50e01c9ae494c26e3c567cbd8adb7c59b18
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Mar 20 16:09:43 2014 -0400

attachment
0001-Ticket-47451-Remove-old-code-from-linked-attr-plugin.patch

Thanks, Mark!

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 3c66c73e3300e5037df99215a277fb3660a83c07
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.

unregister tasks created by plugins
0001-Ticket-47451-Need-to-unregister-tasks-created-by-plu.patch

Replying to [comment:37 mreynolds]:

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

Indeed. Thanks, Mark!

ad7885e..005c4c9 master -> master
commit 005c4c9be360a7ebba38b61f934ace94224eef3f
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 fb7eef1c08a2d15c94eaedaabec2b07e970ffb3a

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

Automember
Managed Entry
MemberOf
Referential Integrity
Pam Passthru

Fix crash caused by plugin task creation
0001-Ticket-47451-Running-a-plugin-task-can-crash-the-ser.patch

d8e8119..0e0848a master -> master
commit 0e0848a8385463532d53db94c0c8cae912c30eb4
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 d34b0ced2ed3ed81c6c487e90d4c372939da4677

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

6c43c22..ff023a4 master -> master
commit ff023a48b7fc78711eed8ae11e67057597acdeb3
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 f17159e73ba0851599c4e50c1bf52d3d10f2711f

FIx compiler warnings
0001-Ticket-47451-Fix-jenkins-errors.patch

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

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

lib389 (CI) test suite
0001-Ticket-47451-Add-Dynamic-Plugin-CI-Suite.patch

Thanks Noriko!

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

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

Fix thread synchronization and various fixes
0001-Ticket-47451-Dynamic-plugins-fixed-thread-synchroniz.patch

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

Looks good to me.

4ae6794..14e5422 master -> master
commit 14e5422328d8f116916efb4a9e192b8db4686e44
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 7be03f890c1773727332686f6c164450d4a31996

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.

Fix crash from invalid plugin config area
0001-Ticket-47451-dynamic-plugins-fix-crash-caused-by-inv.patch

Thanks, Mark. I feel better now. :)

9300e96..d7be25e master -> master
commit d7be25e108a9ef57a4ccae38c88550d2fbe85578
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 a61c3cb754f1f4831d74b67d7a692a75b448f36f

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

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.