Merge lp:~sforshee/powerd/worker into lp:powerd

[Seth Forshee]

* Add powerd_run_mainloop_sync() for running a task on the main loop and waiting for it to complete
* Convert system request code to use powerd_run_mainloop_sync() rather than using mutexes

* Add powerd_run_mainloop_sync() for running a task on the main loop and waiting for it to complete
* Convert system request code to use powerd_run_mainloop_sync() rather than using mutexes

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:36
http://jenkins.qa.ubuntu.com/job/powerd-ci/53/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/powerd-saucy-armhf-ci/7
        deb: http://jenkins.qa.ubuntu.com/job/powerd-saucy-armhf-ci/7/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/powerd-ci/53/rebuild

review: Approve (continuous-integration)

[Matt Fischer (mfisch)]

Approved per IRC discussion. Coders need to avoid making the calls per comment and since the functions are static, I'm confident that other coders of powerd will respect that. We did discuss making a check function but it might be wasteful perf wise.

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2013-06-06 21:25:50 +0000
+++ CMakeLists.txt	2013-06-10 18:15:31 +0000
@@ -38,6 +38,7 @@
   src/powerd-object.c
   src/power-request.c
   src/powerd-sensors.cpp
+  src/util.c
   src/${GDBUS_NAME}.c
 )
 

=== modified file 'src/power-request.c'
--- src/power-request.c	2013-06-04 20:07:30 +0000
+++ src/power-request.c	2013-06-10 18:15:31 +0000
@@ -60,15 +60,6 @@
 static gint state_request_count[POWERD_NUM_POWER_STATES - 1];
 
 /*
- * Mutex to protect code accessing state_request_hash
- *
- * XXX: Try to move all access to main loop to eliminate need for mutex.
- * Dbus calls are already processed from main loop, just need to move
- * internal requests to be made there as well.
- */
-static GMutex state_request_mutex;
-
-/*
  * States for "state transition" state machine.
  *
  * When no state transition is in progress we're in the idle state. When
@@ -179,19 +170,21 @@
         g_variant_builder_add(builder, "(sii)", sr->owner, sr->pid, sr->state);
 }
 
-/* internal callers should pass in NULL for the builder, its only used
+/*
+ * Must only be called from main loop.
+ *
+ * internal callers should pass in NULL for the builder, its only used
  * by dbus callers. Returns a count of the total number of active
- * requests. */
+ * requests.
+ */
 static guint
 list_sys_requests_internal(GVariantBuilder *builder)
 {
     guint count = 0;
 
-    g_mutex_lock(&state_request_mutex);
     g_message("System state requests:");
     g_hash_table_foreach(state_request_hash, list_request, builder);
     count = g_hash_table_size(state_request_hash);
-    g_mutex_unlock(&state_request_mutex);
 
     return count;
 }
@@ -227,6 +220,15 @@
     return memcmp(sr_a->cookie, sr_b->cookie, sizeof(sr_a->cookie));
 }
 
+/* Should be called only on main loop */
+static int request_sys_state_worker(gpointer data)
+{
+    struct SysStateRequest *sr = data;
+    g_hash_table_insert(state_request_hash, sr->cookie, sr);
+    state_request_count[sr->state - 1] += 1;
+    return 0;
+}
+
 /* Note: owner is NULL for internal usage */
 gboolean
 request_sys_state_internal(int state, int pid, uuid_t *cookie, const gchar *owner)
@@ -257,11 +259,7 @@
     uuid_generate(sr->cookie);
     memcpy(*cookie, sr->cookie, sizeof(sr->cookie));
 
-    g_mutex_lock(&state_request_mutex);
-    g_hash_table_insert(state_request_hash, sr->cookie, sr);
-    state_request_count[state - 1] += 1;
-    g_mutex_unlock(&state_request_mutex);
-
+    powerd_run_mainloop_sync(request_sys_state_worker, sr);
     update_system_state();
     return TRUE;
 }
@@ -291,14 +289,13 @@
     return TRUE;
 }
 
-gboolean
-clear_sys_state_internal(uuid_t cookie)
+/* Must only be called from main loop */
+static int clear_sys_state_worker(gpointer data)
 {
+    unsigned char *uuid = data;
     struct SysStateRequest *sr;
-    gboolean found = FALSE;
-    char cookie_str[UUID_STR_LEN];
-
-    g_mutex_lock(&state_request_mutex);
+    int found = FALSE;
+    enum SysPowerStates state;
 
     /*
      * This involves two lookups into the hash, one to find the
@@ -307,21 +304,29 @@
      * way to do this; too bad g_hash_table_steal() doesn't return
      * a pointer to the data.
      */
-    sr = g_hash_table_lookup(state_request_hash, cookie);
+    sr = g_hash_table_lookup(state_request_hash, uuid);
     if (sr) {
-        enum SysPowerStates state = sr->state;
-        found = g_hash_table_remove(state_request_hash, cookie);
+        state = sr->state;
+        found = g_hash_table_remove(state_request_hash, uuid);
         if (!found)
             g_warning("State request found on lookup but not on remove");
         else
             state_request_count[state - 1] -= 1;
     }
-    g_mutex_unlock(&state_request_mutex);
-
+
+    return found;
+}
+
+gboolean
+clear_sys_state_internal(uuid_t cookie)
+{
+    char cookie_str[UUID_STR_LEN];
+    int found;
+
+    found = powerd_run_mainloop_sync(clear_sys_state_worker, cookie);
     if (!found) {
         uuid_unparse(cookie, cookie_str);
         g_warning("request (%s) not found", cookie_str);
-        g_mutex_unlock(&state_request_mutex);
         return FALSE;
     }
 
@@ -357,19 +362,18 @@
     return TRUE;
 }
 
