From 0e34df58e5aff5b9fffb191a412d330729688af9 Mon Sep 17 00:00:00 2001 From: Olivier Martin Date: Fri, 5 Apr 2024 13:25:51 +0200 Subject: [PATCH] Single lock for all gattlib library --- common/gattlib_callback_connected_device.c | 27 +- common/gattlib_callback_disconnected_device.c | 25 +- common/gattlib_callback_discovered_device.c | 24 +- common/gattlib_callback_notification_device.c | 9 +- common/gattlib_common.c | 78 ++++-- common/gattlib_common_adapter.c | 108 ++++++++ common/gattlib_device_state_management.c | 75 +++--- common/gattlib_internal.h | 55 ++-- dbus/CMakeLists.txt | 1 + dbus/gattlib.c | 231 +++++++++++++---- dbus/gattlib_adapter.c | 239 +++++++++++++----- dbus/gattlib_advertisement.c | 35 ++- dbus/gattlib_backend.h | 10 +- dbus/gattlib_char.c | 66 ++++- dbus/gattlib_notification.c | 68 ++++- dbus/gattlib_stream.c | 8 +- 16 files changed, 826 insertions(+), 233 deletions(-) create mode 100644 common/gattlib_common_adapter.c diff --git a/common/gattlib_callback_connected_device.c b/common/gattlib_callback_connected_device.c index 329c7c0..055d491 100644 --- a/common/gattlib_callback_connected_device.c +++ b/common/gattlib_callback_connected_device.c @@ -50,22 +50,31 @@ static gpointer _gattlib_connected_device_thread(gpointer data) { gattlib_connection_t* connection = data; const gchar *device_mac_address = org_bluez_device1_get_address(connection->backend.device); - // Mutex to ensure the device is valid and not freed during its use - g_mutex_lock(&connection->device->device_mutex); // Mutex to ensure the handler is valid - g_rec_mutex_lock(&connection->on_connection.mutex); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(connection)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return NULL; + } if (!gattlib_has_valid_handler(&connection->on_connection)) { - goto EXIT; + g_rec_mutex_unlock(&m_gattlib_mutex); + return NULL; } + // Ensure we increment device reference counter to prevent the device/connection is freed during the execution + gattlib_device_ref(connection->device); + + // We need to release the lock here to ensure the connection callback that is actually + // doing the application sepcific work is not locking the BLE state. + g_rec_mutex_unlock(&m_gattlib_mutex); + connection->on_connection.callback.connection_handler( connection->device->adapter, device_mac_address, connection, 0 /* no error */, connection->on_connection.user_data); -EXIT: - g_rec_mutex_unlock(&connection->on_connection.mutex); - g_mutex_unlock(&connection->device->device_mutex); + gattlib_device_unref(connection->device); return NULL; } @@ -75,6 +84,10 @@ static void* _connected_device_thread_args_allocator(va_list args) { } void gattlib_on_connected_device(gattlib_connection_t* connection) { + if (!gattlib_device_is_valid(connection->device)) { + return; + } + gattlib_handler_dispatch_to_thread( &connection->on_connection, #if defined(WITH_PYTHON) diff --git a/common/gattlib_callback_disconnected_device.c b/common/gattlib_callback_disconnected_device.c index ae6f51b..f92b507 100644 --- a/common/gattlib_callback_disconnected_device.c +++ b/common/gattlib_callback_disconnected_device.c @@ -31,9 +31,14 @@ void gattlib_disconnected_device_python_callback(gattlib_connection_t* connectio #endif void gattlib_on_disconnected_device(gattlib_connection_t* connection) { - if (gattlib_has_valid_handler(&connection->on_disconnection)) { - g_rec_mutex_lock(&connection->on_disconnection.mutex); + g_rec_mutex_lock(&m_gattlib_mutex); + if (!gattlib_connection_is_connected(connection)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return; + } + + if (gattlib_has_valid_handler(&connection->on_disconnection)) { #if defined(WITH_PYTHON) // Check if we are using the Python callback, in case of Python argument we keep track of the argument to free them // once we are done with the handler. @@ -44,16 +49,16 @@ void gattlib_on_disconnected_device(gattlib_connection_t* connection) { // For GATT disconnection we do not use thread to ensure the callback is synchronous. connection->on_disconnection.callback.disconnection_handler(connection, connection->on_disconnection.user_data); - - g_rec_mutex_unlock(&connection->on_disconnection.mutex); } - // Signal the device is now disconnected - g_mutex_lock(&connection->device->device_mutex); - connection->disconnection_wait.value = true; - g_cond_broadcast(&connection->disconnection_wait.condition); - g_mutex_unlock(&connection->device->device_mutex); - // Clean GATTLIB connection on disconnection gattlib_connection_free(connection); + + g_rec_mutex_unlock(&m_gattlib_mutex); + + // Signal the device is now disconnected + g_mutex_lock(&m_gattlib_signal.mutex); + m_gattlib_signal.signals |= GATTLIB_SIGNAL_DEVICE_DISCONNECTION; + g_cond_broadcast(&m_gattlib_signal.condition); + g_mutex_unlock(&m_gattlib_signal.mutex); } diff --git a/common/gattlib_callback_discovered_device.c b/common/gattlib_callback_discovered_device.c index ff2e822..dc552d1 100644 --- a/common/gattlib_callback_discovered_device.c +++ b/common/gattlib_callback_discovered_device.c @@ -55,21 +55,33 @@ struct gattlib_discovered_device_thread_args { static gpointer _gattlib_discovered_device_thread(gpointer data) { struct gattlib_discovered_device_thread_args* args = data; - g_rec_mutex_lock(&args->gattlib_adapter->discovered_device_callback.mutex); + g_rec_mutex_lock(&m_gattlib_mutex); - if (!gattlib_has_valid_handler(&args->gattlib_adapter->discovered_device_callback)) { + if (!gattlib_adapter_is_valid(args->gattlib_adapter)) { + g_rec_mutex_unlock(&m_gattlib_mutex); goto EXIT; } + if (!gattlib_has_valid_handler(&args->gattlib_adapter->discovered_device_callback)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + goto EXIT; + } + + // Increase adapter reference counter to ensure the adapter is not freed while + // the callback is in use. + gattlib_adapter_ref(args->gattlib_adapter); + + g_rec_mutex_unlock(&m_gattlib_mutex); + args->gattlib_adapter->discovered_device_callback.callback.discovered_device( args->gattlib_adapter, args->mac_address, args->name, args->gattlib_adapter->discovered_device_callback.user_data ); -EXIT: - g_rec_mutex_unlock(&args->gattlib_adapter->discovered_device_callback.mutex); + gattlib_adapter_unref(args->gattlib_adapter); +EXIT: free(args->mac_address); if (args->name != NULL) { free(args->name); @@ -96,6 +108,10 @@ static void* _discovered_device_thread_args_allocator(va_list args) { } void gattlib_on_discovered_device(gattlib_adapter_t* gattlib_adapter, OrgBluezDevice1* device1) { + if (!gattlib_adapter_is_valid(gattlib_adapter)) { + return; + } + gattlib_handler_dispatch_to_thread( &gattlib_adapter->discovered_device_callback, #if defined(WITH_PYTHON) diff --git a/common/gattlib_callback_notification_device.c b/common/gattlib_callback_notification_device.c index 66cc6fc..3706225 100644 --- a/common/gattlib_callback_notification_device.c +++ b/common/gattlib_callback_notification_device.c @@ -56,14 +56,19 @@ void gattlib_notification_device_thread(gpointer data, gpointer user_data) { struct gattlib_notification_device_thread_args* args = data; struct gattlib_handler* handler = user_data; - g_rec_mutex_lock(&handler->mutex); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(args->connection)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return; + } handler->callback.notification_handler( args->uuid, args->data, args->data_length, handler->user_data ); - g_rec_mutex_unlock(&handler->mutex); + g_rec_mutex_unlock(&m_gattlib_mutex); if (args->uuid != NULL) { free(args->uuid); diff --git a/common/gattlib_common.c b/common/gattlib_common.c index f6b8623..c708944 100644 --- a/common/gattlib_common.c +++ b/common/gattlib_common.c @@ -8,11 +8,21 @@ #include "gattlib_internal.h" + int gattlib_register_notification(gattlib_connection_t* connection, gattlib_event_handler_t notification_handler, void* user_data) { GError *error = NULL; + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); if (connection == NULL) { - return GATTLIB_INVALID_PARAMETER; + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + + if (!gattlib_device_is_valid(connection->device)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; } connection->notification.callback.notification_handler = notification_handler; @@ -25,19 +35,33 @@ int gattlib_register_notification(gattlib_connection_t* connection, gattlib_even if (error != NULL) { GATTLIB_LOG(GATTLIB_ERROR, "gattlib_register_notification: Failed to create thread pool: %s", error->message); g_error_free(error); - return GATTLIB_ERROR_INTERNAL; + ret = GATTLIB_ERROR_INTERNAL; + goto EXIT; } else { assert(connection->notification.thread_pool != NULL); - return GATTLIB_SUCCESS; } + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } int gattlib_register_indication(gattlib_connection_t* connection, gattlib_event_handler_t indication_handler, void* user_data) { GError *error = NULL; + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); if (connection == NULL) { - return GATTLIB_INVALID_PARAMETER; + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; } + + if (!gattlib_device_is_valid(connection->device)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + connection->indication.callback.notification_handler = indication_handler; connection->indication.user_data = user_data; @@ -48,19 +72,36 @@ int gattlib_register_indication(gattlib_connection_t* connection, gattlib_event_ if (error != NULL) { GATTLIB_LOG(GATTLIB_ERROR, "gattlib_register_indication: Failed to create thread pool: %s", error->message); g_error_free(error); - return GATTLIB_ERROR_INTERNAL; - } else { - return GATTLIB_SUCCESS; + ret = GATTLIB_ERROR_INTERNAL; + goto EXIT; } + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } int gattlib_register_on_disconnect(gattlib_connection_t *connection, gattlib_disconnection_handler_t handler, void* user_data) { + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); + if (connection == NULL) { - return GATTLIB_INVALID_PARAMETER; + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; } + + if (!gattlib_device_is_valid(connection->device)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + connection->on_disconnection.callback.disconnection_handler = handler; connection->on_disconnection.user_data = user_data; - return GATTLIB_SUCCESS; + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } void bt_uuid_to_uuid(bt_uuid_t* bt_uuid, uuid_t* uuid) { @@ -144,10 +185,8 @@ int gattlib_uuid_cmp(const uuid_t *uuid1, const uuid_t *uuid2) { } void gattlib_handler_free(struct gattlib_handler* handler) { - g_rec_mutex_lock(&handler->mutex); - if (!gattlib_has_valid_handler(handler)) { - goto EXIT; + return; } // Reset callback to stop calling it after we stopped @@ -172,9 +211,6 @@ void gattlib_handler_free(struct gattlib_handler* handler) { g_thread_pool_free(handler->thread_pool, FALSE /* immediate */, TRUE /* wait */); handler->thread_pool = NULL; } - -EXIT: - g_rec_mutex_unlock(&handler->mutex); } bool gattlib_has_valid_handler(struct gattlib_handler* handler) { @@ -185,8 +221,11 @@ void gattlib_handler_dispatch_to_thread(struct gattlib_handler* handler, void (* GThreadFunc thread_func, const char* thread_name, void* (*thread_args_allocator)(va_list args), ...) { GError *error = NULL; + g_rec_mutex_lock(&m_gattlib_mutex); + if (!gattlib_has_valid_handler(handler)) { // We do not have (anymore) a callback, nothing to do + g_rec_mutex_unlock(&m_gattlib_mutex); return; } @@ -198,6 +237,8 @@ void gattlib_handler_dispatch_to_thread(struct gattlib_handler* handler, void (* } #endif + g_rec_mutex_unlock(&m_gattlib_mutex); + // We create a thread to ensure the callback is not blocking the mainloop va_list args; va_start(args, thread_args_allocator); @@ -218,3 +259,10 @@ void gattlib_free_mem(void *ptr) { free(ptr); } } + +int gattlib_device_ref(gattlib_device_t* device) { + g_rec_mutex_lock(&m_gattlib_mutex); + device->reference_counter++; + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_SUCCESS; +} diff --git a/common/gattlib_common_adapter.c b/common/gattlib_common_adapter.c new file mode 100644 index 0000000..2a514cd --- /dev/null +++ b/common/gattlib_common_adapter.c @@ -0,0 +1,108 @@ +/* + * SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-or-later + * + * Copyright (c) 2021-2024, Olivier Martin + */ + +#include "gattlib_internal.h" + +// Keep track of the allocated adapters to avoid an adapter to be freed twice. +// It could happen when using Python wrapper. +GSList *m_adapter_list; + +bool gattlib_adapter_is_valid(gattlib_adapter_t* adapter) { + bool is_valid; + + g_rec_mutex_lock(&m_gattlib_mutex); + + GSList *adapter_entry = g_slist_find(m_adapter_list, adapter); + if (adapter_entry == NULL) { + is_valid = false; + } else { + is_valid = true; + } + + g_rec_mutex_unlock(&m_gattlib_mutex); + return is_valid; +} + +bool gattlib_adapter_is_scanning(gattlib_adapter_t* adapter) { + bool is_scanning = false; + + g_rec_mutex_lock(&m_gattlib_mutex); + + GSList *adapter_entry = g_slist_find(m_adapter_list, adapter); + if (adapter_entry == NULL) { + goto EXIT; + } + + is_scanning = adapter->backend.ble_scan.is_scanning; + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return is_scanning; +} + +struct _device_is_valid { + gattlib_device_t* device; + bool found; +}; + +static void _gattlib_device_is_valid(gpointer data, gpointer user_data) { + gattlib_adapter_t* adapter = data; + struct _device_is_valid* device_is_valid = user_data; + + GSList *device_entry = g_slist_find(adapter->devices, device_is_valid->device); + if (device_entry != NULL) { + device_is_valid->found = true; + } +} + +bool gattlib_device_is_valid(gattlib_device_t* device) { + struct _device_is_valid device_is_valid = { + .device = device, + .found = false + }; + + g_rec_mutex_lock(&m_gattlib_mutex); + g_slist_foreach(m_adapter_list, _gattlib_device_is_valid, &device_is_valid); + g_rec_mutex_unlock(&m_gattlib_mutex); + + return device_is_valid.found; +} + +struct _connection_is_connected { + gattlib_connection_t* connection; + bool is_connected; +}; + +static gint _is_device_connection(gconstpointer a, gconstpointer b) { + const gattlib_device_t* device = a; + return (&device->connection == b); +} + +static void _gattlib_connection_is_connected(gpointer data, gpointer user_data) { + gattlib_adapter_t* adapter = data; + struct _connection_is_connected* connection_is_connected = user_data; + + GSList *device_entry = g_slist_find_custom(adapter->devices, user_data, _is_device_connection); + if (device_entry == NULL) { + return; + } + + gattlib_device_t* device = device_entry->data; + connection_is_connected->is_connected = (device->state == CONNECTED); +} + +bool gattlib_connection_is_connected(gattlib_connection_t* connection) { + struct _connection_is_connected connection_is_connected = { + .connection = connection, + .is_connected = false + }; + + g_rec_mutex_lock(&m_gattlib_mutex); + g_slist_foreach(m_adapter_list, _gattlib_connection_is_connected, &connection_is_connected); + g_rec_mutex_unlock(&m_gattlib_mutex); + + return connection_is_connected.is_connected; +} diff --git a/common/gattlib_device_state_management.c b/common/gattlib_device_state_management.c index aa79e4e..550da0d 100644 --- a/common/gattlib_device_state_management.c +++ b/common/gattlib_device_state_management.c @@ -8,10 +8,10 @@ const char* device_state_str[] = { "NOT_FOUND", - "CONNECTING", - "CONNECTED", - "DISCONNECTING", - "DISCONNECTED" + "CONNECTING", + "CONNECTED", + "DISCONNECTING", + "DISCONNECTED" }; static gint _compare_device_with_device_id(gconstpointer a, gconstpointer b) { @@ -26,41 +26,33 @@ static GSList* _find_device_with_device_id(gattlib_adapter_t* adapter, const cha } gattlib_device_t* gattlib_device_get_device(gattlib_adapter_t* adapter, const char* device_id) { - gattlib_device_t* device = NULL; - - g_rec_mutex_lock(&adapter->mutex); - GSList *item = _find_device_with_device_id(adapter, device_id); if (item == NULL) { - goto EXIT; + return NULL; } - device = (gattlib_device_t*)item->data; - -EXIT: - g_rec_mutex_unlock(&adapter->mutex); - return device; + return (gattlib_device_t*)item->data; } enum _gattlib_device_state gattlib_device_get_state(gattlib_adapter_t* adapter, const char* device_id) { - enum _gattlib_device_state state = NOT_FOUND; - - g_rec_mutex_lock(&adapter->mutex); - gattlib_device_t* device = gattlib_device_get_device(adapter, device_id); - if (device != NULL) { - state = device->state; + if (device == NULL) { + return NOT_FOUND; } - g_rec_mutex_unlock(&adapter->mutex); - return state; + return device->state; } int gattlib_device_set_state(gattlib_adapter_t* adapter, const char* device_id, enum _gattlib_device_state new_state) { enum _gattlib_device_state old_state; int ret = GATTLIB_SUCCESS; - g_rec_mutex_lock(&adapter->mutex); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } old_state = gattlib_device_get_state(adapter, device_id); if (old_state == NOT_FOUND) { @@ -77,6 +69,7 @@ int gattlib_device_set_state(gattlib_adapter_t* adapter, const char* device_id, GATTLIB_LOG(GATTLIB_DEBUG, "gattlib_device_set_state:%s: Set initial state %s", device_id, device_state_str[new_state]); + device->reference_counter = 1; device->adapter = adapter; device->device_id = g_strdup(device_id); device->state = new_state; @@ -103,7 +96,7 @@ int gattlib_device_set_state(gattlib_adapter_t* adapter, const char* device_id, case DISCONNECTED: GATTLIB_LOG(GATTLIB_DEBUG, "gattlib_device_set_state: Free device %p", device); adapter->devices = g_slist_remove(adapter->devices, device); - free(device); + gattlib_device_unref(device); break; case CONNECTING: GATTLIB_LOG(GATTLIB_DEBUG, "gattlib_device_set_state: Connecting device needs to be removed - ignore it"); @@ -130,7 +123,7 @@ int gattlib_device_set_state(gattlib_adapter_t* adapter, const char* device_id, } EXIT: - g_rec_mutex_unlock(&adapter->mutex); + g_rec_mutex_unlock(&m_gattlib_mutex); return ret; } @@ -139,7 +132,7 @@ static void _gattlib_device_free(gpointer data) { switch (device->state) { case DISCONNECTED: - free(device); + gattlib_device_unref(device); break; default: GATTLIB_LOG(GATTLIB_WARNING, "Memory of the BLE device '%s' has not been freed because in state %s", @@ -148,13 +141,24 @@ static void _gattlib_device_free(gpointer data) { } int gattlib_devices_free(gattlib_adapter_t* adapter) { - g_rec_mutex_lock(&adapter->mutex); g_slist_free_full(adapter->devices, _gattlib_device_free); - g_rec_mutex_unlock(&adapter->mutex); - return 0; } +int gattlib_device_unref(gattlib_device_t* device) { + g_rec_mutex_lock(&m_gattlib_mutex); + device->reference_counter--; + if (device->reference_counter > 0) { + goto EXIT; + } + + free(device); + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_SUCCESS; +} + static void _gattlib_device_is_disconnected(gpointer data, gpointer user_data) { gattlib_device_t* device = data; bool* devices_are_disconnected_ptr = user_data; @@ -167,9 +171,7 @@ static void _gattlib_device_is_disconnected(gpointer data, gpointer user_data) { int gattlib_devices_are_disconnected(gattlib_adapter_t* adapter) { bool devices_are_disconnected = true; - g_rec_mutex_lock(&adapter->mutex); g_slist_foreach(adapter->devices, _gattlib_device_is_disconnected, &devices_are_disconnected); - g_rec_mutex_unlock(&adapter->mutex); return devices_are_disconnected; } @@ -182,10 +184,17 @@ static void _gattlib_device_dump_state(gpointer data, gpointer user_data) { } void gattlib_devices_dump_state(gattlib_adapter_t* adapter) { - g_rec_mutex_lock(&adapter->mutex); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + goto EXIT; + } + GATTLIB_LOG(GATTLIB_DEBUG, "Device list:"); g_slist_foreach(adapter->devices, _gattlib_device_dump_state, NULL); - g_rec_mutex_unlock(&adapter->mutex); + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); } #endif diff --git a/common/gattlib_internal.h b/common/gattlib_internal.h index b4a43bd..61e3639 100644 --- a/common/gattlib_internal.h +++ b/common/gattlib_internal.h @@ -24,6 +24,18 @@ struct gattlib_python_args { }; #endif +#define GATTLIB_SIGNAL_DEVICE_DISCONNECTION (1 << 0) +#define GATTLIB_SIGNAL_ADAPTER_STOP_SCANNING (1 << 1) + +struct gattlib_signal { + // Used by gattlib_disconnection when we want to wait for the disconnection to be effective + GCond condition; + // Mutex for condition + GMutex mutex; + // Identify the gattlib signals + uint32_t signals; +}; + struct gattlib_handler { union { gattlib_discovered_device_t discovered_device; @@ -36,11 +48,6 @@ struct gattlib_handler { void* user_data; // We create a thread to ensure the callback is not blocking the mainloop GThread *thread; - // The mutex ensures the callbacks is not being freed while being called - // We use a recursive mutex to be able to disable BLE scan from 'on_discovered_device' - // when we want to connect to the discovered device. - // Note: The risk is that we are actually realising the handle from the one we are executing - GRecMutex mutex; // Thread pool GThreadPool *thread_pool; #if defined(WITH_PYTHON) @@ -64,8 +71,9 @@ struct _gattlib_adapter { // BLE adapter name char* name; - // The recursive mutex allows to ensure sensible operations are always covered by a mutex in a same thread - GRecMutex mutex; + // reference counter is used to know whether the adapter is still use by callback + // When the reference counter is 0 then the adapter is freed + uintptr_t reference_counter; // List of `gattlib_device_t`. This list allows to know weither a device is // discovered/disconnected/connecting/connected/disconnecting. @@ -81,13 +89,6 @@ struct _gattlib_connection { // Context specific to the backend implementation (eg: dbus backend) struct _gattlib_connection_backend backend; - struct { - // Used by gattlib_disconnection when we want to wait for the disconnection to be effective - GCond condition; - // Used to avoid spurious or stolen wakeup - bool value; - } disconnection_wait; - struct gattlib_handler on_connection; struct gattlib_handler notification; struct gattlib_handler indication; @@ -98,7 +99,10 @@ typedef struct _gattlib_device { struct _gattlib_adapter* adapter; // On some platform, the name could be a UUID, on others its the DBUS device path char* device_id; - GMutex device_mutex; + + // reference counter is used to know whether the device is still use by callback + // When the reference counter is 0 then the device is freed + uintptr_t reference_counter; // We keep the state to prevent concurrent connecting/connected/disconnecting operation enum _gattlib_device_state state; @@ -106,6 +110,27 @@ typedef struct _gattlib_device { struct _gattlib_connection connection; } gattlib_device_t; +// This recursive mutex ensures all gattlib objects can be accessed in a multi-threaded environment +// The recursive mutex allows a same thread to lock twice the mutex without being blocked by itself. +extern GRecMutex m_gattlib_mutex; + +// Keep track of the allocated adapters to avoid an adapter to be freed twice. +// It could happen when using Python wrapper. +extern GSList *m_adapter_list; + +// This structure is used for inter-thread communication +extern struct gattlib_signal m_gattlib_signal; + +bool gattlib_adapter_is_valid(gattlib_adapter_t* adapter); +bool gattlib_adapter_is_scanning(gattlib_adapter_t* adapter); +int gattlib_adapter_ref(gattlib_adapter_t* adapter); +int gattlib_adapter_unref(gattlib_adapter_t* adapter); + +bool gattlib_device_is_valid(gattlib_device_t* device); +int gattlib_device_ref(gattlib_device_t* device); +int gattlib_device_unref(gattlib_device_t* device); +bool gattlib_connection_is_connected(gattlib_connection_t* connection); + void gattlib_handler_dispatch_to_thread(struct gattlib_handler* handler, void (*python_callback)(), GThreadFunc thread_func, const char* thread_name, void* (*thread_args_allocator)(va_list args), ...); void gattlib_handler_free(struct gattlib_handler* handler); diff --git a/dbus/CMakeLists.txt b/dbus/CMakeLists.txt index 1517e5a..7f17c55 100644 --- a/dbus/CMakeLists.txt +++ b/dbus/CMakeLists.txt @@ -73,6 +73,7 @@ set(gattlib_SRCS gattlib.c gattlib_stream.c bluez5/lib/uuid.c ${CMAKE_CURRENT_LIST_DIR}/../common/gattlib_common.c + ${CMAKE_CURRENT_LIST_DIR}/../common/gattlib_common_adapter.c ${CMAKE_CURRENT_LIST_DIR}/../common/gattlib_device_state_management.c ${CMAKE_CURRENT_LIST_DIR}/../common/gattlib_eddystone.c ${CMAKE_CURRENT_LIST_DIR}/../common/gattlib_callback_connected_device.c diff --git a/dbus/gattlib.c b/dbus/gattlib.c index c5d1c56..cc629e3 100644 --- a/dbus/gattlib.c +++ b/dbus/gattlib.c @@ -19,6 +19,12 @@ static void _on_device_connect(gattlib_connection_t* connection) { GDBusObjectManager *device_manager; GError *error = NULL; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_device_is_valid(connection->device)) { + goto EXIT; + } + // Stop the timeout for connection if (connection->backend.connection_timeout_id) { g_source_remove(connection->backend.connection_timeout_id); @@ -35,17 +41,20 @@ static void _on_device_connect(gattlib_connection_t* connection) { GATTLIB_LOG(GATTLIB_ERROR, "gattlib_connect: Failed to get device manager from adapter"); } //TODO: Free device - return; + goto EXIT; } connection->backend.dbus_objects = g_dbus_object_manager_get_objects(device_manager); gattlib_device_set_state(connection->device->adapter, connection->device->device_id, CONNECTED); gattlib_on_connected_device(connection); + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); } gboolean on_handle_device_property_change( - OrgBluezGattCharacteristic1 *object, + GDBusProxy *proxy, GVariant *arg_changed_properties, const gchar *const *arg_invalidated_properties, gpointer user_data) @@ -54,6 +63,7 @@ gboolean on_handle_device_property_change( // Retrieve 'Value' from 'arg_changed_properties' if (g_variant_n_children (arg_changed_properties) > 0) { + const gchar* device_object_path = g_dbus_proxy_get_object_path(proxy); GVariantIter *iter; const gchar *key; GVariant *value; @@ -62,17 +72,14 @@ gboolean on_handle_device_property_change( while (g_variant_iter_loop (iter, "{&sv}", &key, &value)) { if (strcmp(key, "Connected") == 0) { if (!g_variant_get_boolean(value)) { - GATTLIB_LOG(GATTLIB_DEBUG, "DBUS: device_property_change(%s): Disconnection", - connection->backend.device_object_path); + GATTLIB_LOG(GATTLIB_DEBUG, "DBUS: device_property_change(%s): Disconnection", device_object_path); gattlib_on_disconnected_device(connection); } else { - GATTLIB_LOG(GATTLIB_DEBUG, "DBUS: device_property_change(%s): Connection", - connection->backend.device_object_path); + GATTLIB_LOG(GATTLIB_DEBUG, "DBUS: device_property_change(%s): Connection", device_object_path); } } else if (strcmp(key, "ServicesResolved") == 0) { if (g_variant_get_boolean(value)) { - GATTLIB_LOG(GATTLIB_DEBUG, "DBUS: device_property_change(%s): Service Resolved", - connection->backend.device_object_path); + GATTLIB_LOG(GATTLIB_DEBUG, "DBUS: device_property_change(%s): Service Resolved", device_object_path); _on_device_connect(connection); } } @@ -132,9 +139,18 @@ void get_device_path_from_mac(const char *adapter_name, const char *mac_address, static gboolean _stop_connect_func(gpointer data) { gattlib_connection_t *connection = data; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_device_is_valid(connection->device)) { + goto EXIT; + } + // Reset the connection timeout connection->backend.connection_timeout_id = 0; +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + // We return FALSE when it is a one-off event return FALSE; } @@ -159,7 +175,7 @@ int gattlib_connect(gattlib_adapter_t* adapter, const char *dst, { const char* adapter_name = NULL; GError *error = NULL; - char object_path[100]; + char object_path[GATTLIB_DBUS_OBJECT_PATH_SIZE_MAX]; int ret = GATTLIB_SUCCESS; // In case NULL is passed, we initialized default adapter @@ -180,14 +196,23 @@ int gattlib_connect(gattlib_adapter_t* adapter, const char *dst, get_device_path_from_mac(adapter_name, dst, object_path, sizeof(object_path)); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + gattlib_device_t* device = gattlib_device_get_device(adapter, object_path); if (device == NULL) { GATTLIB_LOG(GATTLIB_DEBUG, "gattlib_connect: Cannot find connection %s", dst); - return GATTLIB_INVALID_PARAMETER; + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; } else if (device->state != DISCONNECTED) { GATTLIB_LOG(GATTLIB_DEBUG, "gattlib_connect: Cannot connect to '%s'. Device is in state %s", dst, device_state_str[device->state]); - return GATTLIB_BUSY; + ret = GATTLIB_BUSY; + goto EXIT; } device->connection.on_connection.callback.connection_handler = connect_cb; @@ -214,6 +239,7 @@ int gattlib_connect(gattlib_adapter_t* adapter, const char *dst, } else { GATTLIB_LOG(GATTLIB_ERROR, "gattlib_connect: Failed to connect to DBus Bluez Device"); } + goto EXIT; } else { device->connection.backend.device = bluez_device; device->connection.backend.device_object_path = strdup(object_path); @@ -255,6 +281,7 @@ int gattlib_connect(gattlib_adapter_t* adapter, const char *dst, // and 'org.bluez.GattCharacteristic1' to be advertised at that moment. device->connection.backend.connection_timeout_id = g_timeout_add_seconds(CONNECT_TIMEOUT_SEC, _stop_connect_func, &device->connection); + g_rec_mutex_unlock(&m_gattlib_mutex); return GATTLIB_SUCCESS; FREE_DEVICE: @@ -265,10 +292,12 @@ FREE_DEVICE: gattlib_adapter_close(adapter); } +EXIT: if (ret != GATTLIB_SUCCESS) { connect_cb(adapter, dst, NULL, ret /* error */, user_data); } + g_rec_mutex_unlock(&m_gattlib_mutex); return ret; } @@ -281,7 +310,6 @@ FREE_DEVICE: void gattlib_connection_free(gattlib_connection_t* connection) { char* device_id; - g_mutex_lock(&connection->device->device_mutex); device_id = connection->device->device_id; // Remove signal @@ -312,26 +340,24 @@ void gattlib_connection_free(gattlib_connection_t* connection) { // Mark the device has disconnected gattlib_device_set_state(connection->device->adapter, device_id, DISCONNECTED); - - g_mutex_unlock(&connection->device->device_mutex); } int gattlib_disconnect(gattlib_connection_t* connection, bool wait_disconnection) { - int ret = GATTLIB_SUCCESS; GError *error = NULL; + int ret = GATTLIB_SUCCESS; if (connection == NULL) { GATTLIB_LOG(GATTLIB_ERROR, "Cannot disconnect - connection parameter is not valid."); return GATTLIB_INVALID_PARAMETER; } - g_mutex_lock(&connection->device->device_mutex); + g_rec_mutex_lock(&m_gattlib_mutex); - if (connection->device->state != CONNECTED) { + if (!gattlib_connection_is_connected(connection)) { GATTLIB_LOG(GATTLIB_ERROR, "Cannot disconnect - connection is not in connected state (state=%s).", device_state_str[connection->device->state]); - ret = GATTLIB_BUSY; - goto EXIT; + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_BUSY; } GATTLIB_LOG(GATTLIB_DEBUG, "Disconnecting bluetooth device %s", connection->backend.device_object_path); @@ -340,6 +366,8 @@ int gattlib_disconnect(gattlib_connection_t* connection, bool wait_disconnection if (error) { GATTLIB_LOG(GATTLIB_ERROR, "Failed to disconnect DBus Bluez Device: %s", error->message); g_error_free(error); + + // We continue, we still want to set the correct state } // Mark the device has disconnected @@ -348,35 +376,53 @@ int gattlib_disconnect(gattlib_connection_t* connection, bool wait_disconnection //Note: Signals and memory will be removed/clean on disconnction callback // See _gattlib_clean_on_disconnection() + // We must release the mutex before the loop to leave other threads to signal the disconnection + g_rec_mutex_unlock(&m_gattlib_mutex); + if (wait_disconnection) { gint64 end_time; + g_mutex_lock(&m_gattlib_signal.mutex); + end_time = g_get_monotonic_time() + GATTLIB_DISCONNECTION_WAIT_TIMEOUT_SEC * G_TIME_SPAN_SECOND; - while (!connection->disconnection_wait.value) { - if (!g_cond_wait_until(&connection->disconnection_wait.condition, &connection->device->device_mutex, end_time)) { + while (gattlib_connection_is_connected(connection)) { + if (!g_cond_wait_until(&m_gattlib_signal.condition, &m_gattlib_signal.mutex, end_time)) { ret = GATTLIB_TIMEOUT; break; } } + + g_mutex_unlock(&m_gattlib_signal.mutex); } -EXIT: - g_mutex_unlock(&connection->device->device_mutex); return ret; } // Bluez was using org.bluez.Device1.GattServices until 5.37 to expose the list of available GATT Services #if BLUEZ_VERSION < BLUEZ_VERSIONS(5, 38) int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_service_t** services, int* services_count) { + const gchar* const* service_str; + GError *error = NULL; + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); + if (connection == NULL) { GATTLIB_LOG(GATTLIB_ERROR, "Gattlib connection not initialized."); + g_rec_mutex_unlock(&m_gattlib_mutex); return GATTLIB_INVALID_PARAMETER; } + if (!gattlib_device_is_valid(connection->device)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_INVALID_PARAMETER; + } + + // Increase 'bluez_device' reference counter to avoid to keep the lock longer OrgBluezDevice1* bluez_device = connection->backend.device; - const gchar* const* service_str; - GError *error = NULL; + g_object_ref(bluez_device); + g_rec_mutex_unlock(&m_gattlib_mutex); const gchar* const* service_strs = org_bluez_device1_get_gatt_services(bluez_device); @@ -398,7 +444,8 @@ int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_s gattlib_primary_service_t* primary_services = calloc(count_max * sizeof(gattlib_primary_service_t), 1); if (primary_services == NULL) { - return GATTLIB_OUT_OF_MEMORY; + ret = GATTLIB_OUT_OF_MEMORY; + goto EXIT; } for (service_str = service_strs; *service_str != NULL; service_str++) { @@ -440,21 +487,33 @@ int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_s if (services_count != NULL) { *services_count = count; } - return GATTLIB_SUCCESS; + +EXIT: + g_object_unref(bluez_device); + return ret; } #else int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_service_t** services, int* services_count) { - if (connection == NULL) { - GATTLIB_LOG(GATTLIB_ERROR, "Gattlib connection not initialized."); - return GATTLIB_INVALID_PARAMETER; - } - GError *error = NULL; - GDBusObjectManager *device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); - OrgBluezDevice1* device = connection->backend.device; const gchar* const* service_str; int ret = GATTLIB_SUCCESS; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (connection == NULL) { + GATTLIB_LOG(GATTLIB_ERROR, "Gattlib connection not initialized."); + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + + if (!gattlib_device_is_valid(connection->device)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + + GDBusObjectManager *device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); + OrgBluezDevice1* device = connection->backend.device; + const gchar* const* service_strs = org_bluez_device1_get_uuids(device); if (device_manager == NULL) { @@ -466,7 +525,7 @@ int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_s ret = GATTLIB_ERROR_DBUS; GATTLIB_LOG(GATTLIB_ERROR, "Gattlib Context not initialized."); } - return ret; + goto EXIT; } if (service_strs == NULL) { @@ -476,7 +535,7 @@ int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_s if (services_count != NULL) { *services_count = 0; } - return GATTLIB_SUCCESS; + goto EXIT; } // Maximum number of primary services @@ -487,7 +546,8 @@ int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_s gattlib_primary_service_t* primary_services = calloc(count_max * sizeof(gattlib_primary_service_t), 1); if (primary_services == NULL) { - return GATTLIB_OUT_OF_MEMORY; + ret = GATTLIB_OUT_OF_MEMORY; + goto EXIT; } GList *l; @@ -590,6 +650,9 @@ int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_s if (ret != GATTLIB_SUCCESS) { free(primary_services); } + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); return ret; } #endif @@ -597,9 +660,21 @@ int gattlib_discover_primary(gattlib_connection_t* connection, gattlib_primary_s // Bluez was using org.bluez.Device1.GattServices until 5.37 to expose the list of available GATT Services #if BLUEZ_VERSION < BLUEZ_VERSIONS(5, 38) int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start, uint16_t end, gattlib_characteristic_t** characteristics, int* characteristics_count) { - OrgBluezDevice1* bluez_device = connection->backend.bluez_device; GError *error = NULL; int handle; + int ret = GATTLIB_SUCCESS; + + // Increase bluez_device object reference counter to avoid to keep locking the mutex + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_device_is_valid(connection->device)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_INVALID_PARAMETER; + } + + OrgBluezDevice1* bluez_device = connection->backend.bluez_device; + g_object_ref(bluez_device); + g_rec_mutex_unlock(&m_gattlib_mutex); const gchar* const* service_strs = org_bluez_device1_get_gatt_services(bluez_device); const gchar* const* service_str; @@ -607,7 +682,8 @@ int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start const gchar* characteristic_str; if (service_strs == NULL) { - return GATTLIB_INVALID_PARAMETER; + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; } // Maximum number of primary services @@ -656,7 +732,8 @@ int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start gattlib_characteristic_t* characteristic_list = calloc(count_max * sizeof(gattlib_characteristic_t), 1); if (characteristic_list == NULL) { - return GATTLIB_OUT_OF_MEMORY; + ret = GATTLIB_OUT_OF_MEMORY; + goto EXIT; } for (service_str = service_strs; *service_str != NULL; service_str++) { @@ -744,7 +821,10 @@ int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start *characteristics = characteristic_list; *characteristics_count = count; - return GATTLIB_SUCCESS; + +EXIT: + g_object_unref(bluez_device); + return ret; } #else static void add_characteristics_from_service(struct _gattlib_connection_backend* backend, GDBusObjectManager *device_manager, @@ -840,10 +920,18 @@ static void add_characteristics_from_service(struct _gattlib_connection_backend* int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start, uint16_t end, gattlib_characteristic_t** characteristics, int* characteristics_count) { GError *error = NULL; - GDBusObjectManager *device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); + GDBusObjectManager *device_manager; GList *l; - int ret; + int ret = GATTLIB_SUCCESS; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_device_is_valid(connection->device)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + + device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); if (device_manager == NULL) { if (error != NULL) { ret = GATTLIB_ERROR_DBUS_WITH_ERROR(error); @@ -853,7 +941,7 @@ int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start ret = GATTLIB_ERROR_DBUS; GATTLIB_LOG(GATTLIB_ERROR, "Gattlib Context not initialized."); } - return ret; + goto EXIT; } // Count the maximum number of characteristic to allocate the array (we count all the characterstic for all devices) @@ -877,7 +965,8 @@ int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start gattlib_characteristic_t* characteristic_list = calloc(count_max * sizeof(gattlib_characteristic_t), 1); if (characteristic_list == NULL) { - return GATTLIB_OUT_OF_MEMORY; + ret = GATTLIB_OUT_OF_MEMORY; + goto EXIT; } // List all services for this device @@ -941,7 +1030,9 @@ int gattlib_discover_char_range(gattlib_connection_t* connection, uint16_t start *characteristics = characteristic_list; *characteristics_count = count; - return GATTLIB_SUCCESS; +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } #endif @@ -961,10 +1052,17 @@ int gattlib_discover_desc(gattlib_connection_t* connection, gattlib_descriptor_t int get_bluez_device_from_mac(struct _gattlib_adapter *adapter, const char *mac_address, OrgBluezDevice1 **bluez_device1) { GError *error = NULL; - char object_path[100]; - int ret; + char object_path[GATTLIB_DBUS_OBJECT_PATH_SIZE_MAX]; + + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_INVALID_PARAMETER; + } if (adapter->backend.adapter_proxy == NULL) { + g_rec_mutex_unlock(&m_gattlib_mutex); return GATTLIB_NO_ADAPTER; } @@ -974,6 +1072,9 @@ int get_bluez_device_from_mac(struct _gattlib_adapter *adapter, const char *mac_ get_device_path_from_mac(NULL, mac_address, object_path, sizeof(object_path)); } + // No need to keep the mutex longer. After it is DBUS specific operations not depending on gattlib structure + g_rec_mutex_unlock(&m_gattlib_mutex); + *bluez_device1 = org_bluez_device1_proxy_new_for_bus_sync( G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, @@ -982,10 +1083,9 @@ int get_bluez_device_from_mac(struct _gattlib_adapter *adapter, const char *mac_ NULL, &error); if (error) { - ret = GATTLIB_ERROR_DBUS_WITH_ERROR(error); GATTLIB_LOG(GATTLIB_ERROR, "Failed to connection to new DBus Bluez Device: %s", error->message); g_error_free(error); - return ret; + return GATTLIB_ERROR_DBUS_WITH_ERROR(error); } return GATTLIB_SUCCESS; @@ -993,15 +1093,31 @@ int get_bluez_device_from_mac(struct _gattlib_adapter *adapter, const char *mac_ int gattlib_get_rssi(gattlib_connection_t *connection, int16_t *rssi) { - if (connection == NULL) { - return GATTLIB_INVALID_PARAMETER; - } - if (rssi == NULL) { return GATTLIB_INVALID_PARAMETER; } - *rssi = org_bluez_device1_get_rssi(connection->backend.device); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (connection == NULL) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_INVALID_PARAMETER; + } + + if (!gattlib_device_is_valid(connection->device)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_INVALID_PARAMETER; + } + + // device is actually a GObject. Increasing its reference counter prevents to + // be freed if the connection is released. + OrgBluezDevice1* dbus_device = connection->backend.device; + g_object_ref(dbus_device); + g_rec_mutex_unlock(&m_gattlib_mutex); + + *rssi = org_bluez_device1_get_rssi(dbus_device); + + g_object_unref(dbus_device); return GATTLIB_SUCCESS; } @@ -1015,9 +1131,12 @@ int gattlib_get_rssi_from_mac(gattlib_adapter_t* adapter, const char *mac_addres return GATTLIB_INVALID_PARAMETER; } + // + // No need of locking the mutex in this function. get_bluez_device_from_mac() ensures the adapter is valid. + // + ret = get_bluez_device_from_mac(adapter, mac_address, &bluez_device1); if (ret != GATTLIB_SUCCESS) { - g_object_unref(bluez_device1); return ret; } diff --git a/dbus/gattlib_adapter.c b/dbus/gattlib_adapter.c index 7eecec3..19c9e5e 100644 --- a/dbus/gattlib_adapter.c +++ b/dbus/gattlib_adapter.c @@ -6,10 +6,12 @@ #include "gattlib_internal.h" -// Keep track of the allocated adapters to avoid an adapter to be freed twice. -// It could happen when using Python wrapper. -static GSList *m_adapter_list; -static GMutex m_adapter_list_mutex; +// This recursive mutex ensures all gattlib objects can be accessed in a multi-threaded environment +// The recursive mutex allows a same thread to lock twice the mutex without being blocked by itself. +GRecMutex m_gattlib_mutex; + +// This structure is used for inter-thread communication +struct gattlib_signal m_gattlib_signal; int gattlib_adapter_open(const char* adapter_name, gattlib_adapter_t** adapter) { @@ -57,17 +59,23 @@ int gattlib_adapter_open(const char* adapter_name, gattlib_adapter_t** adapter) // Initialize stucture gattlib_adapter->name = strdup(adapter_name); + gattlib_adapter->reference_counter = 1; gattlib_adapter->backend.adapter_proxy = adapter_proxy; - g_mutex_lock(&m_adapter_list_mutex); + g_rec_mutex_lock(&m_gattlib_mutex); m_adapter_list = g_slist_append(m_adapter_list, gattlib_adapter); - g_mutex_unlock(&m_adapter_list_mutex); - *adapter = gattlib_adapter; + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_SUCCESS; } const char *gattlib_adapter_get_name(gattlib_adapter_t* adapter) { + // + // Note: There is a risk we access the memory when it has been freed + // What we should do is to take 'm_gattlib_mutex', then to check the adapter is valid + // then to duplicate the string + // return adapter->name; } @@ -84,8 +92,6 @@ gattlib_adapter_t* init_default_adapter(void) { } GDBusObjectManager *get_device_manager_from_adapter(gattlib_adapter_t* gattlib_adapter, GError **error) { - g_mutex_lock(&m_adapter_list_mutex); - if (gattlib_adapter->backend.device_manager) { goto EXIT; } @@ -103,12 +109,10 @@ GDBusObjectManager *get_device_manager_from_adapter(gattlib_adapter_t* gattlib_a NULL, NULL, NULL, NULL, error); if (gattlib_adapter->backend.device_manager == NULL) { - g_mutex_unlock(&m_adapter_list_mutex); return NULL; } EXIT: - g_mutex_unlock(&m_adapter_list_mutex); return gattlib_adapter->backend.device_manager; } @@ -138,7 +142,13 @@ static void device_manager_on_added_device1_signal(const char* device1_path, gat return; } - g_rec_mutex_lock(&gattlib_adapter->mutex); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(gattlib_adapter)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + g_object_unref(device1); + return; + } //TODO: Add support for connected device with 'gboolean org_bluez_device1_get_connected (OrgBluezDevice1 *object);' // When the device is connected, we potentially need to initialize some attributes @@ -147,7 +157,7 @@ static void device_manager_on_added_device1_signal(const char* device1_path, gat gattlib_on_discovered_device(gattlib_adapter, device1); } - g_rec_mutex_unlock(&gattlib_adapter->mutex); + g_rec_mutex_unlock(&m_gattlib_mutex); g_object_unref(device1); } } @@ -196,10 +206,6 @@ on_interface_proxy_properties_changed (GDBusObjectManagerClient *device_manager, const char* proxy_object_path = g_dbus_proxy_get_object_path(interface_proxy); gattlib_adapter_t* gattlib_adapter = user_data; - if (gattlib_adapter->backend.device_manager == NULL) { - return; - } - // Count number of invalidated properties size_t invalidated_properties_count = 0; if (invalidated_properties != NULL) { @@ -216,6 +222,16 @@ on_interface_proxy_properties_changed (GDBusObjectManagerClient *device_manager, g_variant_print(changed_properties, TRUE), invalidated_properties_count); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(gattlib_adapter)) { + goto EXIT; + } + + if (gattlib_adapter->backend.device_manager == NULL) { + goto EXIT; + } + // Check if the object is a 'org.bluez.Device1' if (strcmp(g_dbus_proxy_get_interface_name(interface_proxy), "org.bluez.Device1") == 0) { // It is a 'org.bluez.Device1' @@ -229,10 +245,10 @@ on_interface_proxy_properties_changed (GDBusObjectManagerClient *device_manager, if (error) { GATTLIB_LOG(GATTLIB_ERROR, "Failed to connection to new DBus Bluez Device: %s", error->message); g_error_free(error); - return; + goto EXIT; } else if (device1 == NULL) { GATTLIB_LOG(GATTLIB_ERROR, "Unexpected NULL device"); - return; + goto EXIT; } // Check if the device has been disconnected @@ -241,8 +257,6 @@ on_interface_proxy_properties_changed (GDBusObjectManagerClient *device_manager, GVariant* has_rssi = g_variant_dict_lookup_value(&dict, "RSSI", NULL); GVariant* has_manufacturer_data = g_variant_dict_lookup_value(&dict, "ManufacturerData", NULL); - g_rec_mutex_lock(&gattlib_adapter->mutex); - enum _gattlib_device_state old_device_state = gattlib_device_get_state(gattlib_adapter, proxy_object_path); if (old_device_state == NOT_FOUND) { @@ -254,11 +268,12 @@ on_interface_proxy_properties_changed (GDBusObjectManagerClient *device_manager, } } - g_rec_mutex_unlock(&gattlib_adapter->mutex); - g_variant_dict_end(&dict); g_object_unref(device1); } + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); } /** @@ -267,11 +282,11 @@ on_interface_proxy_properties_changed (GDBusObjectManagerClient *device_manager, * It either called when we wait for BLE scan to complete or when we close the BLE adapter */ static void _wait_scan_loop_stop_scanning(gattlib_adapter_t* gattlib_adapter) { - g_mutex_lock(&gattlib_adapter->backend.ble_scan.scan_loop.mutex); - while (gattlib_adapter->backend.ble_scan.is_scanning) { - g_cond_wait(&gattlib_adapter->backend.ble_scan.scan_loop.cond, &gattlib_adapter->backend.ble_scan.scan_loop.mutex); + g_mutex_lock(&m_gattlib_signal.mutex); + while (gattlib_adapter_is_scanning(gattlib_adapter)) { + g_cond_wait(&m_gattlib_signal.condition, &m_gattlib_signal.mutex); } - g_mutex_unlock(&gattlib_adapter->backend.ble_scan.scan_loop.mutex); + g_mutex_unlock(&m_gattlib_signal.mutex); } /** @@ -280,16 +295,26 @@ static void _wait_scan_loop_stop_scanning(gattlib_adapter_t* gattlib_adapter) { static gboolean _stop_scan_on_timeout(gpointer data) { gattlib_adapter_t* gattlib_adapter = data; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(gattlib_adapter)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return FALSE; + } + if (gattlib_adapter->backend.ble_scan.is_scanning) { - g_mutex_lock(&gattlib_adapter->backend.ble_scan.scan_loop.mutex); + g_mutex_lock(&m_gattlib_signal.mutex); gattlib_adapter->backend.ble_scan.is_scanning = false; - g_cond_broadcast(&gattlib_adapter->backend.ble_scan.scan_loop.cond); - g_mutex_unlock(&gattlib_adapter->backend.ble_scan.scan_loop.mutex); + m_gattlib_signal.signals |= GATTLIB_SIGNAL_ADAPTER_STOP_SCANNING; + g_cond_broadcast(&m_gattlib_signal.condition); + g_mutex_unlock(&m_gattlib_signal.mutex); } // Unset timeout ID to not try removing it gattlib_adapter->backend.ble_scan.ble_scan_timeout_id = 0; + g_rec_mutex_unlock(&m_gattlib_mutex); + GATTLIB_LOG(GATTLIB_DEBUG, "BLE scan is stopped after scanning time has expired."); return FALSE; } @@ -301,6 +326,12 @@ static gboolean _stop_scan_on_timeout(gpointer data) { static void* _ble_scan_loop_thread(void* args) { gattlib_adapter_t* gattlib_adapter = args; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(gattlib_adapter)) { + goto EXIT; + } + if (gattlib_adapter->backend.ble_scan.ble_scan_timeout_id > 0) { GATTLIB_LOG(GATTLIB_WARNING, "A BLE scan seems to already be in progress."); } @@ -314,11 +345,20 @@ static void* _ble_scan_loop_thread(void* args) { _stop_scan_on_timeout, gattlib_adapter); } + g_rec_mutex_unlock(&m_gattlib_mutex); + // Wait for the BLE scan to be explicitely stopped by 'gattlib_adapter_scan_disable()' or timeout. _wait_scan_loop_stop_scanning(gattlib_adapter); // Note: The function only resumes when loop timeout as expired or g_main_loop_quit has been called. + g_rec_mutex_lock(&m_gattlib_mutex); + + // Confirm gattlib_adapter is still valid + if (!gattlib_adapter_is_valid(gattlib_adapter)) { + goto EXIT; + } + g_signal_handler_disconnect(G_DBUS_OBJECT_MANAGER(gattlib_adapter->backend.device_manager), gattlib_adapter->backend.ble_scan.added_signal_id); g_signal_handler_disconnect(G_DBUS_OBJECT_MANAGER(gattlib_adapter->backend.device_manager), gattlib_adapter->backend.ble_scan.removed_signal_id); g_signal_handler_disconnect(G_DBUS_OBJECT_MANAGER(gattlib_adapter->backend.device_manager), gattlib_adapter->backend.ble_scan.changed_signal_id); @@ -326,7 +366,9 @@ static void* _ble_scan_loop_thread(void* args) { // Ensure BLE device discovery is stopped gattlib_adapter_scan_disable(gattlib_adapter); - return 0; +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return NULL; } static int _gattlib_adapter_scan_enable_with_filter(gattlib_adapter_t* adapter, uuid_t **uuid_list, int16_t rssi_threshold, uint32_t enabled_filters, @@ -443,12 +485,19 @@ int gattlib_adapter_scan_enable_with_filter(gattlib_adapter_t* adapter, uuid_t * gattlib_discovered_device_t discovered_device_cb, size_t timeout, void *user_data) { GError *error = NULL; - int ret; + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } ret = _gattlib_adapter_scan_enable_with_filter(adapter, uuid_list, rssi_threshold, enabled_filters, discovered_device_cb, timeout, user_data); if (ret != GATTLIB_SUCCESS) { - return ret; + goto EXIT; } adapter->backend.ble_scan.is_scanning = true; @@ -457,42 +506,67 @@ int gattlib_adapter_scan_enable_with_filter(gattlib_adapter_t* adapter, uuid_t * if (adapter->backend.ble_scan.scan_loop_thread == NULL) { GATTLIB_LOG(GATTLIB_ERROR, "Failed to create BLE scan thread: %s", error->message); g_error_free(error); - return GATTLIB_ERROR_INTERNAL; + ret = GATTLIB_ERROR_INTERNAL; + goto EXIT; } - g_mutex_lock(&adapter->backend.ble_scan.scan_loop.mutex); - while (adapter->backend.ble_scan.is_scanning) { - g_cond_wait(&adapter->backend.ble_scan.scan_loop.cond, &adapter->backend.ble_scan.scan_loop.mutex); + // We need to release the mutex to ensure we leave the other thread to signal us + g_rec_mutex_unlock(&m_gattlib_mutex); + + g_mutex_lock(&m_gattlib_signal.mutex); + while (gattlib_adapter_is_scanning(adapter)) { + g_cond_wait(&m_gattlib_signal.condition, &m_gattlib_signal.mutex); + } + g_mutex_unlock(&m_gattlib_signal.mutex); + + // Get the mutex again + g_rec_mutex_lock(&m_gattlib_mutex); + + // Ensure the adapter is still valid when we get the mutex again + if (!gattlib_adapter_is_valid(adapter)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; } // Free thread g_thread_unref(adapter->backend.ble_scan.scan_loop_thread); adapter->backend.ble_scan.scan_loop_thread = NULL; - g_mutex_unlock(&adapter->backend.ble_scan.scan_loop.mutex); - return 0; +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } int gattlib_adapter_scan_enable_with_filter_non_blocking(gattlib_adapter_t* adapter, uuid_t **uuid_list, int16_t rssi_threshold, uint32_t enabled_filters, gattlib_discovered_device_t discovered_device_cb, size_t timeout, void *user_data) { GError *error = NULL; - int ret; + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } ret = _gattlib_adapter_scan_enable_with_filter(adapter, uuid_list, rssi_threshold, enabled_filters, discovered_device_cb, timeout, user_data); if (ret != GATTLIB_SUCCESS) { - return ret; + goto EXIT; } adapter->backend.ble_scan.scan_loop_thread = g_thread_try_new("gattlib_ble_scan", _ble_scan_loop_thread, adapter, &error); if (adapter->backend.ble_scan.scan_loop_thread == NULL) { GATTLIB_LOG(GATTLIB_ERROR, "Failed to create BLE scan thread: %s", error->message); g_error_free(error); - return GATTLIB_ERROR_INTERNAL; + ret = GATTLIB_ERROR_INTERNAL; + goto EXIT; } - return 0; +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } int gattlib_adapter_scan_enable(gattlib_adapter_t* adapter, gattlib_discovered_device_t discovered_device_cb, size_t timeout, void *user_data) @@ -505,14 +579,21 @@ int gattlib_adapter_scan_enable(gattlib_adapter_t* adapter, gattlib_discovered_d int gattlib_adapter_scan_disable(gattlib_adapter_t* adapter) { GError *error = NULL; + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } if (adapter->backend.adapter_proxy == NULL) { GATTLIB_LOG(GATTLIB_INFO, "Could not disable BLE scan. No BLE adapter setup."); - return GATTLIB_NO_ADAPTER; + ret = GATTLIB_NO_ADAPTER; + goto EXIT; } - g_mutex_lock(&adapter->backend.ble_scan.scan_loop.mutex); - if (!org_bluez_adapter1_get_discovering(adapter->backend.adapter_proxy)) { GATTLIB_LOG(GATTLIB_DEBUG, "No discovery in progress. We skip discovery stopping (1)."); goto EXIT; @@ -526,6 +607,7 @@ int gattlib_adapter_scan_disable(gattlib_adapter_t* adapter) { org_bluez_adapter1_call_stop_discovery_sync(adapter->backend.adapter_proxy, NULL, &error); if (error != NULL) { if (((error->domain == 238) || (error->domain == 239)) && (error->code == 36)) { + GATTLIB_LOG(GATTLIB_WARNING, "No bluetooth scan has been started."); // Correspond to error: GDBus.Error:org.bluez.Error.Failed: No discovery started goto EXIT; } else { @@ -539,7 +621,10 @@ int gattlib_adapter_scan_disable(gattlib_adapter_t* adapter) { // Stop BLE scan loop thread if (adapter->backend.ble_scan.is_scanning) { adapter->backend.ble_scan.is_scanning = false; - g_cond_broadcast(&adapter->backend.ble_scan.scan_loop.cond); + g_mutex_lock(&m_gattlib_signal.mutex); + m_gattlib_signal.signals |= GATTLIB_SIGNAL_ADAPTER_STOP_SCANNING; + g_cond_broadcast(&m_gattlib_signal.condition); + g_mutex_unlock(&m_gattlib_signal.mutex); } // Remove timeout @@ -549,20 +634,33 @@ int gattlib_adapter_scan_disable(gattlib_adapter_t* adapter) { } EXIT: - g_mutex_unlock(&adapter->backend.ble_scan.scan_loop.mutex); - return GATTLIB_SUCCESS; + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } int gattlib_adapter_close(gattlib_adapter_t* adapter) { bool are_devices_disconnected; + int ret = GATTLIB_SUCCESS; + + // + // TODO: Should we use reference counting to be able to open multiple times an adapter + // without freeing it on the first gattlib_adapter_close() + // + + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_adapter_is_valid(adapter)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } are_devices_disconnected = gattlib_devices_are_disconnected(adapter); if (!are_devices_disconnected) { GATTLIB_LOG(GATTLIB_ERROR, "Adapter cannot be closed as some devices are not disconnected"); - return GATTLIB_BUSY; + ret = GATTLIB_BUSY; + goto EXIT; } - g_mutex_lock(&m_adapter_list_mutex); GSList *adapter_entry = g_slist_find(m_adapter_list, adapter); if (adapter_entry == NULL) { GATTLIB_LOG(GATTLIB_WARNING, "Adapter has already been closed"); @@ -578,6 +676,34 @@ int gattlib_adapter_close(gattlib_adapter_t* adapter) { g_thread_join(adapter->backend.ble_scan.scan_loop_thread); } + // Unref/Free the adapter + gattlib_adapter_unref(adapter); + + // Remove adapter from the global list + m_adapter_list = g_slist_remove(m_adapter_list, adapter); + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; +} + +int gattlib_adapter_ref(gattlib_adapter_t* adapter) { + g_rec_mutex_lock(&m_gattlib_mutex); + adapter->reference_counter++; + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_SUCCESS; +} + +int gattlib_adapter_unref(gattlib_adapter_t* adapter) { + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); + adapter->reference_counter--; + + if (adapter->reference_counter > 0) { + goto EXIT; + } + // Ensure the thread is freed on adapter closing if (adapter->backend.ble_scan.scan_loop_thread) { g_thread_unref(adapter->backend.ble_scan.scan_loop_thread); @@ -603,12 +729,7 @@ int gattlib_adapter_close(gattlib_adapter_t* adapter) { free(adapter); - // Remove adapter from the global list - m_adapter_list = g_slist_remove(m_adapter_list, adapter); - - adapter = NULL; - EXIT: - g_mutex_unlock(&m_adapter_list_mutex); - return GATTLIB_SUCCESS; -} + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; +} \ No newline at end of file diff --git a/dbus/gattlib_advertisement.c b/dbus/gattlib_advertisement.c index a4f6457..cb89abc 100644 --- a/dbus/gattlib_advertisement.c +++ b/dbus/gattlib_advertisement.c @@ -116,13 +116,33 @@ int gattlib_get_advertisement_data(gattlib_connection_t *connection, gattlib_advertisement_data_t **advertisement_data, size_t *advertisement_data_count, uint16_t *manufacturer_id, uint8_t **manufacturer_data, size_t *manufacturer_data_size) { + int ret; + + g_rec_mutex_lock(&m_gattlib_mutex); + if (connection == NULL) { + g_rec_mutex_unlock(&m_gattlib_mutex); return GATTLIB_INVALID_PARAMETER; } - return get_advertisement_data_from_device(connection->backend.device, - advertisement_data, advertisement_data_count, - manufacturer_id, manufacturer_data, manufacturer_data_size); + if (!gattlib_device_is_valid(connection->device)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return GATTLIB_INVALID_PARAMETER; + } + + // device is actually a GObject. Increasing its reference counter prevents to + // be freed if the connection is released. + OrgBluezDevice1* dbus_device = connection->backend.device; + g_object_ref(dbus_device); + g_rec_mutex_unlock(&m_gattlib_mutex); + + ret = get_advertisement_data_from_device(dbus_device, + advertisement_data, advertisement_data_count, + manufacturer_id, manufacturer_data, manufacturer_data_size); + + g_object_unref(dbus_device); + + return ret; } int gattlib_get_advertisement_data_from_mac(gattlib_adapter_t* adapter, const char *mac_address, @@ -132,10 +152,14 @@ int gattlib_get_advertisement_data_from_mac(gattlib_adapter_t* adapter, const ch OrgBluezDevice1 *bluez_device1; int ret; + // + // No need of locking the mutex in this function. It is already done by get_bluez_device_from_mac() + // and get_advertisement_data_from_device() does not depend on gattlib objects + // + ret = get_bluez_device_from_mac(adapter, mac_address, &bluez_device1); if (ret != GATTLIB_SUCCESS) { - g_object_unref(bluez_device1); - return ret; + goto EXIT; } ret = get_advertisement_data_from_device(bluez_device1, @@ -144,6 +168,7 @@ int gattlib_get_advertisement_data_from_mac(gattlib_adapter_t* adapter, const ch g_object_unref(bluez_device1); +EXIT: return ret; } diff --git a/dbus/gattlib_backend.h b/dbus/gattlib_backend.h index 94ac6ff..511a155 100644 --- a/dbus/gattlib_backend.h +++ b/dbus/gattlib_backend.h @@ -36,6 +36,11 @@ #define GATTLIB_DEFAULT_ADAPTER "hci0" +// Arbitrary size used to build DBUS object path from adapter name and mac address. +// Otherwise DBUs Object path are unlimited: +// See: https://dbus.freedesktop.org/doc/api/html/group__DBusProtocol.html#ga80186ac58d031d83127d1ad6644b0011 +#define GATTLIB_DBUS_OBJECT_PATH_SIZE_MAX 200 + struct _gattlib_connection_backend { char* device_object_path; OrgBluezDevice1* device; @@ -69,11 +74,6 @@ struct _gattlib_adapter_backend { GThread *scan_loop_thread; // Thread used to run the '_scan_loop()' when non-blocking bool is_scanning; - struct { - GMutex mutex; - GCond cond; - } scan_loop; - uint32_t enabled_filters; } ble_scan; }; diff --git a/dbus/gattlib_char.c b/dbus/gattlib_char.c index 8f26ee2..5d7f9b3 100644 --- a/dbus/gattlib_char.c +++ b/dbus/gattlib_char.c @@ -103,13 +103,20 @@ static bool handle_dbus_battery_from_uuid(struct _gattlib_connection_backend* ba struct dbus_characteristic get_characteristic_from_uuid(gattlib_connection_t* connection, const uuid_t* uuid) { GError *error = NULL; - GDBusObjectManager *device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); + GDBusObjectManager *device_manager; bool is_battery_level_uuid = false; - struct dbus_characteristic dbus_characteristic = { - .type = TYPE_NONE + .type = TYPE_NONE }; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(connection)) { + goto EXIT; + } + + device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); + if (device_manager == NULL) { if (error != NULL) { GATTLIB_LOG(GATTLIB_ERROR, "Gattlib Context not initialized (%d, %d).", error->domain, error->code); @@ -117,7 +124,7 @@ struct dbus_characteristic get_characteristic_from_uuid(gattlib_connection_t* co } else { GATTLIB_LOG(GATTLIB_ERROR, "Gattlib Context not initialized."); } - return dbus_characteristic; // Return characteristic of type TYPE_NONE + goto EXIT; // Return characteristic of type TYPE_NONE } // Some GATT Characteristics are handled by D-BUS @@ -125,7 +132,7 @@ struct dbus_characteristic get_characteristic_from_uuid(gattlib_connection_t* co is_battery_level_uuid = true; } else if (gattlib_uuid_cmp(uuid, &m_ccc_uuid) == 0) { GATTLIB_LOG(GATTLIB_ERROR, "Error: Bluez v5.42+ does not expose Client Characteristic Configuration Descriptor through DBUS interface"); - return dbus_characteristic; + goto EXIT; } GList *l; @@ -162,18 +169,27 @@ struct dbus_characteristic get_characteristic_from_uuid(gattlib_connection_t* co } } +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); return dbus_characteristic; } static struct dbus_characteristic get_characteristic_from_handle(gattlib_connection_t* connection, unsigned int handle) { GError *error = NULL; - GDBusObjectManager *device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); unsigned int char_handle; - + GDBusObjectManager *device_manager; struct dbus_characteristic dbus_characteristic = { - .type = TYPE_NONE + .type = TYPE_NONE }; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(connection)) { + goto EXIT; + } + + device_manager = get_device_manager_from_adapter(connection->device->adapter, &error); + if (device_manager == NULL) { if (error != NULL) { GATTLIB_LOG(GATTLIB_ERROR, "Gattlib Context not initialized (%d, %d).", error->domain, error->code); @@ -181,7 +197,7 @@ static struct dbus_characteristic get_characteristic_from_handle(gattlib_connect } else { GATTLIB_LOG(GATTLIB_ERROR, "Gattlib Context not initialized."); } - return dbus_characteristic; + goto EXIT; } for (GList *l = connection->backend.dbus_objects; l != NULL; l = l->next) { @@ -209,6 +225,8 @@ static struct dbus_characteristic get_characteristic_from_handle(gattlib_connect } } +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); return dbus_characteristic; } @@ -272,6 +290,11 @@ static int read_battery_level(struct dbus_characteristic *dbus_characteristic, v #endif int gattlib_read_char_by_uuid(gattlib_connection_t* connection, uuid_t* uuid, void **buffer, size_t *buffer_len) { + // + // No need of locking the gattlib mutex. get_characteristic_from_uuid() is taking care of the gattlib + // object coherency. And 'dbus_characteristic' is not linked to gattlib object + // + struct dbus_characteristic dbus_characteristic = get_characteristic_from_uuid(connection, uuid); if (dbus_characteristic.type == TYPE_NONE) { return GATTLIB_NOT_FOUND; @@ -297,6 +320,11 @@ int gattlib_read_char_by_uuid(gattlib_connection_t* connection, uuid_t* uuid, vo int gattlib_read_char_by_uuid_async(gattlib_connection_t* connection, uuid_t* uuid, gatt_read_cb_t gatt_read_cb) { int ret = GATTLIB_SUCCESS; + // + // No need of locking the gattlib mutex. get_characteristic_from_uuid() is taking care of the gattlib + // object coherency. And 'dbus_characteristic' is not linked to gattlib object + // + struct dbus_characteristic dbus_characteristic = get_characteristic_from_uuid(connection, uuid); if (dbus_characteristic.type == TYPE_NONE) { return GATTLIB_NOT_FOUND; @@ -391,6 +419,11 @@ int gattlib_write_char_by_uuid(gattlib_connection_t* connection, uuid_t* uuid, c { int ret; + // + // No need of locking the gattlib mutex. get_characteristic_from_uuid() is taking care of the gattlib + // object coherency. And 'dbus_characteristic' is not linked to gattlib object + // + struct dbus_characteristic dbus_characteristic = get_characteristic_from_uuid(connection, uuid); if (dbus_characteristic.type == TYPE_NONE) { return GATTLIB_NOT_FOUND; @@ -410,6 +443,11 @@ int gattlib_write_char_by_handle(gattlib_connection_t* connection, uint16_t hand { int ret; + // + // No need of locking the gattlib mutex. get_characteristic_from_handle() is taking care of the gattlib + // object coherency. And 'dbus_characteristic' is not linked to gattlib object + // + struct dbus_characteristic dbus_characteristic = get_characteristic_from_handle(connection, handle); if (dbus_characteristic.type == TYPE_NONE) { return GATTLIB_NOT_FOUND; @@ -425,6 +463,11 @@ int gattlib_write_without_response_char_by_uuid(gattlib_connection_t* connection { int ret; + // + // No need of locking the gattlib mutex. get_characteristic_from_uuid() is taking care of the gattlib + // object coherency. And 'dbus_characteristic' is not linked to gattlib object + // + struct dbus_characteristic dbus_characteristic = get_characteristic_from_uuid(connection, uuid); if (dbus_characteristic.type == TYPE_NONE) { return GATTLIB_NOT_FOUND; @@ -444,6 +487,11 @@ int gattlib_write_without_response_char_by_handle(gattlib_connection_t* connecti { int ret; + // + // No need of locking the gattlib mutex. get_characteristic_from_handle() is taking care of the gattlib + // object coherency. And 'dbus_characteristic' is not linked to gattlib object + // + struct dbus_characteristic dbus_characteristic = get_characteristic_from_handle(connection, handle); if (dbus_characteristic.type == TYPE_NONE) { return GATTLIB_NOT_FOUND; diff --git a/dbus/gattlib_notification.c b/dbus/gattlib_notification.c index 0b38902..51b8a1e 100644 --- a/dbus/gattlib_notification.c +++ b/dbus/gattlib_notification.c @@ -31,6 +31,13 @@ gboolean on_handle_battery_level_property_change( g_variant_print(arg_changed_properties, TRUE), arg_invalidated_properties); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(connection)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return FALSE; + } + if (gattlib_has_valid_handler(&connection->notification)) { // Retrieve 'Value' from 'arg_changed_properties' if (g_variant_n_children (arg_changed_properties) > 0) { @@ -54,6 +61,7 @@ gboolean on_handle_battery_level_property_change( g_variant_iter_free(iter); } } + g_rec_mutex_unlock(&m_gattlib_mutex); return TRUE; } #endif @@ -66,6 +74,13 @@ static gboolean on_handle_characteristic_property_change( { gattlib_connection_t* connection = user_data; + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(connection)) { + g_rec_mutex_unlock(&m_gattlib_mutex); + return FALSE; + } + if (gattlib_has_valid_handler(&connection->notification)) { GVariantDict dict; g_variant_dict_init(&dict, arg_changed_properties); @@ -96,6 +111,8 @@ static gboolean on_handle_characteristic_property_change( } else { GATTLIB_LOG(GATTLIB_DEBUG, "on_handle_characteristic_property_change: not a notification handler"); } + + g_rec_mutex_unlock(&m_gattlib_mutex); return TRUE; } @@ -142,10 +159,17 @@ static gboolean on_handle_characteristic_indication( } static int connect_signal_to_characteristic_uuid(gattlib_connection_t* connection, const uuid_t* uuid, void *callback) { - int ret; + int ret = GATTLIB_SUCCESS; assert(callback != NULL); + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(connection)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } + struct dbus_characteristic dbus_characteristic = get_characteristic_from_uuid(connection, uuid); if (dbus_characteristic.type == TYPE_NONE) { char uuid_str[MAX_LEN_UUID_STR + 1]; @@ -153,7 +177,8 @@ static int connect_signal_to_characteristic_uuid(gattlib_connection_t* connectio gattlib_uuid_to_string(uuid, uuid_str, sizeof(uuid_str)); GATTLIB_LOG(GATTLIB_ERROR, "GATT characteristic '%s' not found", uuid_str); - return GATTLIB_NOT_FOUND; + ret = GATTLIB_NOT_FOUND; + goto EXIT; } #if BLUEZ_VERSION > BLUEZ_VERSIONS(5, 40) else if (dbus_characteristic.type == TYPE_BATTERY_LEVEL) { @@ -163,7 +188,8 @@ static int connect_signal_to_characteristic_uuid(gattlib_connection_t* connectio G_CALLBACK (on_handle_battery_level_property_change), connection); - return GATTLIB_SUCCESS; + ret = GATTLIB_SUCCESS; + goto EXIT; } else { assert(dbus_characteristic.type == TYPE_GATT); } @@ -176,33 +202,47 @@ static int connect_signal_to_characteristic_uuid(gattlib_connection_t* connectio connection); if (signal_id == 0) { GATTLIB_LOG(GATTLIB_ERROR, "Failed to connect signal to DBus GATT notification"); - return GATTLIB_ERROR_DBUS; + ret = GATTLIB_ERROR_DBUS; + goto EXIT; } // Add signal to the list struct gattlib_notification_handle *notification_handle = calloc(sizeof(struct gattlib_notification_handle), 1); if (notification_handle == NULL) { - return GATTLIB_OUT_OF_MEMORY; + ret = GATTLIB_OUT_OF_MEMORY; + goto EXIT; } notification_handle->gatt = dbus_characteristic.gatt; notification_handle->signal_id = signal_id; memcpy(¬ification_handle->uuid, uuid, sizeof(*uuid)); connection->backend.notified_characteristics = g_list_append(connection->backend.notified_characteristics, notification_handle); + // Note: An optimisation could be to release mutex here after increasing reference counter of 'dbus_characteristic.gatt' + GError *error = NULL; org_bluez_gatt_characteristic1_call_start_notify_sync(dbus_characteristic.gatt, NULL, &error); if (error) { ret = GATTLIB_ERROR_DBUS_WITH_ERROR(error); GATTLIB_LOG(GATTLIB_ERROR, "Failed to start DBus GATT notification: %s", error->message); g_error_free(error); - return ret; - } else { - return GATTLIB_SUCCESS; + goto EXIT; } + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } static int disconnect_signal_to_characteristic_uuid(gattlib_connection_t* connection, const uuid_t* uuid, void *callback) { struct gattlib_notification_handle *notification_handle = NULL; + int ret = GATTLIB_SUCCESS; + + g_rec_mutex_lock(&m_gattlib_mutex); + + if (!gattlib_connection_is_connected(connection)) { + ret = GATTLIB_INVALID_PARAMETER; + goto EXIT; + } // Find notification handle for (GList *l = connection->backend.notified_characteristics; l != NULL; l = l->next) { @@ -216,7 +256,8 @@ static int disconnect_signal_to_characteristic_uuid(gattlib_connection_t* connec } if (notification_handle == NULL) { - return GATTLIB_NOT_FOUND; + ret = GATTLIB_NOT_FOUND; + goto EXIT; } g_signal_handler_disconnect(notification_handle->gatt, notification_handle->signal_id); @@ -230,10 +271,13 @@ static int disconnect_signal_to_characteristic_uuid(gattlib_connection_t* connec if (error) { GATTLIB_LOG(GATTLIB_ERROR, "Failed to stop DBus GATT notification: %s", error->message); g_error_free(error); - return GATTLIB_NOT_FOUND; - } else { - return GATTLIB_SUCCESS; + ret = GATTLIB_NOT_FOUND; + goto EXIT; } + +EXIT: + g_rec_mutex_unlock(&m_gattlib_mutex); + return ret; } int gattlib_notification_start(gattlib_connection_t* connection, const uuid_t* uuid) { diff --git a/dbus/gattlib_stream.c b/dbus/gattlib_stream.c index bda7ed3..c383245 100644 --- a/dbus/gattlib_stream.c +++ b/dbus/gattlib_stream.c @@ -31,13 +31,19 @@ int gattlib_write_char_stream_close(gattlib_stream_t *stream) int gattlib_write_char_by_uuid_stream_open(gattlib_connection_t* connection, uuid_t* uuid, gattlib_stream_t **stream, uint16_t *mtu) { - struct dbus_characteristic dbus_characteristic = get_characteristic_from_uuid(connection, uuid); GError *error = NULL; GUnixFDList *fd_list; GVariant *out_fd; int ret; int fd; + // + // No need of locking the gattlib mutex. get_characteristic_from_uuid() is taking care of the gattlib + // object coherency. And 'dbus_characteristic' is not linked to gattlib object + // + + struct dbus_characteristic dbus_characteristic = get_characteristic_from_uuid(connection, uuid); + GVariantBuilder *variant_options = g_variant_builder_new(G_VARIANT_TYPE("a{sv}")); org_bluez_gatt_characteristic1_call_acquire_write_sync(