<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#ffffff">
On 08/28/2009 07:25 AM, Rich Megginson wrote:
<blockquote cite="mid:4A97E8C9.4020302@redhat.com" type="cite">Noriko
Hosoi wrote:
<br>
<blockquote type="cite">Thanks a lot, Rich! I revised my proposal
based upon your suggestions. Some comments are in-line...
<br>
--noriko
<br>
<br>
On 08/26/2009 09:13 AM, Rich Megginson wrote:
<br>
[...]
<br>
<blockquote type="cite">About this code:
<br>
if (new_attrs)
<br>
{
<br>
- *attrs = new_attrs;
<br>
+ charray_merge_nodup(attrs, new_attrs, 0);
<br>
+ slapi_ch_free((void **)&new_attrs);
<br>
}
<br>
<br>
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).
<br>
</blockquote>
I wanted to avoid malloc/free as much as possible, but it looks I
cannot... :( Changing it as follows:
<br>
if (new_attrs)
<br>
{
<br>
charray_merge_nodup(attrs, new_attrs, 1);
<br>
slapi_ch_array_free(new_attrs);
<br>
}
<br>
<br>
<blockquote type="cite"><br>
+ int i;
<br>
+ const char *val = slapi_value_get_string(sval);
<br>
+ for (i = slapi_attr_first_value(attr, &sval);
<br>
<br>
I don't think you should be calling slapi_value_get_string() here since
sval is not initialized yet.
<br>
</blockquote>
Thanks for finding it out! My copy & paste error... :( Changing
it as follows:
<br>
int i;
<br>
const char *val = NULL;
<br>
for (i = slapi_attr_first_value(attr, &sval);
<br>
<blockquote type="cite"><br>
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. </blockquote>
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.
<br>
<blockquote type="cite">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. </blockquote>
<br>
<blockquote type="cite">Also, there is a reference to frational
attr in the slapi_set_plugin_default_config() code.
<br>
</blockquote>
Thanks! I removed it... :p
<br>
<blockquote type="cite"><br>
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.
<br>
</blockquote>
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.
<br>
2911 } else { /* cn=plugin default config does not exist. Let's add
it. */
<br>
2912 LDAPMod *mods[3];
<br>
2913 LDAPMod mod[2];
<br>
2914 const char *objectclass[3];
<br>
2915 char *attrlist[2];
<br>
2916 *2917 slapi_free_search_results_internal(&pb);
<br>
2918 pblock_done(&pb);*
<br>
<br>
<blockquote type="cite"><br>
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 **.
<br>
</blockquote>
Thanks! This is what I needed...
<br>
<blockquote type="cite"><br>
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?
<br>
<br>
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.
<br>
<br>
</blockquote>
I cleaned up the code. It just adds the attribute value as follows.
<br>
/* add nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE
entryusn
<br>
* to cn=plugin default config,cn=config */
<br>
rc =
slapi_set_plugin_default_config("nsds5ReplicatedAttributeList",
<br>
"(objectclass=*) $ EXCLUDE
entryusn");
<br>
<br>
Sorry about the previous code I wiped out. I wanted to do this...
<br>
Let's assume we found exisiting nsds5ReplicatedAttributeList lines:
<br>
nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type0
<br>
nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type1 type 2
<br>
I wanted to convert them in to one line:
<br>
nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type0 type1
type 2 entryusn
<br>
<br>
Instead, I changed to just add nsds5ReplicatedAttributeList:
(objectclass=*) $ EXCLUDE entryusn to the entry. So, it'll end up like
this:
<br>
nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type0
<br>
nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE type1 type 2
<br>
nsds5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE entryusn
<br>
</blockquote>
Ok - will the replication code be able to handle multiple values of
nsds5ReplicatedAttributeList?
<br>
</blockquote>
Yes, it's already in the proposal... (and is tested :) )<br>
--noriko<br>
<blockquote cite="mid:4A97E8C9.4020302@redhat.com" type="cite">
<blockquote type="cite"><br>
------------------------------------------------------------------------
<br>
<br>
--
<br>
389-devel mailing list
<br>
<a class="moz-txt-link-abbreviated" href="mailto:389-devel@redhat.com">389-devel@redhat.com</a>
<br>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/fedora-directory-devel">https://www.redhat.com/mailman/listinfo/fedora-directory-devel</a>
<br>
</blockquote>
<br>
<pre wrap="">
<hr size="4" width="90%">
--
389-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:389-devel@redhat.com">389-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/fedora-directory-devel">https://www.redhat.com/mailman/listinfo/fedora-directory-devel</a>
</pre>
</blockquote>
<br>
</body>
</html>