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

[Rich Megginson]

Noriko Hosoi wrote:
> 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
Ok - will the replication code be able to handle multiple values of 
nsds5ReplicatedAttributeList?
>
> ------------------------------------------------------------------------
>
> --
> 389-devel mailing list
> 389-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/fedora-directory-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3258 bytes
Desc: S/MIME Cryptographic Signature
Url : http://lists.fedoraproject.org/pipermail/389-devel/attachments/20090828/114be2ac/attachment.bin