From 62af3beb9b2a3137a76456f534db7be1b172210c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=BA=C5=A1=20Hon=C4=9Bk?= Date: Wed, 16 Jan 2019 09:49:28 +0100 Subject: [PATCH] Issue 50474 - Unify result codes for add and modify of repl5 config Bug Description: Same constraints resulting in error are reported as different LDAP result codes when using different operation for adjusting these. Fix Description: A part of the code had not conveyed the error reason down the stack, therefore adding this information and returning the proper code. Fixes: https://pagure.io/389-ds-base/issue/50474 Author: Matus Honek Review by: mreynolds, spichugi (thanks!) --- .../suites/replication/replica_config_test.py | 25 +++++-- ldap/servers/plugins/replication/repl5.h | 2 +- .../plugins/replication/repl5_replica.c | 71 +++++++++++-------- .../replication/repl5_replica_config.c | 3 +- 4 files changed, 63 insertions(+), 38 deletions(-) diff --git a/dirsrvtests/tests/suites/replication/replica_config_test.py b/dirsrvtests/tests/suites/replication/replica_config_test.py index 9a0e1b41f..3dc03713a 100644 --- a/dirsrvtests/tests/suites/replication/replica_config_test.py +++ b/dirsrvtests/tests/suites/replication/replica_config_test.py @@ -4,7 +4,6 @@ import copy import os import ldap from lib389._constants import * -from lib389 import Entry from lib389.topologies import topology_st as topo from lib389.replica import Replicas @@ -104,12 +103,14 @@ def agmt_setup(topo): def perform_invalid_create(many, properties, attr, value): my_properties = copy.deepcopy(properties) my_properties[attr] = value - with pytest.raises(ldap.LDAPError): + with pytest.raises(ldap.LDAPError) as ei: many.create(properties=my_properties) + return ei.value def perform_invalid_modify(o, attr, value): - with pytest.raises(ldap.LDAPError): + with pytest.raises(ldap.LDAPError) as ei: o.replace(attr, value) + return ei.value @pytest.mark.parametrize("attr, too_small, too_big, overflow, notnum, valid", repl_add_attrs) def test_replica_num_add(topo, attr, too_small, too_big, overflow, notnum, valid): @@ -254,9 +255,25 @@ def test_agmt_num_modify(topo, attr, too_small, too_big, overflow, notnum, valid # Value is valid agmt.replace(attr, valid) + +@pytest.mark.bz1546739 +def test_same_attr_yields_same_return_code(topo): + """Test that various operations with same incorrect attribute value yield same return code + """ + attr = 'nsDS5ReplicaId' + + replica_reset(topo) + replicas = Replicas(topo.standalone) + e = perform_invalid_create(replicas, replica_dict, attr, too_big) + assert type(e) is ldap.UNWILLING_TO_PERFORM + + replica = replica_setup(topo) + e = perform_invalid_modify(replica, attr, too_big) + assert type(e) is ldap.UNWILLING_TO_PERFORM + + if __name__ == '__main__': # Run isolated # -s for DEBUG mode CURRENT_FILE = os.path.realpath(__file__) pytest.main(["-s", CURRENT_FILE]) - diff --git a/ldap/servers/plugins/replication/repl5.h b/ldap/servers/plugins/replication/repl5.h index 138578d5f..1801a333e 100644 --- a/ldap/servers/plugins/replication/repl5.h +++ b/ldap/servers/plugins/replication/repl5.h @@ -662,7 +662,7 @@ Replica *replica_new(const Slapi_DN *root); Replica *windows_replica_new(const Slapi_DN *root); /* this function should be called to construct the replica object during addition of the replica over LDAP */ -Replica *replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation); +int replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation, Replica **r); void replica_destroy(void **arg); int replica_subentry_update(Slapi_DN *repl_root, ReplicaId rid); int replica_subentry_check(Slapi_DN *repl_root, ReplicaId rid); diff --git a/ldap/servers/plugins/replication/repl5_replica.c b/ldap/servers/plugins/replication/repl5_replica.c index 6a5363e43..b3f03d5c0 100644 --- a/ldap/servers/plugins/replication/repl5_replica.c +++ b/ldap/servers/plugins/replication/repl5_replica.c @@ -128,8 +128,9 @@ replica_new(const Slapi_DN *root) e = _replica_get_config_entry(root, NULL); if (e) { errorbuf[0] = '\0'; - r = replica_new_from_entry(e, errorbuf, - PR_FALSE /* not a newly added entry */); + replica_new_from_entry(e, errorbuf, + PR_FALSE, /* not a newly added entry */ + &r); if (NULL == r) { slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "replica_new - " @@ -143,17 +144,17 @@ replica_new(const Slapi_DN *root) } /* constructs the replica object from the newly added entry */ -Replica * -replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation) +int +replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation, Replica **rp) { - int rc = 0; Replica *r; + int rc = LDAP_SUCCESS; if (e == NULL) { if (NULL != errortext) { PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "NULL entry"); } - return NULL; + return LDAP_OTHER; } r = (Replica *)slapi_ch_calloc(1, sizeof(Replica)); @@ -162,7 +163,7 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation) if (NULL != errortext) { PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "Out of memory"); } - rc = -1; + rc = LDAP_OTHER; goto done; } @@ -170,7 +171,7 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation) if (NULL != errortext) { PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "failed to create replica lock"); } - rc = -1; + rc = LDAP_OTHER; goto done; } @@ -178,7 +179,7 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation) if (NULL != errortext) { PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "failed to create replica lock"); } - rc = -1; + rc = LDAP_OTHER; goto done; } @@ -191,14 +192,17 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation) /* read parameters from the replica config entry */ rc = _replica_init_from_config(r, e, errortext); - if (rc != 0) { + if (rc != LDAP_SUCCESS) { goto done; } /* configure ruv */ rc = _replica_configure_ruv(r, PR_FALSE); if (rc != 0) { + rc = LDAP_OTHER; goto done; + } else { + rc = LDAP_SUCCESS; } /* If smallest csn exists in RUV for our local replica, it's ok to begin iteration */ @@ -217,8 +221,12 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation) * (done by the update state event scheduled below) */ } - if (rc != 0) + if (rc != 0) { + rc = LDAP_OTHER; goto done; + } else { + rc = LDAP_SUCCESS; + } /* ONREPL - the state update can occur before the entry is added to the DIT. In that case the updated would fail but nothing bad would happen. The next @@ -237,11 +245,12 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation) } done: - if (rc != 0 && r) { + if (rc != LDAP_SUCCESS && r) { replica_destroy((void **)&r); } - return r; + *rp = r; + return rc; } @@ -1789,9 +1798,9 @@ _replica_check_validity(const Replica *r) if (r->repl_root == NULL || r->repl_type == 0 || r->repl_rid == 0 || r->repl_csngen == NULL || r->repl_name == NULL) { - return -1; + return LDAP_OTHER; } else { - return 0; + return LDAP_SUCCESS; } } @@ -1841,7 +1850,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) (char *)slapi_entry_get_dn((Slapi_Entry *)e)); slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - %s\n", errormsg); - return -1; + return LDAP_OTHER; } r->repl_root = slapi_sdn_new_dn_passin(val); @@ -1851,7 +1860,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, attr_replicaType))) { if (repl_config_valid_num(attr_replicaType, val, 0, REPLICA_TYPE_UPDATABLE, &rc, errormsg, &rtype) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } r->repl_type = rtype; slapi_ch_free_string(&val); @@ -1867,7 +1876,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, type_replicaBackoffMin))) { if (repl_config_valid_num(type_replicaBackoffMin, val, 1, INT_MAX, &rc, errormsg, &backoff_min) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } slapi_ch_free_string(&val); } else { @@ -1882,7 +1891,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, type_replicaBackoffMax))) { if (repl_config_valid_num(type_replicaBackoffMax, val, 1, INT_MAX, &rc, errormsg, &backoff_max) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } slapi_ch_free_string(&val); } else { @@ -1899,7 +1908,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) backoff_min, backoff_max); slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - " "%s\n", errormsg); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } else { slapi_counter_set_value(r->backoff_min, backoff_min); slapi_counter_set_value(r->backoff_max, backoff_max); @@ -1910,7 +1919,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, type_replicaProtocolTimeout))) { if (repl_config_valid_num(type_replicaProtocolTimeout, val, 0, INT_MAX, &rc, errormsg, &ptimeout) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } slapi_ch_free_string(&val); slapi_counter_set_value(r->protocol_timeout, ptimeout); @@ -1926,7 +1935,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, type_replicaReleaseTimeout))) { if (repl_config_valid_num(type_replicaReleaseTimeout, val, 0, INT_MAX, &rc, errortext, &release_timeout) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } slapi_counter_set_value(r->release_timeout, release_timeout); slapi_ch_free_string(&val); @@ -1950,7 +1959,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) type_replicaPrecisePurge, precise_purging); slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - " "%s\n", errormsg); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } slapi_ch_free_string(&precise_purging); } else { @@ -1963,7 +1972,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if((val = slapi_entry_attr_get_charptr(e, attr_flags))) { if (repl_config_valid_num(attr_flags, val, 0, 1, &rc, errortext, &rflags) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } r->repl_flags = (uint32_t)rflags; slapi_ch_free_string(&val); @@ -1990,7 +1999,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) int64_t rid; if (repl_config_valid_num(attr_replicaId, val, 1, 65534, &rc, errormsg, &rid) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } r->repl_rid = (ReplicaId)rid; slapi_ch_free_string(&val); @@ -2000,7 +2009,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) attr_replicaId, (char *)slapi_entry_get_dn((Slapi_Entry *)e)); slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - %s\n", errormsg); - return -1; + return LDAP_OTHER; } } @@ -2013,7 +2022,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) (char *)slapi_entry_get_dn((Slapi_Entry *)e)); slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - %s\n", errormsg); - return -1; + return LDAP_OTHER; } r->repl_csngen = object_new((void *)gen, (FNFree)csngen_free); @@ -2031,7 +2040,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, attr_replicaBindDnGroupCheckInterval))) { if (repl_config_valid_num(attr_replicaBindDnGroupCheckInterval, val, -1, INT_MAX, &rc, errormsg, &interval) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } r->updatedn_group_check_interval = interval; slapi_ch_free_string(&val); @@ -2051,7 +2060,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) (char *)slapi_entry_get_dn((Slapi_Entry *)e), rc); slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - %s\n", errormsg); - return -1; + return LDAP_OTHER; } else r->new_name = PR_TRUE; } @@ -2072,7 +2081,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, type_replicaPurgeDelay))) { if (repl_config_valid_num(type_replicaPurgeDelay, val, -1, INT_MAX, &rc, errormsg, &interval) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } r->repl_purge_delay = interval; slapi_ch_free_string(&val); @@ -2083,7 +2092,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext) if ((val = slapi_entry_attr_get_charptr(e, type_replicaTombstonePurgeInterval))) { if (repl_config_valid_num(type_replicaTombstonePurgeInterval, val, -1, INT_MAX, &rc, errormsg, &interval) != 0) { slapi_ch_free_string(&val); - return -1; + return LDAP_UNWILLING_TO_PERFORM; } r->tombstone_reap_interval = interval; slapi_ch_free_string(&val); diff --git a/ldap/servers/plugins/replication/repl5_replica_config.c b/ldap/servers/plugins/replication/repl5_replica_config.c index 7649aa14e..749e90936 100644 --- a/ldap/servers/plugins/replication/repl5_replica_config.c +++ b/ldap/servers/plugins/replication/repl5_replica_config.c @@ -267,9 +267,8 @@ replica_config_add(Slapi_PBlock *pb __attribute__((unused)), } /* create replica object */ - r = replica_new_from_entry(e, errortext, PR_TRUE /* is a newly added entry */); + *returncode = replica_new_from_entry(e, errortext, PR_TRUE /* is a newly added entry */, &r); if (r == NULL) { - *returncode = LDAP_OPERATIONS_ERROR; goto done; } -- 2.21.0