Re: [389-devel] Please review: Allow modrdn to move subtree and rename non-leaf node

[Noriko Hosoi <nhosoi@xxxxxxxxxx>]

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 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@to dbscan_bin_CPPFLAGS.  I mistakenly assumed NSPR is not linked. :p

Thank you so much, Rich and Nathan!!
--noriko

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

--
389-devel mailing list
389-devel@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/389-devel