Bug 202843

Created attachment 134332 [details]
LDIF for bug reproduction

Created attachment 142873 [details]
bug analysis and discussion

Comment from Pete:
You can use the start dependency code as a model for call ordering - the plugins
just need to be inserted in the list in the right order initially - they will
never have to be updated so no other code changes. 

Created attachment 142912 [details]
cvs diffs

Files:
 plugins/cos/cos_cache.c
 plugins/referint/referint.c
 slapd/delete.c
 slapd/modify.c
 slapd/modrdn.c
 slapd/pblock.c
 slapd/plugin.c
 slapd/slap.h
 slapd/slapi-plugin.h

Created attachment 142913 [details]
Fix proposal

Created attachment 142914 [details]
Test result: CoS is called after Referential Integrity Plugin

Attachment referred by the previous comment.

Sorry, it's confusing, isn't it?  Let me clarify some more.
Comment #4 is the diffs for the reviews.  
Comment #5 is the description for the diffs.  
Comment #6 shows just ordering the plugins was not enough for updating the CoS
cache.  So, Comment #6 is a sort of codicil.

Comment on attachment 142912 [details]
cvs diffs

found a bug in plugin_create_pluginlist:
+int 
+plugin_create_pluginlist( Slapi_Entry *plugin_entry, char *attr_name, 
+					 struct slapdplugin *plugin, struct
slapdplugin **plugin_list)
+{
+	 Slapi_Attr *attr = 0;
+	 int hint =0;
+	 int num_vals = 0;
+	 int val_index = 0;
+	 Slapi_Value *val;
+
+	 if( 0 == slapi_entry_attr_find( plugin_entry, attr_name, &attr ))
+	 {
+		 /* allocate memory for the string array */
+		 slapi_attr_get_numvalues( attr, &num_vals );
+
+		 if(num_vals)
+		 {
+			 hint = slapi_attr_first_value( attr, &val );
+			 while(val_index < num_vals)
+			 {
+				 /* add the plugin to the list at the place
befores */
+
+				 add_plugin_to_list_after( plugin_list, plugin,
slapi_value_get_string(val) );
+				 hint = slapi_attr_next_value( attr, hint, &val
);
+				 val_index++;
+			 }
+		 }

This does not work when multiple nsslapd-plugin-depends-on-call is set in one
plugin entry.

I'll fix it and send out a review request again.

Created attachment 143004 [details]
cvs diffs (revised)

Fixed a bug reported in the previous comment.
That is, added a code to delete the plugin if the plugin is already in the list
before the plugin with "name".

+static void
+add_plugin_to_list_after(struct slapdplugin **list,
+						  struct slapdplugin *plugin,
const char *name)
+{
	 struct slapdplugin **tmp;
-	 for ( tmp = list; *tmp; tmp = &(*tmp)->plg_next )
+	 PR_ASSERT(name && *name);
+	 for ( tmp = list; tmp && *tmp; tmp = &(*tmp)->plg_next )
	 {
+		 if (!strcasecmp(plugin->plg_name, (*tmp)->plg_name)) {
+			 /* plugin is already in the list before the plugin
with "name".
+			    delete it first */
+			 *tmp = (*tmp)->plg_next;
+			 if (NULL == *tmp)
+				 break;
+		 }
+		 else if (!strcasecmp(name, (*tmp)->plg_name))

Based upon the following advice from Rich, fixed in the different way.

Richard Megginson wrote:
> Could we solve this problem by having CoS (and probably Roles too) register
both a SLAPI_PLUGIN_INTERNAL_POST_op_FN and a SLAPI_PLUGIN_POST_op_FN?  I think
part of the problem is that CoS is not notified of changes during internal
operations - SLAPI_PLUGIN_INTERNAL_POST_op_FN might take care of that.
>
> If that doesn't work, maybe we could use some sort of generic entry state
change notification method.  I had forgotten about the statechange plugin which
could be used for this purpose.  It would work something like this:
> CoS (and Roles too, I think) registers an entry state change callback using
the slapi_apib interface.  With CoS, this can just simply call cos_post_op. 
Then, we change the internal_modify, internal_modrdn, internal_add, and
internal_delete functions to call this entry post change function using the apib
interface. 
    [...]
> I see.  Then can we register CoS as a "object" plugintype?  Several other
plug-ins take this approach, which need to register callbacks for preop, postop,
and other types.  For example, see the repl5_init.c - the main
replication_multimaster_plugin_init() calls slapi_register_plugin() with the
different types of plugins it wants to register as. 

Created attachment 143177 [details]
cvs diff (create_inctance.c cos.c roles_plugin.c)

Files:
  ldap/admin/src/create_instance.c 
  ldap/servers/plugins/cos/cos.c 
  ldap/servers/plugins/roles/roles_plugin.c

Changes:
1) registered cos_post_op and roles_post_op as SLAPI_PLUGIN_INTERNAL_POST_op_FN
functions.
2) changed the plugin type of CoS and Roles from "postoperation" to "object".

Richard Megginson wrote:
> Looks good.  Ship it! 

Thank you, Rich!!

Created attachment 143178 [details]
cvs commit message

Checked in into HEAD.

Created attachment 143368 [details]
problem description + review request

Created attachment 143369 [details]
cvs commit message

Reviewed by Rich (Thank you!)

Checked in into HEAD.

Created attachment 143568 [details]
Yet another side effect...

Comments from Rich:
I worry that there is some cleanup that we won't do if we skip the
plugin_closeall, but then we don't have any plugins (other than the database)
that run in db2ldif mode.  I guess let's see if the acceptance tests like the
change. 

I'm going to check in ldif2ldbm.c (with the dblayer_close call removed) and
check the acceptance results tomorrow.

Created attachment 143569 [details]
cvs commit ldif2ldbm.c

Checked in into HEAD.

I'm running the acceptance tests on 32-bit/64-bit RHEL4 from now...

verified aginst:
1193765112 idm-console-framework-1.1.0-5.el5idm Tue Oct 30 2007 
1193765112 redhat-idm-console-1.0.0-13.el5idm Tue Oct 30 2007 
1194380792 tftp-0.42-3.1 Tue Nov 06 2007 
1195006662 subversion-1.4.2-2.el5 Tue Nov 13 2007 
1195174828 redhat-ds-base-8.0.0-11.el5dsrv Thu Nov 15 2007