[389-devel] Please review: Allow modrdn to move subtree and rename non-leaf node
Rich Megginson
rmeggins at redhat.com
Tue Jan 19 22:59:15 UTC 2010
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 at 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 at lists.fedoraproject.org
> https://admin.fedoraproject.org/mailman/listinfo/389-devel
More information about the 389-devel
mailing list