On 10/29/2015 01:28 PM, thierry bordaz wrote:
On 10/23/2015 10:44 AM, Ludwig Krispenz wrote:
Hi,
the attached two patches address issues I found when testing ca management in the topology plugin


Thanks for review,
Ludwig


Hi Ludwig,

Patch 20 is good to me. I have one remark, you call ipa_topo_cfg_host_find with lock flag. So that the replica config is not updated during the test. Now the lock protects each call separately. The risk is very low that the target host could become unmanaged by the time we test the source host.
yes, and if two paralle operations do related things like adding an agreement and making a host managed/unmanaged there is a race for the lock. The lock itself cannot prevent these things, it only can protect the data structures from being read while modified. Also with two separate locked calls the second call has a chance to be aware of parallel changes
ACK.

Patch 21 is also good. Just in ipa_topo_util_init_hosts, why not calling ipa_topo_cfg_host_add to not duplicate the source ?
no reason, revised patch is attached, thanks for noticing

thanks
thierry

>From 8efbeb6ecbc39c8019d66c69e4759d7ffb34a991 Mon Sep 17 00:00:00 2001
From: Ludwig Krispenz <lkris...@redhat.com>
Date: Fri, 30 Oct 2015 09:44:21 +0100
Subject: [PATCH] update list of managed servers when a suffix becomes managed

    when a suffix becomes managed for a host, the host needs to
    be added to the managed servers, otherwise connectivity check would fail
---
 daemons/ipa-slapi-plugins/topology/topology.h      |  3 +-
 daemons/ipa-slapi-plugins/topology/topology_cfg.c  | 36 ++++++----------------
 daemons/ipa-slapi-plugins/topology/topology_post.c |  5 +--
 daemons/ipa-slapi-plugins/topology/topology_util.c | 28 ++++++++++++++++-
 4 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/topology/topology.h b/daemons/ipa-slapi-plugins/topology/topology.h
index fea8281ac5f0865aca4052f6139e4384f5665b87..d264ed9c1e3e903d7554963b843d1f98385ec47a 100644
--- a/daemons/ipa-slapi-plugins/topology/topology.h
+++ b/daemons/ipa-slapi-plugins/topology/topology.h
@@ -178,7 +178,7 @@ void ipa_topo_lock_conf(void);
 void ipa_topo_unlock_conf(void);
 int ipa_topo_acquire_startup_inprogress(void);
 void ipa_topo_release_startup_inprogress(void);
-void ipa_topo_cfg_host_add(Slapi_Entry *hostentry);
+void ipa_topo_cfg_host_add(TopoReplica *tconf, char *host);
 void ipa_topo_cfg_host_del(Slapi_Entry *hostentry);
 TopoReplicaHost *ipa_topo_cfg_host_find(TopoReplica *tconf, char *host, int lock);
 TopoReplicaHost *ipa_topo_cfg_host_new(char *newhost);
@@ -283,6 +283,7 @@ int ipa_topo_util_setup_servers(void);
 void ipa_topo_util_update_segments_for_host(TopoReplica *conf, char *hostname);
 char *ipa_topo_util_get_ldap_principal(char *repl_root, char *hostname);
 void ipa_topo_util_disable_repl_for_principal(char *repl_root, char *principal);
+void ipa_topo_util_init_hosts(Slapi_Entry *hostentry);
 void ipa_topo_util_add_host(Slapi_Entry *hostentry);
 void ipa_topo_util_delete_host(Slapi_Entry *hostentry);
 void ipa_topo_util_update_host(Slapi_Entry *hostentry, LDAPMod **mods);
diff --git a/daemons/ipa-slapi-plugins/topology/topology_cfg.c b/daemons/ipa-slapi-plugins/topology/topology_cfg.c
index d211f20f6bf267ecf4eca79b423a600e53bc5795..3ca61a8ea7c463c45f3dbf2e13a9790c5079e2d7 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_cfg.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_cfg.c
@@ -471,38 +471,22 @@ ipa_topo_cfg_host_new(char *newhost)
 }
 
 void
