[389-devel] Please review (revised): Plugin Default Config Entry

[Noriko Hosoi]

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:
[...]
> 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).
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);
     }

>
> +            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.
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);
>
> 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. 
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.
> 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. 

> Also, there is a reference to frational attr in the 
> slapi_set_plugin_default_config() code.
Thanks!  I removed it... :p
>
> 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.
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);*

>
> 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 **.
Thanks!  This is what I needed...
>
> 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.
>
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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/fedora-directory-devel/attachments/20090826/054a272c/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Plugin-Default-Config-Entry.patch
Type: text/x-patch
Size: 22232 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/fedora-directory-devel/attachments/20090826/054a272c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3250 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/fedora-directory-devel/attachments/20090826/054a272c/attachment.p7s>