From 62af3beb9b2a3137a76456f534db7be1b172210c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mat=C3=BA=C5=A1=20Hon=C4=9Bk?= <mhonek@redhat.com>
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 <mhonek@redhat.com>
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