From: Marcel Holtmann <marcel@holtmann.org>
Date: Thu, 3 Jul 2008 11:32:11 +0000 (+0200)
Subject: Simplify the locking and remove some deadlocks
X-Git-Tag: 0.1~296
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=dd1ef7a7020ce9a32c55476e3e9a5587b141537b;p=platform%2Fupstream%2Fconnman.git

Simplify the locking and remove some deadlocks
---

diff --git a/include/element.h b/include/element.h
index af34da8..c4d97cc 100644
--- a/include/element.h
+++ b/include/element.h
@@ -66,6 +66,7 @@ struct connman_driver;
 
 struct connman_element {
 	gint refcount;
+	GStaticMutex mutex;
 	gchar *name;
 	gchar *path;
 	enum connman_element_type type;
@@ -96,6 +97,9 @@ struct connman_element {
 	} ipv4;
 };
 
+#define connman_element_lock(element)    g_static_mutex_lock(&(element)->mutex)
+#define connman_element_unlock(element)  g_static_mutex_unlock(&(element)->mutex)
+
 extern struct connman_element *connman_element_create(void);
 extern struct connman_element *connman_element_ref(struct connman_element *element);
 extern void connman_element_unref(struct connman_element *element);
diff --git a/plugins/ethernet.c b/plugins/ethernet.c
index 8905223..21ca7fc 100644
--- a/plugins/ethernet.c
+++ b/plugins/ethernet.c
@@ -236,6 +236,8 @@ static void ethernet_remove(struct connman_element *element)
 {
 	DBG("element %p name %s", element, element->name);
 
+	remove_elements(element);
+
 	g_static_mutex_lock(&ethernet_mutex);
 	ethernet_list = g_slist_remove(ethernet_list, element);
 	g_static_mutex_unlock(&ethernet_mutex);
diff --git a/plugins/ipv4.c b/plugins/ipv4.c
index 8b3457b..6d2e2a8 100644
--- a/plugins/ipv4.c
+++ b/plugins/ipv4.c
@@ -50,10 +50,10 @@ static int ipv4_probe(struct connman_element *element)
 	resolver->type = CONNMAN_ELEMENT_TYPE_RESOLVER;
 	resolver->netdev.name = g_strdup(element->netdev.name);
 
-	connman_element_register(resolver, element);
-
 	connman_element_set_data(element, resolver);
 
+	connman_element_register(resolver, element);
+
 	return 0;
 }
 
diff --git a/src/element.c b/src/element.c
index 55fa6cb..01cae2d 100644
--- a/src/element.c
+++ b/src/element.c
@@ -32,13 +32,13 @@
 
 static DBusConnection *connection;
 
-static GStaticRWLock driver_lock = G_STATIC_RW_LOCK_INIT;
-static GSList *driver_list = NULL;
-static GThreadPool *driver_thread;
-
 static GStaticRWLock element_lock = G_STATIC_RW_LOCK_INIT;
 static GNode *element_root = NULL;
-static GThreadPool *element_thread;
+
+static GSList *driver_list = NULL;
+
+static GThreadPool *thread_register;
+static GThreadPool *thread_unregister;
 
 static const char *type2string(enum connman_element_type type)
 {
@@ -246,22 +246,59 @@ static gint compare_priority(gconstpointer a, gconstpointer b)
 	return driver2->priority - driver1->priority;
 }
 
