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-...
Thanks, --noriko
Hi Noriko,
Allow modrdn to move subtree and rename non-leaf node
This patch includes ...
I've read your design and implementation guide. It is very detailed and discusses a lot of questions in a rather comprehensive way. A lot of uncertainties i had in the beginning of the document were resolved at the end (for example, the "ancestorid investigation").
Here are some questions or remarks that i still do have after this reading: Citation: ------------------- This function entryrdn_index_read finds out the entry_id of the given DN sdn. It starts from the suffix in the entryrdn index following the child links to the bottom. If it is successful, the entry_is of the target node is set to the address that the argument id points to. -------------------
the previously used "entrydn" index and the corresponding function needed just one step(?) to find out the entry id from it's dn. Several steps are now needed with "entryrdn" index. You talk about an expensive entry_id - > dn conversion and you introduce the dn cache to take care of it. However you don't mention the expensive conversion dn -> entry_id (that was the role of entrydn.db4). As far as i understand the standard entry cache should help this conversion? And maybe this type of conversion is much less frequent than entry_id - > dn?
It would be interesting to have an estimation of the performance loss (or gain?) due to this new entryrdn index for typical read/search operations - 3 to 5 levels of rdn in DIT... I think it can be done with ldclt in a similar way you've done it for ancestorid index.
One can extend your investigation of ancestorid index further: while we use entryrdn index is there still a need to maintain parentid (numsubordinates, ancestorid)? One of the points is that db2ldif needs the parentid attribute when the indexes are corrupted, but does it really need the _index_ parentid? I think db2ldif reads the entries directly from id2entry?
The questions concerning the interaction with other components of the server : some of server plug-ins assume that modrdn changes only the rdn part. These plug-ins will be broken. One of them is the referential integrity plug-in - it takes for granted that only the rdn part changes (update_integrity(argv, dn, newrdn, logChanges)). Other possible candidates are memberOf, linked attributes plug-ins, maybe some others. entryusn?
Are there any repercussions on replication protocol and mechanism?
Thank you for your efforts !
Hi Andrey,
Thanks so much for reading through the design doc! My responses are in line...
On 01/14/2010 07:45 AM, Andrey Ivanov wrote:
Hi Noriko,
Allow modrdn to move subtree and rename non-leaf node
This patch includes
...
I've read your design and implementation guide. It is very detailed and discusses a lot of questions in a rather comprehensive way. A lot of uncertainties i had in the beginning of the document were resolved at the end (for example, the "ancestorid investigation").
Here are some questions or remarks that i still do have after this reading: Citation:
This function entryrdn_index_read finds out the entry_id of the given DN sdn. It starts from the suffix in the entryrdn index following the child links to the bottom. If it is successful, the entry_is of the target node is set to the address that the argument id points to.
the previously used "entrydn" index and the corresponding function needed just one step(?) to find out the entry id from it's dn. Several steps are now needed with "entryrdn" index.
That's right.
You talk about an expensive entry_id -> dn conversion and you introduce the dn cache to take care of it. However you don't mention the expensive conversion dn -> entry_id (that was the role of entrydn.db4). As far as i understand the standard entry cache should help this conversion? And maybe this type of conversion is much less frequent than entry_id -> dn?
Yes and yes. That's my observation. If you have a dn (operations from clients always have it), the matched entry is retrieved from the database first, then put in the entry cache. As long as the entry cache holds the entry, dn -> entry_id will not be needed. On the other hand, entry_id -> dn is more engaged especially when you need to scan the primary db file id2entry.db4. That occurs on the unindexed search. Unindexed search is generally slow, but without dn cache, it'd be even worse...
It would be interesting to have an estimation of the performance loss (or gain?) due to this new entryrdn index for typical read/search operations - 3 to 5 levels of rdn in DIT... I think it can be done with ldclt in a similar way you've done it for ancestorid index.
A very good idea. I'm going to run the tests and push the results to the wiki.
One can extend your investigation of ancestorid index further: while we use entryrdn index is there still a need to maintain parentid (numsubordinates, ancestorid)? One of the points is that db2ldif needs the parentid attribute when the indexes are corrupted, but does it really need the _index_ parentid? I think db2ldif reads the entries directly from id2entry?
Another good point. I'm adding them to the ToDo list. As you suggested, we may be able to eliminate the parentid index. Numsubordinates may not be (just my initial guess, though...)
The questions concerning the interaction with other components of the server : some of server plug-ins assume that modrdn changes only the rdn part. These plug-ins will be broken. One of them is the referential integrity plug-in - it takes for granted that only the rdn part changes (update_integrity(argv, dn, newrdn, logChanges)). Other possible candidates are memberOf, linked attributes plug-ins, maybe some others. entryusn?
Are there any repercussions on replication protocol and mechanism?
I ran the test suite that covers these functionalities. So far, I haven't seen any breakage. But I might be missing something important, so your reviews would be greatly appreciated.
BTW, I've updated the patch at http://nhosoi.fedorapeople.org/0001-Allow-modrdn-to-move-subtree-and-rename-... . It's sync'ed with the latest commit and it mentions about another new API slapi_dn_syntax_check. - slapi_dn_syntax_check by nkinder@redhat.com is added to check the dn befor modify operations
Thanks, --noriko
Hi,
You talk about an expensive entry_id -> dn conversion and you introduce the dn cache to take care of it. However you don't mention the expensive conversion dn -> entry_id (that was the role of entrydn.db4). As far as i understand the standard entry cache should help this conversion? And maybe this type of conversion is much less frequent than entry_id -> dn?
Yes and yes. That's my observation. If you have a dn (operations from clients always have it), the matched entry is retrieved from the database first, then put in the entry cache. As long as the entry cache holds the entry, dn -> entry_id will not be needed. On the other hand, entry_id -> dn is more engaged especially when you need to scan the primary db file id2entry.db4. That occurs on the unindexed search. Unindexed search is generally slow, but without dn cache, it'd be even worse...
Ok. That's what I've thought. Howard also confirms and explains this in his mail.
some of server plug-ins assume that modrdn changes only the rdn part. These plug-ins will be broken. One of them is the referential integrity plug-in - it takes for granted that only the rdn part changes (update_integrity(argv, dn, newrdn, logChanges)). Other possible candidates are memberOf, linked attributes plug-ins, maybe some others. entryusn?
Are there any repercussions on replication protocol and mechanism?
I ran the test suite that covers these functionalities. So far, I haven't seen any breakage. But I might be missing something important, so your reviews would be greatly appreciated.
I'm pretty sure the referential integrity plug-in will not work for modrdn operations with a new superior. Looking more thoroughly through the code ( ldap / servers / plugins / referint / referint.c) confirms my suspicion that new rdn superior is not taken into account. The function referint_postop_modrdn extracts from the parameter block only SLAPI_MODRDN_TARGET and SLAPI_MODRDN_NEWRDN, it does not extract SLAPI_MODRDN_NEWSUPERIOR neither passes it further down the utility functions - update_integrity(argv, dn, newrdn, logChanges) and writeintegritylog(argv[1],dn, newrdn). The same applies to the delayed referint operations (the plug-in writes to the special integrity log file only the old DN and the new RDN, but never the new superior : writeintegritylog(argv[1],dn, newrdn);)
@+
I ran the test suite that covers these functionalities. So far, I haven't seen any breakage. But I might be missing something important, so your reviews would be greatly appreciated.
I'm pretty sure the referential integrity plug-in will not work for modrdn operations with a new superior. Looking more thoroughly through the code ( ldap / servers / plugins / referint / referint.c) confirms my suspicion that new rdn superior is not taken into account. The function referint_postop_modrdn extracts from the parameter block only SLAPI_MODRDN_TARGET and SLAPI_MODRDN_NEWRDN, it does not extract SLAPI_MODRDN_NEWSUPERIOR neither passes it further down the utility functions - update_integrity(argv, dn, newrdn, logChanges) and writeintegritylog(argv[1],dn, newrdn). The same applies to the delayed referint operations (the plug-in writes to the special integrity log file only the old DN and the new RDN, but never the new superior : writeintegritylog(argv[1],dn, newrdn);)
Another thought on the subject of referential integrity plug-in - in the previous mail i have only mentioned one-entry renames with a new superior. The things get even worse when we rename a whole non-empty sub-tree. It means that the referential integrity plug-in should change to the new DNs all the references to all the entries of the whole sub-tree, not only for one entry.
And what if we rename a sub-tree containing both the referenced entry and the entry referencing the first one's DN in one of its "integrity" attributes? It actually means that we need at first make the rename and then all the searches and replacements. Though it seems it's already the case as it's a post-op(?) plug-in...
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-...
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
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?
idl_new_delete_key - why the changes there?
modutil.c line 632 - why the change there?
Thanks, --noriko
-- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
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-...
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
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?
idl_new_delete_key - why the changes there?
modutil.c line 632 - why the change there?
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?
Thanks, --noriko
-- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
-- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
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-...
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@to dbscan_bin_CPPFLAGS. I mistakenly assumed NSPR is not linked. :p
Thank you so much, Rich and Nathan!! --noriko
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-...
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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Thanks to Andrey, Rich, and Nathan who reviewed and commented on the patch.
The change has been pushed to master.
I also put the latest patch here: http://nhosoi.fedorapeople.org/0001-Allow-modrdn-to-move-subtree-and-rename-... ------------------------------------------------------------------------ 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 - slapi_dn_syntax_check by nkinder@redhat.com is added to check the dn before modify operations - big fix for 171338 - Enhancement: winsync modrdn not synced
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
and bugzilla at: https://bugzilla.redhat.com/show_bug.cgi?id=171338
389-devel@lists.fedoraproject.org