Noriko Hosoi wrote: > 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" ok > >>> 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.) OK - good > >>> 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)); > } Ok > >> 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 > ------------------------------------------------------------------------ > > -- > 389-devel mailing list > 389-devel@xxxxxxxxxxxxxxxxxxxxxxx > https://admin.fedoraproject.org/mailman/listinfo/389-devel -- 389-devel mailing list 389-devel@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/389-devel