+static gboolean match_driver(struct connman_element *element,
+					struct connman_driver *driver)
+{
+	if (element->type != driver->type &&
+			driver->type != CONNMAN_ELEMENT_TYPE_UNKNOWN)
+		return FALSE;
+
+	if (element->subtype == driver->subtype ||
+			driver->subtype == CONNMAN_ELEMENT_SUBTYPE_UNKNOWN)
+		return TRUE;
+
+	return FALSE;
+}
+
+static gboolean probe_driver(GNode *node, gpointer data)
+{
+	struct connman_element *element = node->data;
+	struct connman_driver *driver = data;
+
+	DBG("element %p name %s", element, element->name);
+
+	if (!element->driver && match_driver(element, driver) == TRUE) {
+		if (driver->probe(element) < 0)
+			return FALSE;
+
+		connman_element_lock(element);
+		element->driver = driver;
+		connman_element_unlock(element);
+	}
+
+	return FALSE;
+}
+
 int connman_driver_register(struct connman_driver *driver)
 {
 	DBG("driver %p name %s", driver, driver->name);
 
 	if (driver->type == CONNMAN_ELEMENT_TYPE_ROOT)
-		return -1;
+		return -EINVAL;
 
 	if (!driver->probe)
 		return -EINVAL;
 
-	g_static_rw_lock_writer_lock(&driver_lock);
+	g_static_rw_lock_writer_lock(&element_lock);
+
 	driver_list = g_slist_insert_sorted(driver_list, driver,
 							compare_priority);
-	g_static_rw_lock_writer_unlock(&driver_lock);
 
-	g_thread_pool_push(driver_thread, driver, NULL);
+	if (element_root != NULL)
+		g_node_traverse(element_root, G_PRE_ORDER,
+				G_TRAVERSE_ALL, -1, probe_driver, driver);
+
+	g_static_rw_lock_writer_unlock(&element_lock);
 
 	return 0;
 }
@@ -276,7 +313,10 @@ static gboolean remove_driver(GNode *node, gpointer data)
 	if (element->driver == driver) {
 		if (driver->remove)
 			driver->remove(element);
+
+		connman_element_lock(element);
 		element->driver = NULL;
+		connman_element_unlock(element);
 	}
 
 	return FALSE;
@@ -286,14 +326,15 @@ void connman_driver_unregister(struct connman_driver *driver)
 {
 	DBG("driver %p name %s", driver, driver->name);
 
-	g_static_rw_lock_reader_lock(&element_lock);
-	g_node_traverse(element_root, G_POST_ORDER, G_TRAVERSE_ALL, -1,
-							remove_driver, driver);
-	g_static_rw_lock_reader_unlock(&element_lock);
+	g_static_rw_lock_writer_lock(&element_lock);
 
-	g_static_rw_lock_writer_lock(&driver_lock);
 	driver_list = g_slist_remove(driver_list, driver);
-	g_static_rw_lock_writer_unlock(&driver_lock);
+
+	if (element_root != NULL)
+		g_node_traverse(element_root, G_POST_ORDER,
+				G_TRAVERSE_ALL, -1, remove_driver, driver);
+
+	g_static_rw_lock_writer_unlock(&element_lock);
 }
 
 struct connman_element *connman_element_create(void)
@@ -306,6 +347,8 @@ struct connman_element *connman_element_create(void)
 
 	element->refcount = 1;
 
+	g_static_mutex_init(&element->mutex);
+
 	element->type    = CONNMAN_ELEMENT_TYPE_UNKNOWN;
 	element->subtype = CONNMAN_ELEMENT_SUBTYPE_UNKNOWN;
 	element->state   = CONNMAN_ELEMENT_STATE_CLOSED;
@@ -383,7 +426,9 @@ int connman_element_add_static_property(struct connman_element *element,
 		break;
 	}
 
+	connman_element_lock(element);
 	element->properties = g_slist_append(element->properties, property);
+	connman_element_unlock(element);
 
 	return 0;
 }
