<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#ffffff" text="#000000"> Thanks a lot, Rich! I revised my proposal based upon your suggestions. Some comments are in-line... --noriko On 08/26/2009 09:13 AM, Rich Megginson wrote: [...] <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite">About this code: if (new_attrs) { - *attrs = new_attrs; + charray_merge_nodup(attrs, new_attrs, 0); + slapi_ch_free((void **)&new_attrs); } the 0 flag to charray_merge_nodup() means "do not copy" - so the strings in new_attrs will be moved to attrs, if they are not duplicates. If they are duplicates, they will not be moved - how will they be freed? There's no way for the caller of charray_merge_nodup to know which strings were moved and which were not (except by comparing pointers in attrs with new_attrs). </blockquote> I wanted to avoid malloc/free as much as possible, but it looks I cannot... :( Changing it as follows: if (new_attrs) { charray_merge_nodup(attrs, new_attrs, 1); slapi_ch_array_free(new_attrs); } <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite"> + int i; + const char *val = slapi_value_get_string(sval); + for (i = slapi_attr_first_value(attr, &sval); I don't think you should be calling slapi_value_get_string() here since sval is not initialized yet. </blockquote> Thanks for finding it out! My copy & paste error... :( Changing it as follows: int i; const char *val = NULL; for (i = slapi_attr_first_value(attr, &sval); <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite"> I think slapi_set_plugin_default_config() should always to a modify/add, and just ignore "type or value exists" errors. Or support single valued attributes only. Or allow the user to specify to add or replace in the case of a multi-valued attribute. </blockquote> I was thinking the value is preferably single, but I wanted to support multiple values if set (e.g., by manually adding dse.ldif as you mentioned). That made the code uglier. I changed slapi_set_plugin_default_config simply adds the given "attr: value" pair to the plugin default config entry instead of replacing the existing pair(s) with it. <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite">What if the user modifies the list directly over LDAP or by editing dse.ldif? An internal call to set the value will wipe out their settings. </blockquote> <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite">Also, there is a reference to frational attr in the slapi_set_plugin_default_config() code. </blockquote> Thanks! I removed it... :p <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite"> I think slapi_set_plugin_default_config() will leak the pblock used for the initial search if rc != LDAP_SUCCESS or entries is NULL or empty. </blockquote> I think it's taken care here.... Valgrind shows no leak in slapi_set_plugin_default_config in both cases entries exist or don't exist. 2911 } else { /* cn=plugin default config does not exist. Let's add it. */ 2912 LDAPMod *mods[3]; 2913 LDAPMod mod[2]; 2914 const char *objectclass[3]; 2915 char *attrlist[2]; 2916 2917 slapi_free_search_results_internal(&pb); 2918 pblock_done(&pb); <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite"> slapi_get_plugin_default_config() - since you are assuming that the values of the requested attribute are all null terminated strings (since you are using slapi_value_get_string()), you could just use slapi_entry_attr_get_charray() to get all of the values as a char **. </blockquote> Thanks! This is what I needed... <blockquote cite="mid:4A955F19.8080708@redhat.com" type="cite"> Why does slapi_set_plugin_default_config() take a char * argument i.e. setting an attribute with a single string value, but slapi_get_plugin_default_config() returns multiple values from a single attribute? I'm not sure what the code in usn_start() is doing. Is nsds5ReplicatedAttributeList multi-valued? If so, each char * in char **old_frac_attrs will be a full attribute list description "(objectclass=*) $ EXCLUDE ...". Then I'm not sure what the code that adds the entryusn attribute to the default excluded list is doing - will it add entryusn to only the first value of nsds5ReplicatedAttributeList? And wipe out the other values? I think that's what will happen, since there is a break in the for loop if token == NULL - it will skip old_frac_attrs[1] etc. Also the call to charray_add - the token argument is not a malloced string, so that will cause problems when slapi_ch_array_free() is called. </blockquote> I cleaned up the code. It just adds the attribute value as follows. /* add nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE entryusn * to cn=plugin default config,cn=config */ rc = slapi_set_plugin_default_config("nsds5ReplicatedAttributeList", "(objectclass=*) $ EXCLUDE entryusn"); Sorry about the previous code I wiped out. I wanted to do this... Let's assume we found exisiting nsds5ReplicatedAttributeList lines: nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type0 nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type1 type 2 I wanted to convert them in to one line: nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type0 type1 type 2 entryusn Instead, I changed to just add nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE entryusn to the entry. So, it'll end up like this: nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type0 nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type1 type 2 nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE entryusn </body> </html>