From 233b64f26df76aa50f4b37aaf6b3804d208fdc1b Mon Sep 17 00:00:00 2001 From: Ludwig Krispenz Date: Mon, 12 Feb 2018 09:24:25 +0100 Subject: [PATCH] Ticket 49551 - v3 - correct handling of numsubordinates for cenotaphs and tombstone delete Bug: The ticket exposed several problems with tombstone handling. - tombstone entries of conflicts were not purged in tombstone purging - cenotaphs are tombstone, but the subordinate count was not managed properly - direct delete of tombstones failed with err=1 - delete of entry with only conflict children failed correctly, but gave no hint why Fix: update the correct numsobordinates attribut for cenotaphs set proper flag in directly deleting a tombstone change search filter for tombstone purging to include ldapsubentries check for conflict children if a delete is rejected and add a message to the response Reviewed by; Thierry, William - thanks --- ldap/servers/plugins/replication/repl5_replica.c | 14 +++++++++-- ldap/servers/plugins/replication/urp.c | 8 +++--- ldap/servers/slapd/back-ldbm/ldbm_add.c | 8 +++--- ldap/servers/slapd/back-ldbm/ldbm_delete.c | 14 ++++++++--- ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c | 3 ++- ldap/servers/slapd/back-ldbm/parents.c | 12 ++++++--- ldap/servers/slapd/entry.c | 31 ++++++++++++++++++++++++ ldap/servers/slapd/slapi-plugin.h | 2 ++ ldap/servers/slapd/slapi-private.h | 1 + ldap/servers/slapd/task.c | 4 +-- 10 files changed, 78 insertions(+), 19 deletions(-) diff --git a/ldap/servers/plugins/replication/repl5_replica.c b/ldap/servers/plugins/replication/repl5_replica.c index bdb8a5167..628fb9ceb 100644 --- a/ldap/servers/plugins/replication/repl5_replica.c +++ b/ldap/servers/plugins/replication/repl5_replica.c @@ -3017,6 +3017,16 @@ process_reap_entry(Slapi_Entry *entry, void *cb_data) search in the future, see _replica_reap_tombstones below and add more to the attrs array */ deletion_csn = entry_get_deletion_csn(entry); + if (deletion_csn == NULL) { + /* this might be a tombstone which was directly added, eg a cenotaph + * check if a tombstonecsn exist and use it + */ + char *tombstonecsn = slapi_entry_attr_get_charptr(entry, SLAPI_ATTR_TOMBSTONE_CSN); + if (tombstonecsn) { + deletion_csn = csn_new_by_string(tombstonecsn); + slapi_ch_free_string(&tombstonecsn); + } + } if ((NULL == deletion_csn || csn_compare(deletion_csn, purge_csn) < 0) && (!is_ruv_tombstone_entry(entry))) { @@ -3116,11 +3126,11 @@ _replica_reap_tombstones(void *arg) */ csn_as_string(purge_csn, PR_FALSE, deletion_csn_str); PR_snprintf(tombstone_filter, 128, - "(&(%s<=%s)(objectclass=nsTombstone))", SLAPI_ATTR_TOMBSTONE_CSN, + "(&(%s<=%s)(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))", SLAPI_ATTR_TOMBSTONE_CSN, csn_as_string(purge_csn, PR_FALSE, deletion_csn_str)); } else { /* Use the old inefficient filter */ - PR_snprintf(tombstone_filter, 128, "(objectclass=nsTombstone)"); + PR_snprintf(tombstone_filter, 128, "(&(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))"); } /* we just need the objectclass - for the deletion csn diff --git a/ldap/servers/plugins/replication/urp.c b/ldap/servers/plugins/replication/urp.c index d4556d7fd..11c5da7cf 100644 --- a/ldap/servers/plugins/replication/urp.c +++ b/ldap/servers/plugins/replication/urp.c @@ -911,7 +911,7 @@ urp_fixup_add_cenotaph (Slapi_PBlock *pb, char *sessionid, CSN *opcsn) cenotaph, NULL, repl_get_plugin_identity(PLUGIN_MULTIMASTER_REPLICATION), - OP_FLAG_REPL_FIXUP|OP_FLAG_NOOP); + OP_FLAG_REPL_FIXUP|OP_FLAG_NOOP|OP_FLAG_CENOTAPH_ENTRY); slapi_add_internal_pb(add_pb); slapi_pblock_get(add_pb, SLAPI_PLUGIN_INTOP_RESULT, &ret); @@ -1922,7 +1922,7 @@ done: newpb = NULL; slapi_log_err(SLAPI_LOG_REPL, sessionid, - "urp_get_min_naming_conflict_entry - Found %d entries\n", i); + "urp_get_min_naming_conflict_entry - Found %d entries\n", min_csn?1:0); return min_naming_conflict_entry; } @@ -2172,8 +2172,8 @@ mod_objectclass_attr(const char *uniqueid, const Slapi_DN *entrysdn, const Slapi char csnstr[CSN_STRSIZE+1] = {0}; slapi_mods_init(&smods, 3); - slapi_mods_add(&smods, LDAP_MOD_ADD, "objectclass", strlen("ldapsubentry"),"ldapsubentry"); - slapi_mods_add(&smods, LDAP_MOD_REPLACE, "conflictcsn", CSN_STRSIZE, csn_as_string(opcsn, PR_FALSE, csnstr)); + slapi_mods_add_string(&smods, LDAP_MOD_ADD, "objectclass", "ldapsubentry"); + slapi_mods_add_string(&smods, LDAP_MOD_REPLACE, "conflictcsn", csn_as_string(opcsn, PR_FALSE, csnstr)); op_result = urp_fixup_modify_entry(uniqueid, entrysdn, opcsn, &smods, 0); slapi_mods_done(&smods); if (op_result == LDAP_TYPE_OR_VALUE_EXISTS) { diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c index c93d44a65..f0a3262ec 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_add.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c @@ -81,6 +81,7 @@ ldbm_back_add(Slapi_PBlock *pb) Slapi_Operation *operation; int is_replicated_operation = 0; int is_resurect_operation = 0; + int is_cenotaph_operation = 0; int is_tombstone_operation = 0; int is_fixup_operation = 0; int is_remove_from_cache = 0; @@ -116,6 +117,7 @@ ldbm_back_add(Slapi_PBlock *pb) } is_resurect_operation = operation_is_flag_set(operation, OP_FLAG_RESURECT_ENTRY); + is_cenotaph_operation = operation_is_flag_set(operation, OP_FLAG_CENOTAPH_ENTRY); is_tombstone_operation = operation_is_flag_set(operation, OP_FLAG_TOMBSTONE_ENTRY); is_fixup_operation = operation_is_flag_set(operation, OP_FLAG_REPL_FIXUP); is_ruv = operation_is_flag_set(operation, OP_FLAG_REPL_RUV); @@ -846,9 +848,9 @@ ldbm_back_add(Slapi_PBlock *pb) the in-memory state of the parent to reflect the new child (update subordinate count specifically */ if (parententry) { - retval = parent_update_on_childchange(&parent_modify_c, - is_resurect_operation ? PARENTUPDATE_RESURECT : PARENTUPDATE_ADD, - NULL); + int op = is_resurect_operation ? PARENTUPDATE_RESURECT : PARENTUPDATE_ADD; + if (is_cenotaph_operation ) op |= PARENTUPDATE_CREATE_TOMBSTONE; + retval = parent_update_on_childchange(&parent_modify_c, op, NULL); slapi_log_err(SLAPI_LOG_BACKLDBM, "ldbm_back_add", "conn=%lu op=%d parent_update_on_childchange: old_entry=0x%p, new_entry=0x%p, rc=%d\n", conn_id, op_id, parent_modify_c.old_entry, parent_modify_c.new_entry, retval); diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index be0db1bd0..bc0a3654e 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c @@ -291,9 +291,16 @@ replace_entry: retval = slapi_entry_has_children(e->ep_entry); if (retval && !is_replicated_operation) { ldap_result_code= LDAP_NOT_ALLOWED_ON_NONLEAF; - slapi_log_err(SLAPI_LOG_BACKLDBM, "ldbm_back_delete", - "conn=%lu op=%d Deleting entry %s has %d children.\n", - conn_id, op_id, slapi_entry_get_dn(e->ep_entry), retval); + if (slapi_entry_has_conflict_children(e->ep_entry, (void *)li->li_identity) > 0) { + ldap_result_message = "Entry has replication conflicts as children"; + slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_delete", + "conn=%lu op=%d Deleting entry %s has replication conflicts as children.\n", + conn_id, op_id, slapi_entry_get_dn(e->ep_entry)); + } else { + slapi_log_err(SLAPI_LOG_BACKLDBM, "ldbm_back_delete", + "conn=%lu op=%d Deleting entry %s has %d children.\n", + conn_id, op_id, slapi_entry_get_dn(e->ep_entry), retval); + } retval = -1; goto error_return; } @@ -431,6 +438,7 @@ replace_entry: slapi_log_err(SLAPI_LOG_WARNING, "ldbm_back_delete", "Attempt to Tombstone again a tombstone entry %s\n", dn); delete_tombstone_entry = 1; + operation_set_flag(operation, OP_FLAG_TOMBSTONE_ENTRY); } } diff --git a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c index b41a2d241..5797dd779 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c @@ -2824,7 +2824,8 @@ _entryrdn_delete_key(backend *be, break; } childelem = (rdn_elem *)dataret.data; - if (!slapi_is_special_rdn(childelem->rdn_elem_nrdn_rdn, RDN_IS_TOMBSTONE)) { + if (!slapi_is_special_rdn(childelem->rdn_elem_nrdn_rdn, RDN_IS_TOMBSTONE) && + !strcasestr(childelem->rdn_elem_nrdn_rdn, "cenotaphid")) { /* there's at least one live child */ slapi_log_err(SLAPI_LOG_ERR, "_entryrdn_delete_key", "Failed to remove %s; has a child %s\n", nrdn, diff --git a/ldap/servers/slapd/back-ldbm/parents.c b/ldap/servers/slapd/back-ldbm/parents.c index 79e66451e..1afc795c0 100644 --- a/ldap/servers/slapd/back-ldbm/parents.c +++ b/ldap/servers/slapd/back-ldbm/parents.c @@ -89,7 +89,11 @@ parent_update_on_childchange(modify_context *mc, int op, size_t *new_sub_count) } } - if (PARENTUPDATE_DELETE_TOMBSTONE != repl_op) { + if ((PARENTUPDATE_ADD == op) && (PARENTUPDATE_CREATE_TOMBSTONE == repl_op)) { + /* we are directly adding a tombstone entry, only need to + * update the tombstone subordinates + */ + } else if (PARENTUPDATE_DELETE_TOMBSTONE != repl_op) { /* are we adding ? */ if (((PARENTUPDATE_ADD == op) || (PARENTUPDATE_RESURECT == op)) && !already_present) { /* If so, and the parent entry does not already have a subcount @@ -136,10 +140,10 @@ parent_update_on_childchange(modify_context *mc, int op, size_t *new_sub_count) } } - /* tombstoneNumSubordinates is needed only when this is repl op - * and a child is being deleted */ + /* tombstoneNumSubordinates has to be updated if a tombstone child has been + * deleted or a tombstone has been directly added (cenotaph) */ current_sub_count = LDAP_MAXINT; - if ((repl_op && (PARENTUPDATE_DEL == op)) || (PARENTUPDATE_RESURECT == op)) { + if (repl_op) { ret = slapi_entry_attr_find(mc->old_entry->ep_entry, tombstone_numsubordinates, &read_attr); if (0 == ret) { diff --git a/ldap/servers/slapd/entry.c b/ldap/servers/slapd/entry.c index 32828b4e2..b85e9f5b0 100644 --- a/ldap/servers/slapd/entry.c +++ b/ldap/servers/slapd/entry.c @@ -3238,6 +3238,37 @@ slapi_entry_has_children(const Slapi_Entry *entry) return slapi_entry_has_children_ext(entry, 0); } +int +slapi_entry_has_conflict_children(const Slapi_Entry *entry, void *plg_id) +{ + Slapi_PBlock *search_pb = NULL; + Slapi_Entry **entries; + int rc = 0; + + search_pb = slapi_pblock_new(); + slapi_search_internal_set_pb(search_pb, slapi_entry_get_dn_const(entry), + LDAP_SCOPE_ONELEVEL, + "(&(objectclass=ldapsubentry)(nsds5ReplConflict=namingConflict*))", + NULL, 0, NULL, NULL, plg_id, 0); + slapi_search_internal_pb(search_pb); + slapi_pblock_get(search_pb, SLAPI_PLUGIN_INTOP_RESULT, &rc); + if (rc) { + rc = -1; + } else { + slapi_pblock_get(search_pb, SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES, &entries); + if (entries && entries[0]) { + /* we found at least one conflict entry */ + rc = 1; + } else { + rc = 0; + } + slapi_free_search_results_internal(search_pb); + } + slapi_pblock_destroy(search_pb); + + return rc; +} + /* * Renames an entry to simulate a MODRDN operation */ diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h index 95cdcc0da..6978e258f 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -2000,6 +2000,8 @@ int slapi_entry_has_children(const Slapi_Entry *e); */ int slapi_entry_has_children_ext(const Slapi_Entry *e, int include_tombstone); +int slapi_entry_has_conflict_children(const Slapi_Entry *e, void *plg_id); + /** * This function determines if an entry is the root DSE. * diff --git a/ldap/servers/slapd/slapi-private.h b/ldap/servers/slapd/slapi-private.h index 548d5cabb..b08c0d7ce 100644 --- a/ldap/servers/slapd/slapi-private.h +++ b/ldap/servers/slapd/slapi-private.h @@ -403,6 +403,7 @@ char *slapi_filter_to_string_internal(const struct slapi_filter *f, char *buf, s #define OP_FLAG_NEVER_CHAIN SLAPI_OP_FLAG_NEVER_CHAIN /* 0x000800 */ #define OP_FLAG_TOMBSTONE_ENTRY SLAPI_OP_FLAG_TOMBSTONE_ENTRY /* 0x001000 */ #define OP_FLAG_RESURECT_ENTRY 0x002000 +#define OP_FLAG_CENOTAPH_ENTRY 0x004000 #define OP_FLAG_ACTION_NOLOG 0x008000 /* Do not log the entry in \ * audit log or change log \ */ diff --git a/ldap/servers/slapd/task.c b/ldap/servers/slapd/task.c index 4bd8895ff..3f9d5d995 100644 --- a/ldap/servers/slapd/task.c +++ b/ldap/servers/slapd/task.c @@ -2352,10 +2352,10 @@ task_fixup_tombstone_thread(void *arg) if (task_data->stripcsn) { /* find tombstones with nsTombstoneCSN */ - filter = "(&(nstombstonecsn=*)(objectclass=nsTombstone))"; + filter = "(&(nstombstonecsn=*)(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))"; } else { /* find tombstones missing nsTombstoneCSN */ - filter = "(&(!(nstombstonecsn=*))(objectclass=nsTombstone))"; + filter = "(&(!(nstombstonecsn=*))(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))"; } /* Okay check the specified backends only */ -- 2.13.6