@@ -395,20 +440,28 @@ int connman_element_set_property(struct connman_element *element,
 	case CONNMAN_PROPERTY_TYPE_INVALID:
 		return -EINVAL;
 	case CONNMAN_PROPERTY_TYPE_IPV4_ADDRESS:
+		connman_element_lock(element);
 		g_free(element->ipv4.address);
 		element->ipv4.address = g_strdup(*((const char **) value));
+		connman_element_unlock(element);
 		break;
 	case CONNMAN_PROPERTY_TYPE_IPV4_NETMASK:
+		connman_element_lock(element);
 		g_free(element->ipv4.netmask);
 		element->ipv4.netmask = g_strdup(*((const char **) value));
+		connman_element_unlock(element);
 		break;
 	case CONNMAN_PROPERTY_TYPE_IPV4_GATEWAY:
+		connman_element_lock(element);
 		g_free(element->ipv4.gateway);
 		element->ipv4.gateway = g_strdup(*((const char **) value));
+		connman_element_unlock(element);
 		break;
 	case CONNMAN_PROPERTY_TYPE_IPV4_NAMESERVER:
+		connman_element_lock(element);
 		g_free(element->ipv4.nameserver);
 		element->ipv4.nameserver = g_strdup(*((const char **) value));
+		connman_element_unlock(element);
 		break;
 	}
 
@@ -433,25 +486,33 @@ int connman_element_get_value(struct connman_element *element,
 		if (element->ipv4.address == NULL)
 			return connman_element_get_value(element->parent,
 								type, value);
+		connman_element_lock(element);
 		*((char **) value) = element->ipv4.address;
+		connman_element_unlock(element);
 		break;
 	case CONNMAN_PROPERTY_TYPE_IPV4_NETMASK:
 		if (element->ipv4.netmask == NULL)
 			return connman_element_get_value(element->parent,
 								type, value);
+		connman_element_lock(element);
 		*((char **) value) = element->ipv4.netmask;
+		connman_element_unlock(element);
 		break;
 	case CONNMAN_PROPERTY_TYPE_IPV4_GATEWAY:
 		if (element->ipv4.gateway == NULL)
 			return connman_element_get_value(element->parent,
 								type, value);
+		connman_element_lock(element);
 		*((char **) value) = element->ipv4.gateway;
+		connman_element_unlock(element);
 		break;
 	case CONNMAN_PROPERTY_TYPE_IPV4_NAMESERVER:
 		if (element->ipv4.nameserver == NULL)
 			return connman_element_get_value(element->parent,
 								type, value);
+		connman_element_lock(element);
 		*((char **) value) = element->ipv4.nameserver;
+		connman_element_unlock(element);
 		break;
 	}
 
