resource-manager: Add guard clause when removing element during list iteration

When notify callback is underway, removing element from notify list
might mess up the list iteration. Therefore, make it not to remove
list element during iteration. Instead, just mark it as removed and
postpone removal until the entire iteration is completed.

Change-Id: Iea6600be99f8499729d3268a9bd57c70ec7a714d
Signed-off-by: Youngjae Cho <y0.cho@samsung.com>

diff --git a/src/resource-manager/resource-manager.c b/src/resource-manager/resource-manager.c
index 83b9da3..9d1a664 100644 (file)
--- a/src/resource-manager/resource-manager.c
+++ b/src/resource-manager/resource-manager.c
@@ -79,6 +79,8 @@ struct resource_event_data {
        int attr_mask;
        syscommon_resman_resource_event_cb callback;
        void *user_data;
+       int removed;
+       GList *l;
 };
 
 static int set_resource_attr_interest(struct syscommon_resman_resource *resource, u_int64_t interest_mask);
@@ -88,7 +90,10 @@ static void
 notify_resource_event(int resource_type, int resource_id, u_int64_t attr_id, const void *data, int count);
 
 static GList *g_resource_driver_head;
+
 static GList *g_resource_event_data_list;
+static int is_notify_underway = 0;
+static bool need_event_data_cleanup = false;
 
 /**
  * key: resource_id, value: struct syscommon_resman_resource
@@ -1955,17 +1960,59 @@ syscommon_resman_is_resource_attr_interested(int resource_id, u_int64_t interest
        return is_resource_attr_interested(resource, interest_mask);
 }
 
+static void add_resource_event_data(struct resource_event_data *event_data)
+{
+       GList *new_list;
+
+       assert(event_data);
+
+       new_list = g_list_alloc();
+       new_list->data = event_data;
+
+       event_data->l = g_steal_pointer(&new_list);
+
+       g_resource_event_data_list = g_list_concat(g_resource_event_data_list, event_data->l);
+}
+
+static void remove_resource_event_data(struct resource_event_data *event_data)
+{
+       assert(event_data);
+
+       if (is_notify_underway > 0) {
+               event_data->removed = 1;
+               need_event_data_cleanup = true;
+               return;
+       }
+
+       g_resource_event_data_list = g_list_remove_link(g_resource_event_data_list, event_data->l);
+       g_list_free(g_steal_pointer(&event_data->l));
+       free(event_data);
+}
+
+static void cleanup_removed_event_data(gpointer data, gpointer user_data)
+{
+       struct resource_event_data *event_data = (struct resource_event_data *) data;
+
+       if (event_data->removed)
+               remove_resource_event_data(event_data);
+}
+
 static void
 notify_resource_event(int resource_type, int resource_id, u_int64_t attr_id, const void *data, int count)
 {
        GList *elem;
        struct resource_event_data *event_data;
 
+       ++is_notify_underway;
+
        for (elem = g_resource_event_data_list; elem; elem = elem->next) {
                event_data = (struct resource_event_data *) elem->data;
 
                assert(event_data);
 
+               if (event_data->removed)
+                       continue;
+
                if ((event_data->resource_type != RESOURCE_TYPE_ALL) && (event_data->resource_type != resource_type))
                        continue;
 
@@ -1974,6 +2021,17 @@ notify_resource_event(int resource_type, int resource_id, u_int64_t attr_id, con
 
                event_data->callback(resource_type, resource_id, attr_id, data, event_data->user_data, count);
        }
+
+       if (--is_notify_underway > 0)
+               return;
+
+       if (!need_event_data_cleanup)
+               return;
+
+       /* cleanup removed event_data during callback */
+       g_list_foreach(g_resource_event_data_list, cleanup_removed_event_data, NULL);
+
+       need_event_data_cleanup = false;
 }
 
 static gboolean find_resource_event_data(gconstpointer data, gconstpointer id)
