<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
On 01/19/2010 02:19 PM, Nathan Kinder wrote:
<blockquote cite="mid:4B562FFB.2020104@redhat.com" type="cite">
<pre wrap="">On 01/19/2010 01:50 PM, Rich Megginson wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Noriko Hosoi wrote:
</pre>
<blockquote type="cite">
<pre wrap=""> Allow modrdn to move subtree and rename non-leaf node
This patch includes
- replacing the entrydn index with the entryrdn index
- replacing a full DN in each entry in the DB with an RDN
- extending Slapi_Entry, entry2str, and str2entry to absorb the
changes made on the entry
- adding DN/RDN helper functions
- adding DN cache
- adding a utility and a migration script to convert the DN format
database to the RDN format
- extending a database dump utility dbscan to support the entryrdn
In addition to the above, compile warnings and memory leaks found
in testing the new feature are fixed.
For more details, see the feature design document at:
<a class="moz-txt-link-freetext" href="http://directory.fedoraproject.org/wiki/Subtree_Rename">http://directory.fedoraproject.org/wiki/Subtree_Rename</a>
The patch is too big to attach to the email. It is located here:
<a class="moz-txt-link-freetext" href="http://nhosoi.fedorapeople.org/0001-Allow-modrdn-to-move-subtree-and-rename-non-leaf-nod.patch">http://nhosoi.fedorapeople.org/0001-Allow-modrdn-to-move-subtree-and-rename-non-leaf-nod.patch</a>
</pre>
</blockquote>
<pre wrap="">Looks good. Just a few questions.
Note that DB_BUFFER_SMALL was new for db 4.3 - I suggest using the code
in cl5_clcache.c that does this:
/* newer bdb uses DB_BUFFER_SMALL instead of ENOMEM as the
error return if the given buffer in which to load a
key or value is too small - if it is not defined, define
it here to ENOMEM
*/
#ifndef DB_BUFFER_SMALL
#define DB_BUFFER_SMALL ENOMEM
#endif
</pre>
</blockquote>
</blockquote>
I think all the files referring DB_BUFFER_SMALL has the define...<br>
$ find . -name "*.[ch]" | xargs egrep DB_BUFFER_SMALL<br>
./plugins/replication/cl5_clcache.c:/* newer bdb uses DB_BUFFER_SMALL
instead of ENOMEM as the<br>
./plugins/replication/cl5_clcache.c:#ifndef DB_BUFFER_SMALL<br>
./plugins/replication/cl5_clcache.c:#define DB_BUFFER_SMALL ENOMEM<br>
./plugins/replication/cl5_clcache.c: if ( 0 == rc ||
DB_BUFFER_SMALL == rc )<br>
./plugins/replication/cl5_clcache.c: if ( DB_BUFFER_SMALL == rc ) {<br>
./plugins/replication/cl5_clcache.c: case DB_BUFFER_SMALL:<br>
<br>
./slapd/tools/dbscan.c:#ifndef DB_BUFFER_SMALL<br>
./slapd/tools/dbscan.c:#define DB_BUFFER_SMALL ENOMEM<br>
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {<br>
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {<br>
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {<br>
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {<br>
<br>
./slapd/back-ldbm/idl_new.c: if (ret == DB_BUFFER_SMALL) {<br>
./slapd/back-ldbm/id2entry.c: if ( (DB_BUFFER_SMALL == *err)
&& (data.dptr == NULL) )<br>
./slapd/back-ldbm/ldbm_entryrdn.c: } else if (DB_BUFFER_SMALL ==
rc) {<br>
./slapd/back-ldbm/ldbm_entryrdn.c: if (DB_BUFFER_SMALL == rc) {<br>
<br>
./slapd/back-ldbm/back-ldbm.h:#ifndef DB_BUFFER_SMALL<br>
./slapd/back-ldbm/back-ldbm.h:#define DB_BUFFER_SMALL ENOMEM<br>
<br>
$ egrep back-ldbm.h idl_new.c id2entry.c ldbm_entryrdn.c<br>
idl_new.c:#include "back-ldbm.h"<br>
id2entry.c:#include "back-ldbm.h"<br>
ldbm_entryrdn.c:#include "back-ldbm.h"<br>
<br>
<blockquote cite="mid:4B562FFB.2020104@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">
in id2entry.c line 295 - you have const char *dn - then you have to cast
away the const almost everywhere it is used - just make it a char *?
does it need to be const?
</pre>
</blockquote>
</blockquote>
I guess not. :) I'm going to change the type...<br>
<blockquote cite="mid:4B562FFB.2020104@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">
idl_new_delete_key - why the changes there?
</pre>
</blockquote>
</blockquote>
Calling cursor->c_get with DB_SET and DB_GET_BOTH were redundant.
Since this is delete, we have both the key and value. All we need to
do should be setting the cursor at the position and delete the
key-value pair.<br>
<br>
This is the snippet of cursor get description:<br>
<blockquote><code class="literal">DB_GET_BOTH</code>
<p> Move the cursor to the specified key/data pair of the database.
The cursor is positioned to a key/data pair if both the key and data
match the values provided on the key and data parameters. </p>
<p> In all other ways, this flag is identical to the <a class="link"
href="http://www.oracle.com/technology/documentation/berkeley-db/db/api_reference/C/dbcget.html#dbcget_DB_SET">DB_SET</a>
flag. </p>
</blockquote>
It was found by valgrind reporting a memory leak. The DBT key has
DB_DBT_MALLOC flag set underneath. Once cursor->c_get is called
with DB_SET, the memory is allocated and filled with the key. (Freeing
the key->data fixes the leak. But I believe we don't have to call
the cursor->c_get twice here.)<br>
<br>
<blockquote cite="mid:4B562FFB.2020104@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">
modutil.c line 632 - why the change there?
</pre>
</blockquote>
</blockquote>
Well, I thought slapi_mod_done wants to do 0 pudding the whole
Slapi_Mods not just the first pointer... Do you think the sizeof(smod)
is intentional?<br>
typedef struct slapi_mod<br>
{<br>
LDAPMod *mod;<br>
int num_elements;<br>
int num_values; <br>
int iterator;<br>
int free_mod; /* flag to inidicate that the mod was dynamically
allocated and needs to be freed */<br>
}slapi_mod;<br>
<br>
622 void<br>
623 slapi_mod_done(Slapi_Mod *smod)<br>
624 {<br>
625 PR_ASSERT(smod!=NULL);<br>
626 if(smod->free_mod)<br>
627 {<br>
628 ber_bvecfree(smod->mod->mod_bvalues);<br>
629 slapi_ch_free((void**)&(smod->mod->mod_type));<br>
630 slapi_ch_free((void**)&(smod->mod));<br>
631 }<br>
632 memset (smod, 0, sizeof(*smod));<br>
633 }<br>
<br>
@@ -629,7 +629,7 @@ slapi_mod_done(Slapi_Mod *smod)<br>
slapi_ch_free((void**)&(smod->mod->mod_type));<br>
slapi_ch_free((void**)&(smod->mod));<br>
}<br>
- memset (smod, 0, sizeof(smod));<br>
+ memset (smod, 0, sizeof(*smod));<br>
}<br>
<br>
<blockquote cite="mid:4B562FFB.2020104@redhat.com" type="cite">
<pre wrap="">I have one other question to tack on from my review.
In Makefile.am, you have:
-dbscan_bin_CPPFLAGS = @db_inc@ $(AM_CPPFLAGS)
-dbscan_bin_LDADD = $(NSPR_LINK) $(DB_LINK)
+dbscan_bin_CPPFLAGS = @db_inc@ @nspr_inc@ $(AM_CPPFLAGS)
+dbscan_bin_LDADD = $(NSPR_LINK) $(DB_LINK) $(NSPR_LINK)
Why is $NSPR_LINK listed twice?
</pre>
</blockquote>
Oops. My mistake. I wanted to add @nspr_inc@to dbscan_bin_CPPFLAGS.
I mistakenly assumed NSPR is not linked. :p<br>
<br>
Thank you so much, Rich and Nathan!!<br>
--noriko<br>
</body>
</html>