@@ -461,29 +522,14 @@ int connman_element_get_value(struct connman_element *element,
 int connman_element_register(struct connman_element *element,
 					struct connman_element *parent)
 {
-	GNode *node;
-	const gchar *basepath;
-
 	DBG("element %p name %s parent %p", element, element->name, parent);
 
 	if (connman_element_ref(element) == NULL)
-		return -1;
-
-	__connman_element_load(element);
-
-	g_static_rw_lock_writer_lock(&element_lock);
+		return -EINVAL;
 
-	if (parent) {
-		node = g_node_find(element_root, G_PRE_ORDER,
-						G_TRAVERSE_ALL, parent);
-		basepath = parent->path;
+	connman_element_lock(element);
 
-		if (element->subtype == CONNMAN_ELEMENT_SUBTYPE_UNKNOWN)
-			element->subtype = parent->subtype;
-	} else {
-		node = element_root;
-		basepath = "";
-	}
+	__connman_element_load(element);
 
 	if (element->name == NULL) {
 		switch (element->type) {
@@ -510,75 +556,20 @@ int connman_element_register(struct connman_element *element,
 		}
 	}
 
-	element->path = g_strdup_printf("%s/%s", basepath, element->name);
 	element->parent = parent;
 
-	DBG("element %p path %s", element, element->path);
-
-	g_node_append_data(node, element);
-
-	g_static_rw_lock_writer_unlock(&element_lock);
-
-	__connman_element_store(element);
-
-	g_dbus_register_interface(connection, element->path,
-					CONNMAN_ELEMENT_INTERFACE,
-					element_methods, NULL, NULL,
-							element, NULL);
-
-	g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
-				CONNMAN_MANAGER_INTERFACE, "ElementAdded",
-				DBUS_TYPE_OBJECT_PATH, &element->path,
-							DBUS_TYPE_INVALID);
-
-	if (element->type == CONNMAN_ELEMENT_TYPE_DEVICE)
-		g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
-				CONNMAN_MANAGER_INTERFACE, "DeviceAdded",
-				DBUS_TYPE_OBJECT_PATH, &element->path,
-							DBUS_TYPE_INVALID);
+	connman_element_unlock(element);
 
-	g_thread_pool_push(element_thread, element, NULL);
+	g_thread_pool_push(thread_register, element, NULL);
 
 	return 0;
 }
 
 void connman_element_unregister(struct connman_element *element)
 {
-	GNode *node;
-
 	DBG("element %p name %s", element, element->name);
 
-	if (element->type == CONNMAN_ELEMENT_TYPE_DEVICE)
-		g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
-				CONNMAN_MANAGER_INTERFACE, "DeviceRemoved",
-				DBUS_TYPE_OBJECT_PATH, &element->path,
-							DBUS_TYPE_INVALID);
-
-	g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
-				CONNMAN_MANAGER_INTERFACE, "ElementRemoved",
-				DBUS_TYPE_OBJECT_PATH, &element->path,
-							DBUS_TYPE_INVALID);
-
-	g_dbus_unregister_interface(connection, element->path,
-						CONNMAN_ELEMENT_INTERFACE);
-
-	g_static_rw_lock_writer_lock(&element_lock);
-
-	if (element->driver) {
-		if (element->driver->remove)
-			element->driver->remove(element);
-		element->driver = NULL;
-	}
-
-	node = g_node_find(element_root, G_PRE_ORDER, G_TRAVERSE_ALL, element);
-	if (node != NULL) {
-		g_node_unlink(node);
-		g_node_destroy(node);
-	}
-
-	g_static_rw_lock_writer_unlock(&element_lock);
-
-	connman_element_unref(element);
+	g_thread_pool_push(thread_unregister, element, NULL);
 }
 
 void connman_element_update(struct connman_element *element)
@@ -598,84 +589,118 @@ void connman_element_update(struct connman_element *element)
 							DBUS_TYPE_INVALID);
 }
 
-static inline void set_driver(struct connman_element *element,
-						struct connman_driver *driver)
+static void register_element(gpointer data, gpointer user_data)
 {
-	g_static_rw_lock_reader_lock(&element_lock);
-	element->driver = driver;
-	g_static_rw_lock_reader_unlock(&element_lock);
-}
+	struct connman_element *element = data;
+	const gchar *basepath;
+	GSList *list;
+	GNode *node;
 
-static gboolean match_driver(struct connman_element *element,
-					struct connman_driver *driver)
-{
-	if (element->type != driver->type &&
-			driver->type != CONNMAN_ELEMENT_TYPE_UNKNOWN)
-		return FALSE;
+	g_static_rw_lock_writer_lock(&element_lock);
 
-	if (element->subtype == driver->subtype ||
-			driver->subtype == CONNMAN_ELEMENT_SUBTYPE_UNKNOWN)
-		return TRUE;
+	connman_element_lock(element);
 
-	return FALSE;
-}
+	if (element->parent) {
+		node = g_node_find(element_root, G_PRE_ORDER,
+					G_TRAVERSE_ALL, element->parent);
+		basepath = element->parent->path;
 
-static gboolean probe_driver(GNode *node, gpointer data)
-{
-	struct connman_element *element = node->data;
-	struct connman_driver *driver = data;
+		if (element->subtype == CONNMAN_ELEMENT_SUBTYPE_UNKNOWN)
+			element->subtype = element->parent->subtype;
+	} else {
+		node = element_root;
+		basepath = "";
+	}
 
-	DBG("element %p name %s", element, element->name);
+	element->path = g_strdup_printf("%s/%s", basepath, element->name);
 
-	if (!element->driver && match_driver(element, driver) == TRUE) {
-		element->driver = driver;
+	connman_element_unlock(element);
 
-		if (driver->probe(element) < 0)
-			element->driver = NULL;
-	}
+	DBG("element %p path %s", element, element->path);
 
-	return FALSE;
-}
+	g_node_append_data(node, element);
 
