[prev in list] [next in list] [prev in thread] [next in thread]
List: fedora-directory-commits
Subject: =?utf-8?q?=5B389-commits=5D?= [389-ds-base] branch 389-ds-base-1.4.0 updated: Ticket 50260 - Invalid
From: pagure () pagure ! io
Date: 2019-03-18 16:45:51
Message-ID: 20190318164551.28418.14271 () pagure01 ! fedoraproject ! org
[Download RAW message or body]
This is an automated email from the git hooks/post-receive script.
mreynolds pushed a commit to branch 389-ds-base-1.4.0
in repository 389-ds-base.
The following commit(s) were added to refs/heads/389-ds-base-1.4.0 by this push:
new 67aaee4 Ticket 50260 - Invalid cache flushing improvements
67aaee4 is described below
commit 67aaee4708765945f5935ed12ddfbeb973fd1f5a
Author: Mark Reynolds <mreynolds@redhat.com>
AuthorDate: Sun Mar 17 13:09:07 2019 -0400
Ticket 50260 - Invalid cache flushing improvements
Description: The original version of the fix only checked if backend
transaction "post" operation plugins failed, but it did
not check for errors from the backend transaction "pre"
operation plugin. To address this we flush invalid
entries whenever any error occurs.
We were also not flushing invalid cache entries when
modrdn errors occurred. Modrdns only make changes to
the DN hashtable inside the entry cache, but we were only
checking the ID hashtable. So we also need to check the
DN hashtable in the entry cache for invalid entries.
https://pagure.io/389-ds-base/issue/50260
Reviewed by: firstyear & tbordaz(Thanks!!)
(cherry picked from commit 33fbced25277b88695bfba7262e606380e9d891f)
---
dirsrvtests/tests/suites/betxns/betxn_test.py | 51 ++++++++++++---------------
ldap/servers/slapd/back-ldbm/cache.c | 42 ++++++++++++++++++++--
ldap/servers/slapd/back-ldbm/ldbm_add.c | 9 +++--
ldap/servers/slapd/back-ldbm/ldbm_delete.c | 11 +++---
ldap/servers/slapd/back-ldbm/ldbm_modify.c | 10 +++---
ldap/servers/slapd/back-ldbm/ldbm_modrdn.c | 10 +++---
6 files changed, 82 insertions(+), 51 deletions(-)
diff --git a/dirsrvtests/tests/suites/betxns/betxn_test.py \
b/dirsrvtests/tests/suites/betxns/betxn_test.py index 2aaddde..94e6081 100644
--- a/dirsrvtests/tests/suites/betxns/betxn_test.py
+++ b/dirsrvtests/tests/suites/betxns/betxn_test.py
@@ -57,24 +57,20 @@ def test_betxt_7bit(topology_st):
user = users.create(properties=TEST_USER_PROPERTIES)
# Attempt a modrdn, this should fail
-
- try:
+ with pytest.raises(ldap.LDAPError):
user.rename(BAD_RDN)
- log.fatal('test_betxt_7bit: Modrdn operation incorrectly succeeded')
- assert False
- except ldap.LDAPError as e:
- log.info('Modrdn failed as expected: error %s' % str(e))
# Make sure the operation did not succeed, attempt to search for the new RDN
+ with pytest.raises(ldap.LDAPError):
+ users.get(u'Fu\u00c4\u00e8')
+ # Make sure original entry is present
user_check = users.get("testuser")
-
assert user_check.dn.lower() == user.dn.lower()
- #
# Cleanup - remove the user
- #
user.delete()
+
log.info('test_betxt_7bit: PASSED')
@@ -153,17 +149,11 @@ def test_betxn_memberof(topology_st):
memberof = MemberOfPlugin(topology_st.standalone)
memberof.enable()
memberof.set_autoaddoc('referral')
- # memberof.add_groupattr('member') # This is already the default.
topology_st.standalone.restart()
groups = Groups(topology_st.standalone, DEFAULT_SUFFIX)
- group1 = groups.create(properties={
- 'cn': 'group1',
- })
-
- group2 = groups.create(properties={
- 'cn': 'group2',
- })
+ group1 = groups.create(properties={'cn': 'group1'})
+ group2 = groups.create(properties={'cn': 'group2'})
# We may need to mod groups to not have nsMemberOf ... ?
if not ds_is_older('1.3.7'):
@@ -171,21 +161,18 @@ def test_betxn_memberof(topology_st):
group2.remove('objectClass', 'nsMemberOf')
# Add group2 to group1 - it should fail with objectclass violation
- try:
+ with pytest.raises(ldap.OBJECT_CLASS_VIOLATION):
group1.add_member(group2.dn)
- log.fatal('test_betxn_memberof: Group2 was incorrectly allowed to be added \
to group1')
- assert False
- except ldap.LDAPError as e:
- log.info('test_betxn_memberof: Group2 was correctly rejected (mod add): \
error: ' + str(e))
- #
+ # verify entry cache reflects the current/correct state of group1
+ assert not group1.is_member(group2.dn)
+
# Done
- #
log.info('test_betxn_memberof: PASSED')
def test_betxn_modrdn_memberof_cache_corruption(topology_st):
- """Test modrdn operations and memberOf
+ """Test modrdn operations and memberOf be txn post op failures
:id: 70d0b96e-b693-4bf7-bbf5-102a66ac5994
@@ -234,9 +221,7 @@ def test_betxn_modrdn_memberof_cache_corruption(topology_st):
with pytest.raises(ldap.OBJECT_CLASS_VIOLATION):
group.rename('cn=group_to_people', newsuperior=peoplebase)
- #
# Done
- #
log.info('test_betxn_modrdn_memberof: PASSED')
@@ -311,15 +296,23 @@ def test_ri_and_mep_cache_corruption(topology_st):
log.fatal("MEP group was not created for the user")
assert False
+ # Test MEP be txn pre op failure does not corrupt entry cache
+ # Should get the same exception for both rename attempts
+ with pytest.raises(ldap.UNWILLING_TO_PERFORM):
+ mep_group.rename("cn=modrdn group")
+
+ with pytest.raises(ldap.UNWILLING_TO_PERFORM):
+ mep_group.rename("cn=modrdn group")
+
# Mess with MEP so it fails
mep_plugin.disable()
mep_group.delete()
mep_plugin.enable()
- # Add another group for verify entry cache is not corrupted
+ # Add another group to verify entry cache is not corrupted
test_group = groups.create(properties={'cn': 'test_group'})
- # Delete user, should fail, and user should still be a member
+ # Delete user, should fail in MEP be txn post op, and user should still be a \
member with pytest.raises(ldap.NO_SUCH_OBJECT):
user.delete()
diff --git a/ldap/servers/slapd/back-ldbm/cache.c \
b/ldap/servers/slapd/back-ldbm/cache.c index ba9d26f..a03cdaa 100644
--- a/ldap/servers/slapd/back-ldbm/cache.c
+++ b/ldap/servers/slapd/back-ldbm/cache.c
@@ -517,7 +517,8 @@ flush_remove_entry(struct timespec *entry_time, struct timespec \
*start_time) /*
* Flush all the cache entries that were added after the "start time"
* This is called when a backend transaction plugin fails, and we need
- * to remove all the possible invalid entries in the cache.
+ * to remove all the possible invalid entries in the cache. We need
+ * to check both the ID and DN hashtables when checking the entry cache.
*
* If the ref count is 0, we can straight up remove it from the cache, but
* if the ref count is greater than 1, then the entry is currently in use.
@@ -528,8 +529,8 @@ flush_remove_entry(struct timespec *entry_time, struct timespec \
*start_time) static void
flush_hash(struct cache *cache, struct timespec *start_time, int32_t type)
{
+ Hashtable *ht = cache->c_idtable; /* start with the ID table as it's in both \
ENTRY and DN caches */ void *e, *laste = NULL;
- Hashtable *ht = cache->c_idtable;
cache_lock(cache);
@@ -570,6 +571,43 @@ flush_hash(struct cache *cache, struct timespec *start_time, \
int32_t type) }
}
+ if (type == ENTRY_CACHE) {
+ /* Also check the DN hashtable */
+ ht = cache->c_dntable;
+
+ for (size_t i = 0; i < ht->size; i++) {
+ e = ht->slot[i];
+ while (e) {
+ struct backcommon *entry = (struct backcommon *)e;
+ uint64_t remove_it = 0;
+ if (flush_remove_entry(&entry->ep_create_time, start_time)) {
+ /* Mark the entry to be removed */
+ slapi_log_err(SLAPI_LOG_CACHE, "flush_hash", "[ENTRY CACHE] \
Removing entry id (%d)\n", + entry->ep_id);
+ remove_it = 1;
+ }
+ laste = e;
+ e = HASH_NEXT(ht, e);
+
+ if (remove_it) {
+ /* since we have the cache lock we know we can trust refcnt */
+ entry->ep_state |= ENTRY_STATE_INVALID;
+ if (entry->ep_refcnt == 0) {
+ entry->ep_refcnt++;
+ lru_delete(cache, laste);
+ entrycache_remove_int(cache, laste);
+ entrycache_return(cache, (struct backentry **)&laste);
+ } else {
+ /* Entry flagged for removal */
+ slapi_log_err(SLAPI_LOG_CACHE, "flush_hash",
+ "[ENTRY CACHE] Flagging entry to be removed later: \
id (%d) refcnt: %d\n", + entry->ep_id, \
entry->ep_refcnt); + }
+ }
+ }
+ }
+ }
+
cache_unlock(cache);
}
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c \
b/ldap/servers/slapd/back-ldbm/ldbm_add.c index 8c0439c..0d82ae9 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_add.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c
@@ -1221,11 +1221,6 @@ ldbm_back_add(Slapi_PBlock *pb)
slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? \
&ldap_result_code : &retval); }
slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
-
- /* Revert the caches if this is the parent operation */
- if (parent_op) {
- revert_cache(inst, &parent_time);
- }
goto error_return;
}
@@ -1253,6 +1248,10 @@ ldbm_back_add(Slapi_PBlock *pb)
goto common_return;
error_return:
+ /* Revert the caches if this is the parent operation */
+ if (parent_op) {
+ revert_cache(inst, &parent_time);
+ }
if (addingentry_id_assigned) {
next_id_return(be, addingentry->ep_id);
}
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c \
b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index 98b3d82..e9f3e32 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c
@@ -1279,11 +1279,6 @@ replace_entry:
slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, &retval);
}
slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
-
- /* Revert the caches if this is the parent operation */
- if (parent_op) {
- revert_cache(inst, &parent_time);
- }
goto error_return;
}
if (parent_found) {
@@ -1370,6 +1365,11 @@ commit_return:
goto common_return;
error_return:
+ /* Revert the caches if this is the parent operation */
+ if (parent_op) {
+ revert_cache(inst, &parent_time);
+ }
+
if (tombstone) {
if (cache_is_in_cache(&inst->inst_cache, tombstone)) {
tomb_ep_id = tombstone->ep_id; /* Otherwise, tombstone might have been \
freed. */ @@ -1388,6 +1388,7 @@ error_return:
CACHE_RETURN(&inst->inst_cache, &tombstone);
tombstone = NULL;
}
+
if (retval == DB_RUNRECOVERY) {
dblayer_remember_disk_filled(li);
ldbm_nasty("ldbm_back_delete", "Delete", 79, retval);
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modify.c \
b/ldap/servers/slapd/back-ldbm/ldbm_modify.c index b90b3e0..b0c477e 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_modify.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_modify.c
@@ -873,11 +873,6 @@ ldbm_back_modify(Slapi_PBlock *pb)
slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? \
&ldap_result_code : &retval); }
slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
-
- /* Revert the caches if this is the parent operation */
- if (parent_op) {
- revert_cache(inst, &parent_time);
- }
goto error_return;
}
retval = plugin_call_mmr_plugin_postop(pb, \
NULL,SLAPI_PLUGIN_BE_TXN_POST_MODIFY_FN); @@ -901,6 +896,11 @@ \
ldbm_back_modify(Slapi_PBlock *pb) goto common_return;
error_return:
+ /* Revert the caches if this is the parent operation */
+ if (parent_op) {
+ revert_cache(inst, &parent_time);
+ }
+
if (postentry != NULL) {
slapi_entry_free(postentry);
postentry = NULL;
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c \
b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c index 73e50eb..65610d6 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
@@ -1217,11 +1217,6 @@ ldbm_back_modrdn(Slapi_PBlock *pb)
slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? \
&ldap_result_code : &retval); }
slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
-
- /* Revert the caches if this is the parent operation */
- if (parent_op) {
- revert_cache(inst, &parent_time);
- }
goto error_return;
}
retval = plugin_call_mmr_plugin_postop(pb, \
NULL,SLAPI_PLUGIN_BE_TXN_POST_MODRDN_FN); @@ -1290,6 +1285,11 @@ \
ldbm_back_modrdn(Slapi_PBlock *pb) goto common_return;
error_return:
+ /* Revert the caches if this is the parent operation */
+ if (parent_op) {
+ revert_cache(inst, &parent_time);
+ }
+
/* result already sent above - just free stuff */
if (postentry) {
slapi_entry_free(postentry);
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
_______________________________________________
389-commits mailing list -- 389-commits@lists.fedoraproject.org
To unsubscribe send an email to 389-commits-leave@lists.fedoraproject.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/389-commits@lists.fedoraproject.org
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic