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(a)redhat.com
https://www.redhat.com/mailman/listinfo/fedora-directory-devel