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
------------------------------------------------------------------------
--
389-devel mailing list
389-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-directory-devel
--
389-devel mailing list
389-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-directory-devel
|
--
389-devel mailing list
389-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-directory-devel
