[389-devel] Please review: Allow modrdn to move subtree and rename non-leaf node
Noriko Hosoi
nhosoi at redhat.com
Tue Jan 19 22:53:26 UTC 2010
On 01/19/2010 02:19 PM, Nathan Kinder wrote:
> On 01/19/2010 01:50 PM, Rich Megginson wrote:
>
>> Noriko Hosoi wrote:
>>
>>
>>> 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:
>>>
>>> http://directory.fedoraproject.org/wiki/Subtree_Rename
>>>
>>> The patch is too big to attach to the email. It is located here:
>>>
>>> http://nhosoi.fedorapeople.org/0001-Allow-modrdn-to-move-subtree-and-rename-non-leaf-nod.patch
>>>
>>>
>>>
>> 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
>>
I think all the files referring DB_BUFFER_SMALL has the define...
$ find . -name "*.[ch]" | xargs egrep DB_BUFFER_SMALL
./plugins/replication/cl5_clcache.c:/* newer bdb uses DB_BUFFER_SMALL
instead of ENOMEM as the
./plugins/replication/cl5_clcache.c:#ifndef DB_BUFFER_SMALL
./plugins/replication/cl5_clcache.c:#define DB_BUFFER_SMALL ENOMEM
./plugins/replication/cl5_clcache.c: if ( 0 == rc ||
DB_BUFFER_SMALL == rc )
./plugins/replication/cl5_clcache.c: if ( DB_BUFFER_SMALL == rc ) {
./plugins/replication/cl5_clcache.c: case DB_BUFFER_SMALL:
./slapd/tools/dbscan.c:#ifndef DB_BUFFER_SMALL
./slapd/tools/dbscan.c:#define DB_BUFFER_SMALL ENOMEM
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {
./slapd/tools/dbscan.c: if (DB_BUFFER_SMALL == rc) {
./slapd/back-ldbm/idl_new.c: if (ret == DB_BUFFER_SMALL) {
./slapd/back-ldbm/id2entry.c: if ( (DB_BUFFER_SMALL == *err) &&
(data.dptr == NULL) )
./slapd/back-ldbm/ldbm_entryrdn.c: } else if (DB_BUFFER_SMALL ==
rc) {
./slapd/back-ldbm/ldbm_entryrdn.c: if (DB_BUFFER_SMALL == rc) {
./slapd/back-ldbm/back-ldbm.h:#ifndef DB_BUFFER_SMALL
./slapd/back-ldbm/back-ldbm.h:#define DB_BUFFER_SMALL ENOMEM
$ egrep back-ldbm.h idl_new.c id2entry.c ldbm_entryrdn.c
idl_new.c:#include "back-ldbm.h"
id2entry.c:#include "back-ldbm.h"
ldbm_entryrdn.c:#include "back-ldbm.h"
>>
>> 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?
>>
I guess not. :) I'm going to change the type...
>> idl_new_delete_key - why the changes there?
>>
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.
This is the snippet of cursor get description:
|DB_GET_BOTH|
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.
In all other ways, this flag is identical to the DB_SET
<http://www.oracle.com/technology/documentation/berkeley-db/db/api_reference/C/dbcget.html#dbcget_DB_SET>
flag.
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.)
>> modutil.c line 632 - why the change there?
>>
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?
typedef struct slapi_mod
{
LDAPMod *mod;
int num_elements;
int num_values;
int iterator;
int free_mod; /* flag to inidicate that the mod was dynamically
allocated and needs to be freed */
}slapi_mod;
622 void
623 slapi_mod_done(Slapi_Mod *smod)
624 {
625 PR_ASSERT(smod!=NULL);
626 if(smod->free_mod)
627 {
628 ber_bvecfree(smod->mod->mod_bvalues);
629 slapi_ch_free((void**)&(smod->mod->mod_type));
630 slapi_ch_free((void**)&(smod->mod));
631 }
632 memset (smod, 0, sizeof(*smod));
633 }
@@ -629,7 +629,7 @@ slapi_mod_done(Slapi_Mod *smod)
slapi_ch_free((void**)&(smod->mod->mod_type));
slapi_ch_free((void**)&(smod->mod));
}
- memset (smod, 0, sizeof(smod));
+ memset (smod, 0, sizeof(*smod));
}
> 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?
>
Oops. My mistake. I wanted to add @nspr_inc at to dbscan_bin_CPPFLAGS. I
mistakenly assumed NSPR is not linked. :p
Thank you so much, Rich and Nathan!!
--noriko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.fedoraproject.org/pipermail/389-devel/attachments/20100119/126db36e/attachment.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6646 bytes
Desc: S/MIME Cryptographic Signature
Url : http://lists.fedoraproject.org/pipermail/389-devel/attachments/20100119/126db36e/attachment.bin
More information about the 389-devel
mailing list