-ipa_topo_cfg_host_add(Slapi_Entry *hostentry)
+ipa_topo_cfg_host_add(TopoReplica *replica, char *newhost)
 {
-    char *newhost;
-    char **repl_root = NULL;
     TopoReplicaHost *hostnode = NULL;
-    TopoReplica *replica = NULL;
-    int i;
+    if (replica == NULL || newhost == NULL) return;
 
-    newhost = slapi_entry_attr_get_charptr(hostentry,"cn");
-    if (newhost == NULL) return;
-
-    repl_root = slapi_entry_attr_get_charray(hostentry,"ipaReplTopoManagedSuffix");
-    if (repl_root == NULL || *repl_root == NULL) return;
-
-    for (i=0; repl_root[i];i++) {
-        replica = ipa_topo_cfg_replica_find(repl_root[i], 1);
-        if (replica == NULL) continue;
-
-        slapi_lock_mutex(replica->repl_lock);
-        if (ipa_topo_cfg_host_find(replica, newhost, 0)) {
-            /* log error */
-            slapi_unlock_mutex(replica->repl_lock);
-            continue;
-        }
-        hostnode = ipa_topo_cfg_host_new(slapi_ch_strdup(newhost));
-        hostnode->next = replica->hosts;
-        replica->hosts = hostnode;
+    slapi_lock_mutex(replica->repl_lock);
+    if (ipa_topo_cfg_host_find(replica, newhost, 0)) {
+        /* host already added */
         slapi_unlock_mutex(replica->repl_lock);
+        return;
     }
+    hostnode = ipa_topo_cfg_host_new(slapi_ch_strdup(newhost));
+    hostnode->next = replica->hosts;
+    replica->hosts = hostnode;
+    slapi_unlock_mutex(replica->repl_lock);
 
-    slapi_ch_array_free(repl_root);
-    slapi_ch_free_string(&newhost);
     return;
 }
 
diff --git a/daemons/ipa-slapi-plugins/topology/topology_post.c b/daemons/ipa-slapi-plugins/topology/topology_post.c
index 5ac029a86a2f8ffbda895c5fe2a1af4c45152832..c525d12229bf1a4ebd55faad129e8910e36e352c 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_post.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_post.c
@@ -98,12 +98,11 @@ ipa_topo_post_add(Slapi_PBlock *pb)
         break;
     }
     case TOPO_HOST_ENTRY: {
-        /* add to list of managed hosts */
-        ipa_topo_cfg_host_add(add_entry);
         /* we are adding a new master, there could be
          * a segment which so far was inactive since
          * the host was not managed
          */
+        /* It will also add to list of managed hosts */
         ipa_topo_util_add_host(add_entry);
         break;
     }
@@ -194,6 +193,8 @@ ipa_topo_post_mod(Slapi_PBlock *pb)
         break;
     }
     case TOPO_HOST_ENTRY: {
+        /* check i host needs to be added to the managed hosts
+         * and if segments need to be created */
         ipa_topo_util_update_host(mod_entry, mods);
         break;
     }
diff --git a/daemons/ipa-slapi-plugins/topology/topology_util.c b/daemons/ipa-slapi-plugins/topology/topology_util.c
index 0e348ca74539c8b297cfd35df00c4367ffce444e..de8147a4a1f2c534fa5646210abc0bc9ba51d175 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_util.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_util.c
@@ -278,7 +278,7 @@ ipa_topo_util_setup_servers(void)
         } else {
             int i = 0;
             for (i=0;entries[i];i++) {
-                ipa_topo_cfg_host_add(entries[i]);
+                ipa_topo_util_init_hosts(entries[i]);
             }
         }
     }
@@ -1445,12 +1445,38 @@ ipa_topo_util_delete_segments_for_host(char *repl_root, char *delhost)
                     "ipa_topo_util_delete_segments_for_host <-- done\n");
 }
 
+void
+ipa_topo_util_init_hosts(Slapi_Entry *hostentry)
+{
+    char *newhost;
+    char **repl_root = NULL;
+    TopoReplica *replica = NULL;
+    int i;
+
+    newhost = slapi_entry_attr_get_charptr(hostentry,"cn");
+    if (newhost == NULL) return;
+
+    repl_root = slapi_entry_attr_get_charray(hostentry,"ipaReplTopoManagedSuffix");
+    if (repl_root == NULL || *repl_root == NULL) return;
+
+    for (i=0; repl_root[i];i++) {
+        replica = ipa_topo_cfg_replica_find(repl_root[i], 1);
+        if (replica == NULL) continue;
+
+        ipa_topo_cfg_host_add(replica, newhost);
+    }
+
+    slapi_ch_array_free(repl_root);
+    slapi_ch_free_string(&newhost);
+    return;
+}
 
 void
 ipa_topo_util_add_managed_host(char *suffix, char *addhost)
 {
     TopoReplica *conf = ipa_topo_cfg_replica_find(suffix,1);
     if (conf) {
+        ipa_topo_cfg_host_add(conf, addhost);
         ipa_topo_util_update_segments_for_host(conf, addhost);
     }
 }
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to