-static void driver_probe(gpointer data, gpointer user_data)
-{
-	struct connman_driver *driver = data;
+	g_static_rw_lock_writer_unlock(&element_lock);
 
-	DBG("driver %p name %s", driver, driver->name);
+	__connman_element_store(element);
 
-	g_static_rw_lock_reader_lock(&element_lock);
-	g_node_traverse(element_root, G_PRE_ORDER, G_TRAVERSE_ALL, -1,
-							probe_driver, driver);
-	g_static_rw_lock_reader_unlock(&element_lock);
+	g_dbus_register_interface(connection, element->path,
+					CONNMAN_ELEMENT_INTERFACE,
+					element_methods, NULL, NULL,
+							element, NULL);
+
+	g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
+				CONNMAN_MANAGER_INTERFACE, "ElementAdded",
+				DBUS_TYPE_OBJECT_PATH, &element->path,
+							DBUS_TYPE_INVALID);
+
+	if (element->type == CONNMAN_ELEMENT_TYPE_DEVICE)
+		g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
+				CONNMAN_MANAGER_INTERFACE, "DeviceAdded",
+				DBUS_TYPE_OBJECT_PATH, &element->path,
+							DBUS_TYPE_INVALID);
+
+	g_static_rw_lock_writer_lock(&element_lock);
+
+	for (list = driver_list; list; list = list->next) {
+		struct connman_driver *driver = list->data;
+
+		if (match_driver(element, driver) == FALSE)
+			continue;
+
+		DBG("driver %p name %s", driver, driver->name);
+
+		if (driver->probe(element) < 0)
+			continue;
+
+		connman_element_lock(element);
+		element->driver = driver;
+		connman_element_unlock(element);
+	}
+
+	g_static_rw_lock_writer_unlock(&element_lock);
 }
 
-static void element_probe(gpointer data, gpointer user_data)
+static void unregister_element(gpointer data, gpointer user_data)
 {
 	struct connman_element *element = data;
-	GSList *list;
+	GNode *node;
 
 	DBG("element %p name %s", element, element->name);
 
-	if (connman_element_ref(element) == NULL)
-		return;
+	g_static_rw_lock_writer_lock(&element_lock);
 
-	g_static_rw_lock_reader_lock(&driver_lock);
+	node = g_node_find(element_root, G_PRE_ORDER, G_TRAVERSE_ALL, element);
 
-	for (list = driver_list; list; list = list->next) {
-		struct connman_driver *driver = list->data;
+	if (element->driver) {
+		if (element->driver->remove)
+			element->driver->remove(element);
 
-		DBG("driver %p name %s", driver, driver->name);
+		connman_element_lock(element);
+		element->driver = NULL;
+		connman_element_unlock(element);
+	}
 
-		set_driver(element, driver);
+	if (node != NULL) {
+		g_node_unlink(node);
+		g_node_destroy(node);
+	}
 
-		if (match_driver(element, driver) == TRUE &&
-						driver->probe(element) == 0)
-			break;
+	g_static_rw_lock_writer_unlock(&element_lock);
 
-		set_driver(element, NULL);
-	}
+	if (element->type == CONNMAN_ELEMENT_TYPE_DEVICE)
+		g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
+				CONNMAN_MANAGER_INTERFACE, "DeviceRemoved",
+				DBUS_TYPE_OBJECT_PATH, &element->path,
+							DBUS_TYPE_INVALID);
+
+	g_dbus_emit_signal(connection, CONNMAN_MANAGER_PATH,
+				CONNMAN_MANAGER_INTERFACE, "ElementRemoved",
+				DBUS_TYPE_OBJECT_PATH, &element->path,
+							DBUS_TYPE_INVALID);
 