@@ -2004,7 +2062,7 @@ int syscommon_resman_subscribe_resource_attribute_event(int resource_type,
                .user_data = user_data,
        };
 
-       g_resource_event_data_list = g_list_append(g_resource_event_data_list, event_data);
+       add_resource_event_data(event_data);
 
        *id = event_data->id;
 
@@ -2032,11 +2090,7 @@ void syscommon_resman_unsubscribe_event(int id)
        if (!list)
                return;
 
-       g_resource_event_data_list = g_list_remove_link(g_resource_event_data_list, list);
-
-       free(list->data);
-       list->data = NULL;
-       g_list_free(list);
+       remove_resource_event_data(list->data);
 }
 
 const char *

diff --git a/tests/resource-manager/test.c b/tests/resource-manager/test.c
index a8bc52a..aac81ff 100644 (file)
--- a/tests/resource-manager/test.c
+++ b/tests/resource-manager/test.c
@@ -9,6 +9,8 @@
 
 #include "../test-main.h"
 
+#define MAGIC_UNSUBSCRIBE      (2147483629)
+
 static int setup_resource_drivers(void **state)
 {
        setup_display_resource_driver();
@@ -393,6 +395,24 @@ static void test_event_callback2(int resource_type, int resource_id, int attr_id
        check_expected(count);
 }
 
+static void test_event_callback_unsubscribe(int resource_type, int resource_id, int attr_id, const void *data, void *user_data, int count)
+{
+       int callback_id;
+       int value;
+
+       check_expected(resource_type);
+       check_expected(resource_id);
+       check_expected(attr_id);
+       check_expected_ptr(data);
+       check_expected(count);
+
+       value = *(const int *) data;
+       if (value == MAGIC_UNSUBSCRIBE) {
+               callback_id = *(int *) user_data;
+               syscommon_resman_unsubscribe_event(callback_id);
+       }
+}
+
 static void test_subscribe_event_pass(void **state)
 {
        int resource_id1;
@@ -671,6 +691,91 @@ static void test_subscribe_resource_attribute_event_fail_invalid_parameter_2(voi
        assert_int_equal(ret, -EINVAL);
 }
 
+static void test_unsubscribe_event_during_callback_pass(void **state)
+{
+       int display_resource_id;
+       int callback_id1;
+       int callback_id2;
+       int int_value = 478478;
+       int ret;
+
+       display_resource_id = create_display_resource();
+
+       /**
+        * The callback test_event_callback_unsubscribe() will unsubscribe itself
+        * on setting DISPLAY_ATTR_INT to MAGIC_UNSUBSCRIBE
+        */
+       ret = syscommon_resman_subscribe_resource_attribute_event(TEST_RESOURCE_TYPE_DISPLAY,
+               DISPLAY_ATTR_INT, test_event_callback_unsubscribe,
+               &callback_id1, /* unsubscribed on setting DISPLAY_ATTR_INT to MAGIC_UNSUBSCRIBE */
+               &callback_id1);
+       assert_int_equal(ret, 0);
+
+       ret = syscommon_resman_subscribe_resource_attribute_event(TEST_RESOURCE_TYPE_DISPLAY,
+               DISPLAY_ATTR_INT, test_event_callback2, NULL, &callback_id2);
+       assert_int_equal(ret, 0);
+
+       /* configure mock setter, success */
+       expect_memory(test_display_set, data, &int_value, sizeof(int));
+       will_return(test_display_set, 0);
+       /* configure test_event_callback_unsubscribe() */
+       expect_value(test_event_callback_unsubscribe, resource_type, TEST_RESOURCE_TYPE_DISPLAY);
+       expect_value(test_event_callback_unsubscribe, resource_id, display_resource_id);
+       expect_value(test_event_callback_unsubscribe, attr_id, DISPLAY_ATTR_INT);
+       expect_memory(test_event_callback_unsubscribe, data, &int_value, sizeof(int));
+       expect_value(test_event_callback_unsubscribe, count, 1);
+       /* configure test_event_callback2() */
+       expect_value(test_event_callback2, resource_type, TEST_RESOURCE_TYPE_DISPLAY);
+       expect_value(test_event_callback2, resource_id, display_resource_id);
+       expect_value(test_event_callback2, attr_id, DISPLAY_ATTR_INT);
+       expect_memory(test_event_callback2, data, &int_value, sizeof(int));
+       expect_value(test_event_callback2, count, 1);
+
+       ret = syscommon_resman_set_resource_attr_int(display_resource_id, DISPLAY_ATTR_INT, int_value);
+       assert_int_equal(ret, 0);
+
+       /* now we expect the test_event_callback_unsubscribe() to remove iteslf */
+       int_value = MAGIC_UNSUBSCRIBE;
+
+       /* configure mock setter, success */
+       expect_memory(test_display_set, data, &int_value, sizeof(int));
+       will_return(test_display_set, 0);
+       /* configure test_event_callback_unsubscribe() */
+       expect_value(test_event_callback_unsubscribe, resource_type, TEST_RESOURCE_TYPE_DISPLAY);
+       expect_value(test_event_callback_unsubscribe, resource_id, display_resource_id);
+       expect_value(test_event_callback_unsubscribe, attr_id, DISPLAY_ATTR_INT);
+       expect_memory(test_event_callback_unsubscribe, data, &int_value, sizeof(int));
+       expect_value(test_event_callback_unsubscribe, count, 1);
+       /* configure test_event_callback2() */
+       expect_value(test_event_callback2, resource_type, TEST_RESOURCE_TYPE_DISPLAY);
+       expect_value(test_event_callback2, resource_id, display_resource_id);
+       expect_value(test_event_callback2, attr_id, DISPLAY_ATTR_INT);
+       expect_memory(test_event_callback2, data, &int_value, sizeof(int));
+       expect_value(test_event_callback2, count, 1);
+
+       ret = syscommon_resman_set_resource_attr_int(display_resource_id, DISPLAY_ATTR_INT, int_value);
+       assert_int_equal(ret, 0);
+
+       int_value = -10;
+
+       /* configure mock setter, success */
+       expect_memory(test_display_set, data, &int_value, sizeof(int));
+       will_return(test_display_set, 0);
+       /* do not configure test_event_callback_unsubscribe() as it must have been removed */
+       /* configure test_event_callback2() */
+       expect_value(test_event_callback2, resource_type, TEST_RESOURCE_TYPE_DISPLAY);
+       expect_value(test_event_callback2, resource_id, display_resource_id);
+       expect_value(test_event_callback2, attr_id, DISPLAY_ATTR_INT);
+       expect_memory(test_event_callback2, data, &int_value, sizeof(int));
+       expect_value(test_event_callback2, count, 1);
+
+       ret = syscommon_resman_set_resource_attr_int(display_resource_id, DISPLAY_ATTR_INT, int_value);
+       assert_int_equal(ret, 0);
+
+       syscommon_resman_unsubscribe_event(callback_id1);
+       delete_display_resource(display_resource_id);
+}
+
 static const struct CMUnitTest resource_manager_testsuite[] = {
        cmocka_unit_test(test_create_resource_pass),
        cmocka_unit_test(test_create_resource_fail_driver_ops_create),
@@ -709,5 +814,7 @@ static const struct CMUnitTest resource_manager_testsuite[] = {
        cmocka_unit_test(test_subscribe_resource_attribute_event_pass),
        cmocka_unit_test(test_subscribe_resource_attribute_event_fail_invalid_parameter_1),
        cmocka_unit_test(test_subscribe_resource_attribute_event_fail_invalid_parameter_2),
+
+       cmocka_unit_test(test_unsubscribe_event_during_callback_pass),
 };
 TESTSUITE_FIXTURE(resource_manager_testsuite, setup_resource_drivers, NULL);