[389-devel] Please review (revised): Plugin Default Config Entry
Rich Megginson
rmeggins at redhat.com
Fri Aug 28 15:25:45 UTC 2009
Noriko Hosoi wrote:
> On 08/28/2009 07:25 AM, Rich Megginson wrote:
>> 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?
> Yes, it's already in the proposal... (and is tested :) )
> --noriko
Ok. Looks good.
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>> --
>>> 389-devel mailing list
>>> 389-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/fedora-directory-devel
>>
>> ------------------------------------------------------------------------
>>
>> --
>> 389-devel mailing list
>> 389-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/fedora-directory-devel
>>
>
> ------------------------------------------------------------------------
>
> --
> 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/c2050260/attachment.bin
More information about the 389-devel
mailing list