Re: [Freeipa-devel] [PATCH 0020-0021] some topology plugin fixes

2015-10-30 Thread Ludwig Krispenz 


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

Re: [Freeipa-devel] [PATCH 0020-0021] some topology plugin fixes

2015-10-30 Thread thierry bordaz 

On 10/30/2015 09:57 AM, Ludwig Krispenz wrote:


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 Ludwig for the changes.

ACK



thanks
thierry




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

Re: [Freeipa-devel] [PATCH 0020-0021] some topology plugin fixes

2015-10-30 Thread Martin Basti 



On 30.10.2015 10:08, thierry bordaz wrote:

On 10/30/2015 09:57 AM, Ludwig Krispenz wrote:


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 Ludwig for the changes.

ACK


Pushed to master: 3f70c9aed7d1357ac5031b8f8b48af320acba567




thanks
thierry








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

Re: [Freeipa-devel] [PATCH 0020-0021] some topology plugin fixes

2015-10-29 Thread thierry bordaz 

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.

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 ?


thanks
thierry
-- 
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