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>

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 0d68390..f5bc668 100644 (file)
--- 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 (file)
--- 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;