389-ds-base

Clone
Source Code
GIT

#50475 Issue 50474 - Unify result codes for add and modify of repl5 config
Closed 3 years ago by spichugi. Opened 4 years ago by mhonek.
mhonek/389-ds-base repl5-add-modify-return-code  into  master

file modified
+21 -4 
@@ -4,7 +4,6 @@ 
 

  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 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 @@ 
 

      # 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])
 

-

file modified
+1 -1 
@@ -662,7 +662,7 @@ 
 

  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);

file modified
+40 -31 
@@ -128,8 +128,9 @@ 
 

      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 @@ 
 

  }
 

  

  /* 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 @@ 
 

          if (NULL != errortext) {
 

              PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "Out of memory");
 

          }
 

-         rc = -1;
 

+         rc = LDAP_OTHER;
 

          goto done;
 

      }
 

  
@@ -170,7 +171,7 @@ 
 

          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 @@ 
 

          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 @@ 
 

  

      /* 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 @@ 
 

           * (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 @@ 
 

      }
 

  

  done:
 

-     if (rc != 0 && r) {
 

+     if (rc != LDAP_SUCCESS && r) {
 

          replica_destroy((void **)&r);
 

      }
 

  

-     return r;
 

+     *rp = r;
 

+     return rc;
 

  }
 

  

  
@@ -1789,9 +1798,9 @@ 
 

  

      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 @@ 
 

                      (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 @@ 
 

          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 @@ 
 

          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 @@ 
 

          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 @@ 
 

                      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 @@ 
 

          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 @@ 
 

          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 @@ 
 

                          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 @@ 
 

          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 @@ 
 

              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 @@ 
 

                          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 @@ 
 

                      (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 @@ 
 

      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 @@ 
 

                          (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 @@ 
 

      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 @@ 
 

      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);

file modified
+1 -2 
@@ -267,9 +267,8 @@ 
 

      }
 

  

      /* 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;
 

      }

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: ???

LGTM

Also LGTM.
The replication test suite is PASSed.

rebased onto 9bf0fc2
4 years ago

Pull-Request has been merged by mhonek
4 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3532

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 4
+21 -4
file changed
dirsrvtests/tests/suites/replication/replica_config_test.py
+1 -1
file changed
ldap/servers/plugins/replication/repl5.h
+40 -31
file changed
ldap/servers/plugins/replication/repl5_replica.c
+1 -2
file changed
ldap/servers/plugins/replication/repl5_replica_config.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.