From b278978aa3fd92e2f6aec39b7f007fdf79ad8a5c Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 8 Mar 2024 06:28:51 +0100 Subject: [PATCH] [Linux] Reuse AdapterIterator in all places where listing managed objects (#32372) * Use dedicated iterator for listing BlueZ objects * Use GAutoPtr to manage GDBusObjectManager instance * Wrap static function with lambda * Fix iterator initialization for AdapterIterator --- src/platform/GLibTypeDeleter.h | 6 ++ src/platform/Linux/bluez/AdapterIterator.cpp | 64 +++++-------------- src/platform/Linux/bluez/AdapterIterator.h | 14 ++-- src/platform/Linux/bluez/BluezConnection.cpp | 17 ++--- src/platform/Linux/bluez/BluezEndpoint.cpp | 13 ++-- src/platform/Linux/bluez/BluezEndpoint.h | 2 +- .../Linux/bluez/BluezObjectIterator.h | 2 + src/platform/Linux/bluez/BluezObjectList.h | 29 ++++----- 8 files changed, 58 insertions(+), 89 deletions(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index f083a6c5e460d5..b1cec176ad4715 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -120,6 +120,12 @@ struct GAutoPtrDeleter using deleter = GObjectDeleter; }; +template <> +struct GAutoPtrDeleter +{ + using deleter = GObjectDeleter; +}; + template <> struct GAutoPtrDeleter { diff --git a/src/platform/Linux/bluez/AdapterIterator.cpp b/src/platform/Linux/bluez/AdapterIterator.cpp index 5510ceae996ce2..ced172446882b8 100644 --- a/src/platform/Linux/bluez/AdapterIterator.cpp +++ b/src/platform/Linux/bluez/AdapterIterator.cpp @@ -26,69 +26,38 @@ namespace chip { namespace DeviceLayer { namespace Internal { -AdapterIterator::~AdapterIterator() -{ - if (mManager != nullptr) - { - g_object_unref(mManager); - } - - if (mObjectList != nullptr) - { - g_list_free_full(mObjectList, g_object_unref); - } -} - -CHIP_ERROR AdapterIterator::Initialize(AdapterIterator * self) +CHIP_ERROR AdapterIterator::Initialize() { // When creating D-Bus proxy object, the thread default context must be initialized. Otherwise, // all D-Bus signals will be delivered to the GLib global default main context. VerifyOrDie(g_main_context_get_thread_default() != nullptr); - CHIP_ERROR err = CHIP_NO_ERROR; GAutoPtr error; - - self->mManager = g_dbus_object_manager_client_new_for_bus_sync( + mManager.reset(g_dbus_object_manager_client_new_for_bus_sync( G_BUS_TYPE_SYSTEM, G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", bluez_object_manager_client_get_proxy_type, nullptr /* unused user data in the Proxy Type Func */, - nullptr /* destroy notify */, nullptr /* cancellable */, &error.GetReceiver()); + nullptr /* destroy notify */, nullptr /* cancellable */, &error.GetReceiver())); - VerifyOrExit(self->mManager != nullptr, ChipLogError(DeviceLayer, "Failed to get DBUS object manager for listing adapters."); - err = CHIP_ERROR_INTERNAL); + VerifyOrReturnError(mManager, CHIP_ERROR_INTERNAL, + ChipLogError(DeviceLayer, "Failed to get D-Bus object manager for listing adapters: %s", error->message)); - self->mObjectList = g_dbus_object_manager_get_objects(self->mManager); - self->mCurrentListItem = self->mObjectList; + mObjectList.Init(mManager.get()); + mIterator = mObjectList.begin(); -exit: - if (error != nullptr) - { - ChipLogError(DeviceLayer, "DBus error: %s", error->message); - } - - return err; + return CHIP_NO_ERROR; } bool AdapterIterator::Advance() { - if (mCurrentListItem == nullptr) + for (; mIterator != BluezObjectList::end(); ++mIterator) { - return false; - } - - while (mCurrentListItem != nullptr) - { - BluezAdapter1 * adapter = bluez_object_get_adapter1(BLUEZ_OBJECT(mCurrentListItem->data)); - if (adapter == nullptr) + BluezAdapter1 * adapter = bluez_object_get_adapter1(&(*mIterator)); + if (adapter != nullptr) { - mCurrentListItem = mCurrentListItem->next; - continue; + mCurrentAdapter.reset(adapter); + ++mIterator; + return true; } - - mCurrentAdapter.reset(adapter); - - mCurrentListItem = mCurrentListItem->next; - - return true; } return false; @@ -112,9 +81,10 @@ uint32_t AdapterIterator::GetIndex() const bool AdapterIterator::Next() { - if (mManager == nullptr) + if (!mManager) { - CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(Initialize, this); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync( + +[](AdapterIterator * self) { return self->Initialize(); }, this); VerifyOrReturnError(err == CHIP_NO_ERROR, false, ChipLogError(DeviceLayer, "Failed to initialize adapter iterator")); } diff --git a/src/platform/Linux/bluez/AdapterIterator.h b/src/platform/Linux/bluez/AdapterIterator.h index 0d44074889773b..38af64f7ecc21c 100644 --- a/src/platform/Linux/bluez/AdapterIterator.h +++ b/src/platform/Linux/bluez/AdapterIterator.h @@ -17,13 +17,15 @@ #pragma once -#include +#include #include #include +#include #include +#include "BluezObjectList.h" #include "Types.h" namespace chip { @@ -46,8 +48,6 @@ namespace Internal { class AdapterIterator { public: - ~AdapterIterator(); - /// Moves to the next DBUS interface. /// /// MUST be called before any of the 'current value' methods are @@ -65,7 +65,7 @@ class AdapterIterator private: /// Sets up the DBUS manager and loads the list - static CHIP_ERROR Initialize(AdapterIterator * self); + CHIP_ERROR Initialize(); /// Loads the next value in the list. /// @@ -73,9 +73,9 @@ class AdapterIterator /// iterate through. bool Advance(); - GDBusObjectManager * mManager = nullptr; // DBus connection - GList * mObjectList = nullptr; // listing of objects on the bus - GList * mCurrentListItem = nullptr; // current item viewed in the list + GAutoPtr mManager; + BluezObjectList mObjectList; + BluezObjectIterator mIterator; // Data valid only if Next() returns true GAutoPtr mCurrentAdapter; }; diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 88595bd86c720c..dd4f9b108e2eb8 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -34,6 +34,7 @@ #include #include "BluezEndpoint.h" +#include "BluezObjectList.h" #include "Types.h" namespace chip { @@ -95,10 +96,6 @@ BluezConnection::ConnectionDataBundle::ConnectionDataBundle(const BluezConnectio CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) { - // populate the service and the characteristics - GList * objects = nullptr; - GList * l; - if (!aEndpoint.mIsCentral) { mpService = reinterpret_cast(g_object_ref(aEndpoint.mpService)); @@ -107,11 +104,9 @@ CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) } else { - objects = g_dbus_object_manager_get_objects(aEndpoint.mpObjMgr); - - for (l = objects; l != nullptr; l = l->next) + for (BluezObject & object : BluezObjectList(aEndpoint.mpObjMgr)) { - BluezGattService1 * service = bluez_object_get_gatt_service1(BLUEZ_OBJECT(l->data)); + BluezGattService1 * service = bluez_object_get_gatt_service1(&object); if (service != nullptr) { if ((BluezIsServiceOnDevice(service, mpDevice)) == TRUE && @@ -126,9 +121,9 @@ CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) VerifyOrExit(mpService != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL service in %s", __func__)); - for (l = objects; l != nullptr; l = l->next) + for (BluezObject & object : BluezObjectList(aEndpoint.mpObjMgr)) { - BluezGattCharacteristic1 * char1 = bluez_object_get_gatt_characteristic1(BLUEZ_OBJECT(l->data)); + BluezGattCharacteristic1 * char1 = bluez_object_get_gatt_characteristic1(&object); if (char1 != nullptr) { if ((BluezIsCharOnService(char1, mpService) == TRUE) && @@ -164,8 +159,6 @@ CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) } exit: - if (objects != nullptr) - g_list_free_full(objects, g_object_unref); return CHIP_NO_ERROR; } diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 8d37b1363e84ad..fe7a5a25171f73 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -72,6 +72,7 @@ #include #include "BluezConnection.h" +#include "BluezObjectList.h" #include "Types.h" namespace chip { @@ -420,15 +421,14 @@ BluezGattService1 * BluezEndpoint::CreateGattService(const char * aUUID) return service; } -void BluezEndpoint::SetupAdapter() +CHIP_ERROR BluezEndpoint::SetupAdapter() { char expectedPath[32]; snprintf(expectedPath, sizeof(expectedPath), BLUEZ_PATH "/hci%u", mAdapterId); - GList * objects = g_dbus_object_manager_get_objects(mpObjMgr); - for (auto l = objects; l != nullptr && mAdapter.get() == nullptr; l = l->next) + for (BluezObject & object : BluezObjectList(mpObjMgr)) { - GAutoPtr adapter(bluez_object_get_adapter1(BLUEZ_OBJECT(l->data))); + GAutoPtr adapter(bluez_object_get_adapter1(&object)); if (adapter.get() != nullptr) { if (mpAdapterAddr == nullptr) // no adapter address provided, bind to the hci indicated by nodeid @@ -450,7 +450,7 @@ void BluezEndpoint::SetupAdapter() } } - VerifyOrExit(mAdapter.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mAdapter in %s", __func__)); + VerifyOrReturnError(mAdapter, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "FAIL: NULL mAdapter in %s", __func__)); bluez_adapter1_set_powered(mAdapter.get(), TRUE); @@ -459,8 +459,7 @@ void BluezEndpoint::SetupAdapter() // and the flag is necessary to force using LE transport. bluez_adapter1_set_discoverable(mAdapter.get(), FALSE); -exit: - g_list_free_full(objects, g_object_unref); + return CHIP_NO_ERROR; } BluezConnection * BluezEndpoint::GetBluezConnection(const char * aPath) diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index d35cebdf6a6cfe..687f0002fe64b0 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -85,7 +85,7 @@ class BluezEndpoint private: CHIP_ERROR StartupEndpointBindings(); - void SetupAdapter(); + CHIP_ERROR SetupAdapter(); void SetupGattServer(GDBusConnection * aConn); void SetupGattService(); diff --git a/src/platform/Linux/bluez/BluezObjectIterator.h b/src/platform/Linux/bluez/BluezObjectIterator.h index 0e09cbcf217fc0..6b177acd03027b 100644 --- a/src/platform/Linux/bluez/BluezObjectIterator.h +++ b/src/platform/Linux/bluez/BluezObjectIterator.h @@ -17,6 +17,8 @@ #pragma once +#include + #include #include diff --git a/src/platform/Linux/bluez/BluezObjectList.h b/src/platform/Linux/bluez/BluezObjectList.h index ee7f6c001514e6..5831f07a3c6c85 100644 --- a/src/platform/Linux/bluez/BluezObjectList.h +++ b/src/platform/Linux/bluez/BluezObjectList.h @@ -35,27 +35,26 @@ namespace Internal { class BluezObjectList { public: - explicit BluezObjectList(GDBusObjectManager * manager) { Initialize(manager); } + BluezObjectList() = default; + explicit BluezObjectList(GDBusObjectManager * manager) { Init(manager); } - ~BluezObjectList() { g_list_free_full(mObjectList, g_object_unref); } - - BluezObjectIterator begin() const { return BluezObjectIterator(mObjectList); } - BluezObjectIterator end() const { return BluezObjectIterator(); } - -protected: - BluezObjectList() {} - - void Initialize(GDBusObjectManager * manager) + ~BluezObjectList() { - if (manager == nullptr) - { - ChipLogError(DeviceLayer, "Manager is NULL in %s", __func__); - return; - } + if (mObjectList != nullptr) + g_list_free_full(mObjectList, g_object_unref); + } + CHIP_ERROR Init(GDBusObjectManager * manager) + { + VerifyOrReturnError(manager != nullptr, CHIP_ERROR_INVALID_ARGUMENT, + ChipLogError(DeviceLayer, "Manager is NULL in %s", __func__)); mObjectList = g_dbus_object_manager_get_objects(manager); + return CHIP_NO_ERROR; } + BluezObjectIterator begin() const { return BluezObjectIterator(mObjectList); } + static BluezObjectIterator end() { return BluezObjectIterator(); } + private: GList * mObjectList = nullptr; };