Use lists for uevent processing

We now have proper list definitions, so we might as well use them
for uevent processing. And we can clean up signalling, too;
the pthreads condition signalling is unreliable, as a condition
signal might be lost if we're in the middle of processing. So
we should rather check the queue status prior to wait for a
condition.
And we can introduce a pthread cleanup handler, as well.

Signed-off-by: Hannes Reinecke <hare@suse.de>

2 files changed, 66 insertions, 53 deletions

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 0d68390..f5bc668 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -39,17 +39,18 @@
 #include <pthread.h>
 #include <limits.h>
 #include <sys/mman.h>
+#include <errno.h>
 
 #include "memory.h"
 #include "debug.h"
+#include "list.h"
 #include "uevent.h"
 
 typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
 pthread_t uevq_thr;
-struct uevent *uevqhp, *uevqtp;
+LIST_HEAD(uevq);
 pthread_mutex_t uevq_lock, *uevq_lockp = &uevq_lock;
-pthread_mutex_t uevc_lock, *uevc_lockp = &uevc_lock;
 pthread_cond_t  uev_cond,  *uev_condp  = &uev_cond;
 uev_trigger *my_uev_trigger;
 void * my_trigger_data;
@@ -57,7 +58,22 @@ int servicing_uev;
 
 int is_uevent_busy(void)
 {
-	return (uevqhp != NULL || servicing_uev);
+	int empty;
+
+	pthread_mutex_lock(uevq_lockp);
+	empty = list_empty(&uevq);
+	pthread_mutex_unlock(uevq_lockp);
+	return (!empty || servicing_uev);
+}
+
+struct uevent * alloc_uevent (void)
+{
+	struct uevent *uev = MALLOC(sizeof(struct uevent));
+
+	if (uev)
+		INIT_LIST_HEAD(&uev->node);
+
+	return uev;
 }
 
 void
@@ -84,37 +100,31 @@ setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached)
 	}
 }
 
-static struct uevent * alloc_uevent (void)
+/*
+ * Called with uevq_lockp held
+ */
+void
+service_uevq(struct list_head *tmpq)
 {
-	return (struct uevent *)MALLOC(sizeof(struct uevent));
+	struct uevent *uev, *tmp;
+
+	list_for_each_entry_safe(uev, tmp, tmpq, node) {
+		list_del_init(&uev->node);
+
+		if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
+			condlog(0, "uevent trigger error");
+
+		FREE(uev);
+	}
 }
 
-void
-service_uevq(void)
+static void uevq_stop(void *arg)
 {
-	int empty;
-	struct uevent *uev;
-
-	do {
-		pthread_mutex_lock(uevq_lockp);
-		empty = (uevqhp == NULL);
-		if (!empty) {
-			uev = uevqhp;
-			uevqhp = uev->next;
-			if (uevqtp == uev)
-				uevqtp = uev->next;
-			pthread_mutex_unlock(uevq_lockp);
-
-			if (my_uev_trigger && my_uev_trigger(uev,
-							my_trigger_data))
-				condlog(0, "uevent trigger error");
-
-			FREE(uev);
-		}
-		else {
-			pthread_mutex_unlock(uevq_lockp);
-		}
-	} while (empty == 0);
+	condlog(3, "Stopping uev queue");
+	pthread_mutex_lock(uevq_lockp);
+	my_uev_trigger = NULL;
+	pthread_cond_signal(uev_condp);
+	pthread_mutex_unlock(uevq_lockp);
 }
 
 /*
@@ -126,13 +136,23 @@ uevq_thread(void * et)
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 
 	while (1) {
-		pthread_mutex_lock(uevc_lockp);
+		LIST_HEAD(uevq_tmp);
+
+		pthread_mutex_lock(uevq_lockp);
 		servicing_uev = 0;
-		pthread_cond_wait(uev_condp, uevc_lockp);
+		/*
+		 * Condition signals are unreliable,
+		 * so make sure we only wait if we have to.
+		 */
+		if (list_empty(&uevq)) {
+			pthread_cond_wait(uev_condp, uevq_lockp);
+		}
 		servicing_uev = 1;
-		pthread_mutex_unlock(uevc_lockp);
-
-		service_uevq();
+		list_splice_init(&uevq, &uevq_tmp);
+		pthread_mutex_unlock(uevq_lockp);
+		if (!my_uev_trigger)
+			break;
+		service_uevq(&uevq_tmp);
 	}
 	return NULL;
 }
@@ -161,12 +181,12 @@ int uevent_listen(int (*uev_trigger)(struct uevent *, void * trigger_data),
 	 * thereby not getting to empty the socket's receive buffer queue
 	 * often enough.
 	 */
-	uevqhp = uevqtp = NULL;
+	INIT_LIST_HEAD(&uevq);
 
 	pthread_mutex_init(uevq_lockp, NULL);
-	pthread_mutex_init(uevc_lockp, NULL);
 	pthread_cond_init(uev_condp, NULL);
 
+	pthread_cleanup_push(uevq_stop, NULL);
 	setup_thread_attr(&attr, 64 * 1024, 0);
 	pthread_create(&uevq_thr, &attr, uevq_thread, NULL);
 
@@ -267,7 +287,7 @@ int uevent_listen(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		buflen = recvmsg(sock, &smsg, 0);
 		if (buflen < 0) {
 			if (errno != EINTR)
-				condlog(0, "error receiving message");
+				condlog(0, "error receiving message, errno %d", errno);
 			continue;
 		}
 
@@ -295,6 +315,10 @@ int uevent_listen(int (*uev_trigger)(struct uevent *, void * trigger_data),
 			condlog(3, "unrecognized message header");
 			continue;
 		}
+		if ((size_t)buflen > sizeof(buf)-1) {
+			condlog(2, "buffer overflow for received uevent");
+			buflen = sizeof(buf)-1;
+		}
 
 		uev = alloc_uevent();
 
@@ -351,28 +375,17 @@ int uevent_listen(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		 * Queue uevent and poke service pthread.
 		 */
 		pthread_mutex_lock(uevq_lockp);
-		if (uevqtp)
-			uevqtp->next = uev;
-		else
-			uevqhp = uev;
-		uevqtp = uev;
-		uev->next = NULL;
-		pthread_mutex_unlock(uevq_lockp);
-
-		pthread_mutex_lock(uevc_lockp);
+		list_add_tail(&uev->node, &uevq);
 		pthread_cond_signal(uev_condp);
-		pthread_mutex_unlock(uevc_lockp);
+		pthread_mutex_unlock(uevq_lockp);
 	}
 
 exit:
 	close(sock);
 
-	pthread_mutex_lock(uevq_lockp);
-	pthread_cancel(uevq_thr);
-	pthread_mutex_unlock(uevq_lockp);
+	pthread_cleanup_pop(1);
 
 	pthread_mutex_destroy(uevq_lockp);
-	pthread_mutex_destroy(uevc_lockp);
 	pthread_cond_destroy(uev_condp);
 
 	return 1;
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 84f7b5e..8aa46c1 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -14,7 +14,7 @@
 #endif
 
 struct uevent {
-	void *next;
+	struct list_head node;
 	char buffer[HOTPLUG_BUFFER_SIZE + OBJECT_SIZE];
 	char *devpath;
 	char *action;