-	g_static_rw_lock_reader_unlock(&driver_lock);
+	g_dbus_unregister_interface(connection, element->path,
+						CONNMAN_ELEMENT_INTERFACE);
 
 	connman_element_unref(element);
 }
@@ -702,31 +727,40 @@ int __connman_element_init(DBusConnection *conn)
 
 	g_static_rw_lock_writer_unlock(&element_lock);
 
-	element_thread = g_thread_pool_new(element_probe, NULL, 1, FALSE, NULL);
-
-	driver_thread = g_thread_pool_new(driver_probe, NULL, 1, FALSE, NULL);
+	thread_register = g_thread_pool_new(register_element,
+							NULL, 1, FALSE, NULL);
+	thread_unregister = g_thread_pool_new(unregister_element,
+							NULL, 1, FALSE, NULL);
 
 	return 0;
 }
 
-static gboolean free_node(GNode *node, gpointer data)
+static gboolean free_driver(GNode *node, gpointer data)
 {
 	struct connman_element *element = node->data;
 
 	DBG("element %p name %s", element, element->name);
 
-	g_dbus_unregister_interface(connection, element->path,
-						CONNMAN_ELEMENT_INTERFACE);
-
 	if (element->driver) {
 		if (element->driver->remove)
 			element->driver->remove(element);
+
+		connman_element_lock(element);
 		element->driver = NULL;
+		connman_element_unlock(element);
 	}
 
-	connman_element_unref(element);
+	return FALSE;
+}
+
+static gboolean free_node(GNode *node, gpointer data)
+{
+	struct connman_element *element = node->data;
+
+	DBG("element %p name %s", element, element->name);
 
-	node->data = NULL;
+	if (g_node_depth(node) > 1)
+		g_thread_pool_push(thread_unregister, element, NULL);
 
 	return FALSE;
 }
@@ -735,18 +769,23 @@ void __connman_element_cleanup(void)
 {
 	DBG("");
 
-	g_thread_pool_free(driver_thread, TRUE, TRUE);
-
-	g_thread_pool_free(element_thread, TRUE, TRUE);
+	g_thread_pool_free(thread_register, TRUE, TRUE);
 
 	g_static_rw_lock_writer_lock(&element_lock);
+	g_node_traverse(element_root, G_POST_ORDER, G_TRAVERSE_ALL, -1,
+							free_driver, NULL);
+	g_static_rw_lock_writer_unlock(&element_lock);
 
+	g_static_rw_lock_writer_lock(&element_lock);
 	g_node_traverse(element_root, G_POST_ORDER, G_TRAVERSE_ALL, -1,
 							free_node, NULL);
+	g_static_rw_lock_writer_unlock(&element_lock);
 
+	g_thread_pool_free(thread_unregister, FALSE, TRUE);
+
+	g_static_rw_lock_writer_lock(&element_lock);
 	g_node_destroy(element_root);
 	element_root = NULL;
-
 	g_static_rw_lock_writer_unlock(&element_lock);
 
 	dbus_connection_unref(connection);
diff --git a/src/main.c b/src/main.c
index 9e237cc..f485a41 100644
--- a/src/main.c
+++ b/src/main.c
@@ -146,16 +146,16 @@ int main(int argc, char *argv[])
 
 	g_main_loop_run(main_loop);
 
-	__connman_plugin_cleanup();
-
-	__connman_manager_cleanup();
-
 	__connman_agent_cleanup();
 
 	__connman_element_cleanup();
 
+	__connman_manager_cleanup();
+
 	__connman_storage_cleanup();
 
+	__connman_plugin_cleanup();
+
 	__connman_log_cleanup();
 
 	g_dbus_cleanup_connection(conn);