+/* Must only be called from main loop */
 static enum SysPowerStates max_requested_state(void)
 {
     enum SysPowerStates ret = POWERD_SYS_STATE_SUSPEND;
     int i;
 
-    g_mutex_lock(&state_request_mutex);
     for (i = POWERD_NUM_POWER_STATES - 1; i > POWERD_SYS_STATE_SUSPEND; i--) {
         if (state_request_count[i - 1] != 0) {
             ret = i;
             break;
         }
     };
-    g_mutex_unlock(&state_request_mutex);
 
     return ret;
 }

=== modified file 'src/powerd-internal.h'
--- src/powerd-internal.h	2013-05-31 13:53:47 +0000
+++ src/powerd-internal.h	2013-06-10 18:15:31 +0000
@@ -41,6 +41,7 @@
 void powerd_exit(void);
 void powerd_reset_activity_timer(int add);
 void powerd_dbus_init_complete(void);
+int powerd_is_mainloop(void);
 
 /* Display functions */
 void powerd_brightness_set_value(gint value);
@@ -83,6 +84,9 @@
 void ofono_proxy_connect_cb(GObject *source_object, GAsyncResult *res,
     gpointer user_data);
 
+/* Utility functions */
+int powerd_run_mainloop_sync(int (*func)(gpointer), gpointer data);
+
 #ifdef __cplusplus
 }
 #endif

=== modified file 'src/powerd.cpp'
--- src/powerd.cpp	2013-06-05 19:13:42 +0000
+++ src/powerd.cpp	2013-06-10 18:15:31 +0000
@@ -47,6 +47,8 @@
 
 #include "libsuspend.h"
 
+static GThread *powerd_mainloop_thread;
+
 namespace
 {
 
@@ -215,6 +217,20 @@
     return g_variant_get_uint32(val);
 }
 
+/* Must be first to run on main loop */
+static gboolean main_init(gpointer unused)
+{
+    powerd_mainloop_thread = g_thread_self();
+    return FALSE;
+}
+
+/* Returns TRUE if current thread of execution is the default main loop */
+int powerd_is_mainloop(void)
+{
+    return (g_thread_self() == powerd_mainloop_thread);
+}
+
+
 int main(int argc, char** argv)
 {
     int i, ret;
@@ -267,6 +283,9 @@
     android_input_stack_initialize(&listener, &config);
     android_input_stack_start();
 
+    /* This needs to be the first thing to run on the main loop */
+    g_idle_add_full(G_PRIORITY_HIGH, main_init, NULL, NULL);
+
     powerd_reset_activity_timer(1);
 
     g_main_loop_run(main_loop);

=== added file 'src/util.c'
--- src/util.c	1970-01-01 00:00:00 +0000
+++ src/util.c	2013-06-10 18:15:31 +0000
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2013 Canonical Ltd.
+ *
+ * This file is part of powerd.
+ *
+ * powerd is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * powerd is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <glib.h>
+
+struct sync_work_data {
+    GCond cond;
+    GMutex mutex;
+    gboolean done;
+    int (*func)(gpointer);
+    gpointer data;
+    int return_val;
+};
+
+static gboolean run_mainloop_sync_worker(gpointer data)
+{
+    struct sync_work_data *work_data = data;
+
+    g_mutex_lock(&work_data->mutex);
+    work_data->return_val = work_data->func(work_data->data);
+    work_data->done = TRUE;
+    g_cond_signal(&work_data->cond);
+    g_mutex_unlock(&work_data->mutex);
+
+    return FALSE;
+}
+
+static int __powerd_run_mainloop_sync(int (*func)(gpointer), gpointer data)
+{
+    struct sync_work_data work_data;
+
+    g_cond_init(&work_data.cond);
+    g_mutex_init(&work_data.mutex);
+    work_data.done = FALSE;
+    work_data.func = func;
+    work_data.data = data;
+
+    g_mutex_lock(&work_data.mutex);
+    g_timeout_add(0, run_mainloop_sync_worker, &work_data);
+    while (!work_data.done)
+        g_cond_wait(&work_data.cond, &work_data.mutex);
+    g_mutex_unlock(&work_data.mutex);
+
+    g_mutex_clear(&work_data.mutex);
+    g_cond_clear(&work_data.cond);
+
+    return work_data.return_val;
+}
+
+/*
+ * Run the supplied function on the main loop, passing it the
+ * supplied data, and wait for it to complete. If currently
+ * executing on the main loop then the function is simply called
+ * inline.
+ */
+int powerd_run_mainloop_sync(int (*func)(gpointer), gpointer data)
+{
+    if (powerd_is_mainloop())
+        return func(data);
+    return __powerd_run_mainloop_sync(func, data);
+}