From f27f83045fe6e1ae98b6e421234b9388ed575a6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Tue, 1 Aug 2023 11:55:22 +0000 Subject: [PATCH 01/12] Replace glib GError with GAutoPtr in AdapterIterator --- src/platform/Linux/bluez/AdapterIterator.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/platform/Linux/bluez/AdapterIterator.cpp b/src/platform/Linux/bluez/AdapterIterator.cpp index 868671e405eaf3..3197be4e73e175 100644 --- a/src/platform/Linux/bluez/AdapterIterator.cpp +++ b/src/platform/Linux/bluez/AdapterIterator.cpp @@ -19,6 +19,7 @@ #include #include +#include #include namespace chip { @@ -51,12 +52,12 @@ CHIP_ERROR AdapterIterator::Initialize(AdapterIterator * self) VerifyOrDie(g_main_context_get_thread_default() != nullptr); CHIP_ERROR err = CHIP_NO_ERROR; - GError * error = nullptr; + GAutoPtr error; - self->mManager = 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); + self->mManager = 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 */, &MakeUniquePointerReceiver(error).Get()); VerifyOrExit(self->mManager != nullptr, ChipLogError(DeviceLayer, "Failed to get DBUS object manager for listing adapters."); err = CHIP_ERROR_INTERNAL); @@ -68,7 +69,6 @@ CHIP_ERROR AdapterIterator::Initialize(AdapterIterator * self) if (error != nullptr) { ChipLogError(DeviceLayer, "DBus error: %s", error->message); - g_error_free(error); } return err; From cc130a442914e769e52af9813a0587d327f19ae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Tue, 1 Aug 2023 12:14:46 +0000 Subject: [PATCH 02/12] Replace glib GError with GAutoPtr in ChipDeviceScanner --- .../Linux/bluez/ChipDeviceScanner.cpp | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index fec7ff519b2425..4e1dba335779dc 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -209,12 +209,12 @@ CHIP_ERROR ChipDeviceScanner::StopScan() CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) { - GError * error = nullptr; + GAutoPtr error; - if (!bluez_adapter1_call_stop_discovery_sync(self->mAdapter, nullptr /* not cancellable */, &error)) + if (!bluez_adapter1_call_stop_discovery_sync(self->mAdapter, nullptr /* not cancellable */, + &MakeUniquePointerReceiver(error).Get())) { ChipLogError(Ble, "Failed to stop discovery %s", error->message); - g_error_free(error); } self->mIsScanning = false; @@ -276,18 +276,17 @@ void ChipDeviceScanner::RemoveDevice(BluezDevice1 & device) } const auto devicePath = g_dbus_proxy_get_object_path(G_DBUS_PROXY(&device)); - GError * error = nullptr; + GAutoPtr error; - if (!bluez_adapter1_call_remove_device_sync(mAdapter, devicePath, nullptr, &error)) + if (!bluez_adapter1_call_remove_device_sync(mAdapter, devicePath, nullptr, &MakeUniquePointerReceiver(error).Get())) { ChipLogDetail(Ble, "Failed to remove device %s: %s", StringOrNullMarker(devicePath), error->message); - g_error_free(error); } } CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self) { - GError * error = nullptr; + GAutoPtr error; self->mObjectAddedSignal = g_signal_connect(self->mManager, "object-added", G_CALLBACK(SignalObjectAdded), self); self->mInterfaceChangedSignal = @@ -315,18 +314,18 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self) g_variant_builder_add(&filterBuilder, "{sv}", "Transport", g_variant_new_string("le")); GVariant * filter = g_variant_builder_end(&filterBuilder); - if (!bluez_adapter1_call_set_discovery_filter_sync(self->mAdapter, filter, self->mCancellable, &error)) + if (!bluez_adapter1_call_set_discovery_filter_sync(self->mAdapter, filter, self->mCancellable, + &MakeUniquePointerReceiver(error).Get())) { // Not critical: ignore if fails ChipLogError(Ble, "Failed to set discovery filters: %s", error->message); - g_clear_error(&error); + g_clear_error(&MakeUniquePointerReceiver(error).Get()); } ChipLogProgress(Ble, "BLE initiating scan."); - if (!bluez_adapter1_call_start_discovery_sync(self->mAdapter, self->mCancellable, &error)) + if (!bluez_adapter1_call_start_discovery_sync(self->mAdapter, self->mCancellable, &MakeUniquePointerReceiver(error).Get())) { ChipLogError(Ble, "Failed to start discovery: %s", error->message); - g_error_free(error); self->mIsScanning = false; self->mDelegate->OnScanComplete(); From 2d87d7dbafe303d9d9a4e1e2e7cc98c275144acf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Wed, 2 Aug 2023 09:36:25 +0000 Subject: [PATCH 03/12] Replace glib GError with GAutoPtr in Bluez helper --- src/platform/Linux/bluez/Helper.cpp | 99 +++++++++++------------------ 1 file changed, 37 insertions(+), 62 deletions(-) diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index 3513c846dd254d..890a03c79f35ca 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -186,13 +186,14 @@ static BluezLEAdvertisement1 * BluezAdvertisingCreate(BluezEndpoint * apEndpoint static void BluezAdvStartDone(GObject * aObject, GAsyncResult * aResult, gpointer apClosure) { BluezLEAdvertisingManager1 * advMgr = BLUEZ_LEADVERTISING_MANAGER1(aObject); - GError * error = nullptr; - BluezEndpoint * endpoint = static_cast(apClosure); - gboolean success = FALSE; + GAutoPtr error; + BluezEndpoint * endpoint = static_cast(apClosure); + gboolean success = FALSE; VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - success = bluez_leadvertising_manager1_call_register_advertisement_finish(advMgr, aResult, &error); + success = + bluez_leadvertising_manager1_call_register_advertisement_finish(advMgr, aResult, &MakeUniquePointerReceiver(error).Get()); if (success == FALSE) { g_dbus_object_manager_server_unexport(endpoint->mpRoot, endpoint->mpAdvPath); @@ -205,20 +206,19 @@ static void BluezAdvStartDone(GObject * aObject, GAsyncResult * aResult, gpointe exit: BLEManagerImpl::NotifyBLEPeripheralAdvStartComplete(success == TRUE, nullptr); - if (error != nullptr) - g_error_free(error); } static void BluezAdvStopDone(GObject * aObject, GAsyncResult * aResult, gpointer apClosure) { BluezLEAdvertisingManager1 * advMgr = BLUEZ_LEADVERTISING_MANAGER1(aObject); BluezEndpoint * endpoint = static_cast(apClosure); - GError * error = nullptr; - gboolean success = FALSE; + GAutoPtr error; + gboolean success = FALSE; VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - success = bluez_leadvertising_manager1_call_unregister_advertisement_finish(advMgr, aResult, &error); + success = + bluez_leadvertising_manager1_call_unregister_advertisement_finish(advMgr, aResult, &MakeUniquePointerReceiver(error).Get()); if (success == FALSE) { @@ -235,8 +235,6 @@ static void BluezAdvStopDone(GObject * aObject, GAsyncResult * aResult, gpointer exit: BLEManagerImpl::NotifyBLEPeripheralAdvStopComplete(success == TRUE, nullptr); - if (error != nullptr) - g_error_free(error); } static CHIP_ERROR BluezAdvSetup(BluezEndpoint * endpoint) @@ -787,22 +785,19 @@ static BluezGattCharacteristic1 * BluezCharacteristicCreate(BluezGattService1 * static void BluezPeripheralRegisterAppDone(GObject * aObject, GAsyncResult * aResult, gpointer apClosure) { - GError * error = nullptr; + GAutoPtr error; BluezGattManager1 * gattMgr = BLUEZ_GATT_MANAGER1(aObject); - gboolean success = bluez_gatt_manager1_call_register_application_finish(gattMgr, aResult, &error); + gboolean success = + bluez_gatt_manager1_call_register_application_finish(gattMgr, aResult, &MakeUniquePointerReceiver(error).Get()); - VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: RegisterApplication : %s", error->message)); + VerifyOrReturn(success == TRUE, { + ChipLogError(DeviceLayer, "FAIL: RegisterApplication : %s", error->message); + BLEManagerImpl::NotifyBLEPeripheralRegisterAppComplete(false, nullptr); + }); BLEManagerImpl::NotifyBLEPeripheralRegisterAppComplete(true, nullptr); ChipLogDetail(DeviceLayer, "BluezPeripheralRegisterAppDone done"); - -exit: - if (error != nullptr) - { - BLEManagerImpl::NotifyBLEPeripheralRegisterAppComplete(false, nullptr); - g_error_free(error); - } } static CHIP_ERROR BluezPeripheralRegisterApp(BluezEndpoint * endpoint) @@ -1249,7 +1244,7 @@ static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint) static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * closure) { BluezConnection * conn = nullptr; - GError * error = nullptr; + GAutoPtr error; GIOStatus status; const char * buf; size_t len, written; @@ -1265,7 +1260,8 @@ static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * closure) buf = (char *) g_variant_get_fixed_array(closure->mpVal, &len, sizeof(uint8_t)); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); - status = g_io_channel_write_chars(conn->mC2Channel.mpChannel, buf, static_cast(len), &written, &error); + status = g_io_channel_write_chars(conn->mC2Channel.mpChannel, buf, static_cast(len), &written, + &MakeUniquePointerReceiver(error).Get()); g_variant_unref(closure->mpVal); closure->mpVal = nullptr; @@ -1287,9 +1283,6 @@ static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * closure) g_free(closure); } - if (error != nullptr) - g_error_free(error); - return CHIP_NO_ERROR; } @@ -1310,7 +1303,7 @@ CHIP_ERROR SendBluezIndication(BLE_CONNECTION_OBJECT apConn, chip::System::Packe static CHIP_ERROR BluezDisconnect(BluezConnection * conn) { - GError * error = nullptr; + GAutoPtr error; gboolean success; VerifyOrExit(conn != nullptr, ChipLogError(DeviceLayer, "conn is NULL in %s", __func__)); @@ -1318,12 +1311,10 @@ static CHIP_ERROR BluezDisconnect(BluezConnection * conn) ChipLogDetail(DeviceLayer, "%s peer=%s", __func__, bluez_device1_get_address(conn->mpDevice)); - success = bluez_device1_call_disconnect_sync(conn->mpDevice, nullptr, &error); + success = bluez_device1_call_disconnect_sync(conn->mpDevice, nullptr, &MakeUniquePointerReceiver(error).Get()); VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: Disconnect: %s", error->message)); exit: - if (error != nullptr) - g_error_free(error); return CHIP_NO_ERROR; } @@ -1447,15 +1438,11 @@ CHIP_ERROR ShutdownBluezBleLayer(BluezEndpoint * apEndpoint) static void SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { BluezGattCharacteristic1 * c1 = BLUEZ_GATT_CHARACTERISTIC1(aObject); - GError * error = nullptr; - gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c1, aResult, &error); + GAutoPtr error; + gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c1, aResult, &MakeUniquePointerReceiver(error).Get()); - VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSendWriteRequest : %s", error->message)); + VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSendWriteRequest : %s", error->message)); BLEManagerImpl::HandleWriteComplete(static_cast(apConnection)); - -exit: - if (error != nullptr) - g_error_free(error); } static CHIP_ERROR SendWriteRequestImpl(ConnectionDataBundle * data) @@ -1504,16 +1491,12 @@ static void OnCharacteristicChanged(GDBusProxy * aInterface, GVariant * aChanged static void SubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { BluezGattCharacteristic1 * c2 = BLUEZ_GATT_CHARACTERISTIC1(aObject); - GError * error = nullptr; - gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &error); + GAutoPtr error; + gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &MakeUniquePointerReceiver(error).Get()); - VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSubscribeCharacteristic : %s", error->message)); + VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSubscribeCharacteristic : %s", error->message)); BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), true); - -exit: - if (error != nullptr) - g_error_free(error); } static CHIP_ERROR SubscribeCharacteristicImpl(BluezConnection * connection) @@ -1541,18 +1524,14 @@ CHIP_ERROR BluezSubscribeCharacteristic(BLE_CONNECTION_OBJECT apConn) static void UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { BluezGattCharacteristic1 * c2 = BLUEZ_GATT_CHARACTERISTIC1(aObject); - GError * error = nullptr; - gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &error); + GAutoPtr error; + gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &MakeUniquePointerReceiver(error).Get()); - VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezUnsubscribeCharacteristic : %s", error->message)); + VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezUnsubscribeCharacteristic : %s", error->message)); // Stop listening to the TX characteristic changes g_signal_handlers_disconnect_by_data(c2, apConnection); BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), false); - -exit: - if (error != nullptr) - g_error_free(error); } static CHIP_ERROR UnsubscribeCharacteristicImpl(BluezConnection * connection) @@ -1586,9 +1565,9 @@ struct ConnectParams static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams) { - BluezDevice1 * device = BLUEZ_DEVICE1(aObject); - GError * error = nullptr; - gboolean success = bluez_device1_call_connect_finish(device, aResult, &error); + BluezDevice1 * device = BLUEZ_DEVICE1(aObject); + GAutoPtr error; + gboolean success = bluez_device1_call_connect_finish(device, aResult, &MakeUniquePointerReceiver(error).Get()); ConnectParams * params = static_cast(apParams); assert(params != nullptr); @@ -1602,14 +1581,14 @@ static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointe // BlueZ returns "Software caused connection abort error", and we should make a connection retry. // It's important to make sure that the connection is correctly ceased, by calling `Disconnect()` // D-Bus method, or else `Connect()` returns immediately without any effect. - if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_DBUS_ERROR) && params->mNumRetries++ < kMaxConnectRetries) + if (g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_DBUS_ERROR) && params->mNumRetries++ < kMaxConnectRetries) { // Clear the error before usage in subsequent call. - g_clear_error(&error); + g_clear_error(&MakeUniquePointerReceiver(error).Get()); - bluez_device1_call_disconnect_sync(device, nullptr, &error); + bluez_device1_call_disconnect_sync(device, nullptr, &MakeUniquePointerReceiver(error).Get()); bluez_device1_call_connect(device, params->mEndpoint->mpConnectCancellable, ConnectDeviceDone, params); - ExitNow(); + return; } BLEManagerImpl::HandleConnectFailed(CHIP_ERROR_INTERNAL); @@ -1620,10 +1599,6 @@ static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointe } chip::Platform::Delete(params); - -exit: - if (error != nullptr) - g_error_free(error); } static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams) From 5fe7ad8ba6e090715f941e6156076148ca9dba0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Tue, 1 Aug 2023 11:27:30 +0000 Subject: [PATCH 04/12] Replace glib GError with GAutoPtr in ConnectivityMangaerImpl --- .../Linux/ConnectivityManagerImpl.cpp | 136 +++++++----------- 1 file changed, 49 insertions(+), 87 deletions(-) diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp index b9686d8ce104f8..329fc93cd5995c 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.cpp +++ b/src/platform/Linux/ConnectivityManagerImpl.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -279,14 +280,14 @@ void ConnectivityManagerImpl::_ClearWiFiStationProvision() if (mWiFiStationMode != kWiFiStationMode_ApplicationControlled) { - GError * err = nullptr; - wpa_fi_w1_wpa_supplicant1_interface_call_remove_all_networks_sync(mWpaSupplicant.iface, nullptr, &err); + GAutoPtr err; + wpa_fi_w1_wpa_supplicant1_interface_call_remove_all_networks_sync(mWpaSupplicant.iface, nullptr, + &MakeUniquePointerReceiver(err).Get()); if (err != nullptr) { ChipLogProgress(DeviceLayer, "wpa_supplicant: failed to remove all networks with error: %s", err ? err->message : "unknown error"); - g_error_free(err); } } } @@ -397,28 +398,27 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte if (g_variant_n_children(changed_properties) > 0) { - GVariantIter * iter; + GAutoPtr iter; const gchar * key; GVariant * value; WiFiDiagnosticsDelegate * delegate = GetDiagnosticDataProvider().GetWiFiDiagnosticsDelegate(); - g_variant_get(changed_properties, "a{sv}", &iter); + g_variant_get(changed_properties, "a{sv}", &MakeUniquePointerReceiver(iter).Get()); - while (g_variant_iter_loop(iter, "{&sv}", &key, &value)) + while (g_variant_iter_loop(iter.get(), "{&sv}", &key, &value)) { - gchar * value_str; - value_str = g_variant_print(value, TRUE); + GAutoPtr value_str(g_variant_print(value, TRUE)); ChipLogProgress(DeviceLayer, "wpa_supplicant:PropertiesChanged:key:%s -> %s", StringOrNullMarker(key), - StringOrNullMarker(value_str)); + StringOrNullMarker(value_str.get())); if (g_strcmp0(key, "State") == 0) { - if (g_strcmp0(value_str, "\'associating\'") == 0) + if (g_strcmp0(value_str.get(), "\'associating\'") == 0) { mAssociationStarted = true; } - else if (g_strcmp0(value_str, "\'disconnected\'") == 0) + else if (g_strcmp0(value_str.get(), "\'disconnected\'") == 0) { gint reason = wpa_fi_w1_wpa_supplicant1_interface_get_disconnect_reason(mWpaSupplicant.iface); @@ -471,7 +471,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte mAssociationStarted = false; } - else if (g_strcmp0(value_str, "\'associated\'") == 0) + else if (g_strcmp0(value_str.get(), "\'associated\'") == 0) { if (delegate) { @@ -480,7 +480,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte DeviceLayer::SystemLayer().ScheduleLambda([]() { ConnectivityMgrImpl().UpdateNetworkStatus(); }); } - else if (g_strcmp0(value_str, "\'completed\'") == 0) + else if (g_strcmp0(value_str.get(), "\'completed\'") == 0) { if (mAssociationStarted) { @@ -496,11 +496,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte mAssociationStarted = false; } } - - g_free(value_str); } - - g_variant_iter_free(iter); } } @@ -510,11 +506,12 @@ void ConnectivityManagerImpl::_OnWpaInterfaceProxyReady(GObject * source_object, // all D-Bus signals will be delivered to the GLib global default main context. VerifyOrDie(g_main_context_get_thread_default() != nullptr); - GError * err = nullptr; + GAutoPtr err; std::lock_guard lock(mWpaSupplicantMutex); - WpaFiW1Wpa_supplicant1Interface * iface = wpa_fi_w1_wpa_supplicant1_interface_proxy_new_for_bus_finish(res, &err); + WpaFiW1Wpa_supplicant1Interface * iface = + wpa_fi_w1_wpa_supplicant1_interface_proxy_new_for_bus_finish(res, &MakeUniquePointerReceiver(err).Get()); if (mWpaSupplicant.iface) { @@ -547,9 +544,6 @@ void ConnectivityManagerImpl::_OnWpaInterfaceProxyReady(GObject * source_object, ChipLogError(DeviceLayer, "wpa_supplicant: Failed to stop auto scan: %s", ErrorStr(errInner)); } }); - - if (err != nullptr) - g_error_free(err); } void ConnectivityManagerImpl::_OnWpaBssProxyReady(GObject * source_object, GAsyncResult * res, gpointer user_data) @@ -558,11 +552,12 @@ void ConnectivityManagerImpl::_OnWpaBssProxyReady(GObject * source_object, GAsyn // all D-Bus signals will be delivered to the GLib global default main context. VerifyOrDie(g_main_context_get_thread_default() != nullptr); - GError * err = nullptr; + GAutoPtr err; std::lock_guard lock(mWpaSupplicantMutex); - WpaFiW1Wpa_supplicant1BSS * bss = wpa_fi_w1_wpa_supplicant1_bss_proxy_new_for_bus_finish(res, &err); + WpaFiW1Wpa_supplicant1BSS * bss = + wpa_fi_w1_wpa_supplicant1_bss_proxy_new_for_bus_finish(res, &MakeUniquePointerReceiver(err).Get()); if (mWpaSupplicant.bss) { @@ -570,7 +565,7 @@ void ConnectivityManagerImpl::_OnWpaBssProxyReady(GObject * source_object, GAsyn mWpaSupplicant.bss = nullptr; } - if (bss != nullptr && err == nullptr) + if (bss != nullptr && err.get() == nullptr) { mWpaSupplicant.bss = bss; ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to wpa_supplicant bss proxy"); @@ -580,9 +575,6 @@ void ConnectivityManagerImpl::_OnWpaBssProxyReady(GObject * source_object, GAsyn ChipLogProgress(DeviceLayer, "wpa_supplicant: failed to create wpa_supplicant bss proxy %s: %s", mWpaSupplicant.interfacePath, err ? err->message : "unknown error"); } - - if (err != nullptr) - g_error_free(err); } void ConnectivityManagerImpl::_OnWpaInterfaceReady(GObject * source_object, GAsyncResult * res, gpointer user_data) @@ -591,12 +583,12 @@ void ConnectivityManagerImpl::_OnWpaInterfaceReady(GObject * source_object, GAsy // all D-Bus signals will be delivered to the GLib global default main context. VerifyOrDie(g_main_context_get_thread_default() != nullptr); - GError * err = nullptr; + GAutoPtr err; std::lock_guard lock(mWpaSupplicantMutex); - gboolean result = - wpa_fi_w1_wpa_supplicant1_call_get_interface_finish(mWpaSupplicant.proxy, &mWpaSupplicant.interfacePath, res, &err); + gboolean result = wpa_fi_w1_wpa_supplicant1_call_get_interface_finish(mWpaSupplicant.proxy, &mWpaSupplicant.interfacePath, res, + &MakeUniquePointerReceiver(err).Get()); if (result) { mWpaSupplicant.state = GDBusWpaSupplicant::WPA_GOT_INTERFACE_PATH; @@ -611,7 +603,7 @@ void ConnectivityManagerImpl::_OnWpaInterfaceReady(GObject * source_object, GAsy } else { - GError * error = nullptr; + GAutoPtr error; GVariant * args = nullptr; GVariantBuilder builder; @@ -625,7 +617,7 @@ void ConnectivityManagerImpl::_OnWpaInterfaceReady(GObject * source_object, GAsy args = g_variant_builder_end(&builder); result = wpa_fi_w1_wpa_supplicant1_call_create_interface_sync(mWpaSupplicant.proxy, args, &mWpaSupplicant.interfacePath, - nullptr, &error); + nullptr, &MakeUniquePointerReceiver(error).Get()); if (result) { @@ -654,13 +646,7 @@ void ConnectivityManagerImpl::_OnWpaInterfaceReady(GObject * source_object, GAsy mWpaSupplicant.interfacePath = nullptr; } } - - if (error != nullptr) - g_error_free(error); } - - if (err != nullptr) - g_error_free(err); } void ConnectivityManagerImpl::_OnWpaInterfaceAdded(WpaFiW1Wpa_supplicant1 * proxy, const gchar * path, GVariant * properties, @@ -736,12 +722,12 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * source_object, GAsyncRe // all D-Bus signals will be delivered to the GLib global default main context. VerifyOrDie(g_main_context_get_thread_default() != nullptr); - GError * err = nullptr; + GAutoPtr err; std::lock_guard lock(mWpaSupplicantMutex); - mWpaSupplicant.proxy = wpa_fi_w1_wpa_supplicant1_proxy_new_for_bus_finish(res, &err); - if (mWpaSupplicant.proxy != nullptr && err == nullptr) + mWpaSupplicant.proxy = wpa_fi_w1_wpa_supplicant1_proxy_new_for_bus_finish(res, &MakeUniquePointerReceiver(err).Get()); + if (mWpaSupplicant.proxy != nullptr && err.get() == nullptr) { mWpaSupplicant.state = GDBusWpaSupplicant::WPA_CONNECTED; ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to wpa_supplicant proxy"); @@ -758,9 +744,6 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * source_object, GAsyncRe err ? err->message : "unknown error"); mWpaSupplicant.state = GDBusWpaSupplicant::WPA_NOT_CONNECTED; } - - if (err != nullptr) - g_error_free(err); } void ConnectivityManagerImpl::StartWiFiManagement() @@ -860,10 +843,10 @@ void ConnectivityManagerImpl::DriveAPState() { if (mWpaSupplicant.networkPath) { - GError * error = nullptr; + GAutoPtr error(nullptr); gboolean result = wpa_fi_w1_wpa_supplicant1_interface_call_remove_network_sync( - mWpaSupplicant.iface, mWpaSupplicant.networkPath, nullptr, &error); + mWpaSupplicant.iface, mWpaSupplicant.networkPath, nullptr, &MakeUniquePointerReceiver(error).Get()); if (result) { @@ -878,9 +861,6 @@ void ConnectivityManagerImpl::DriveAPState() error ? error->message : "unknown error"); err = CHIP_ERROR_INTERNAL; } - - if (error != nullptr) - g_error_free(error); } } } @@ -896,8 +876,8 @@ void ConnectivityManagerImpl::DriveAPState() CHIP_ERROR ConnectivityManagerImpl::ConfigureWiFiAP() { - CHIP_ERROR ret = CHIP_NO_ERROR; - GError * err = nullptr; + CHIP_ERROR ret = CHIP_NO_ERROR; + GAutoPtr err; GVariant * args = nullptr; GVariantBuilder builder; @@ -930,17 +910,17 @@ CHIP_ERROR ConnectivityManagerImpl::ConfigureWiFiAP() g_variant_builder_add(&builder, "{sv}", "frequency", g_variant_new_int32(channel)); args = g_variant_builder_end(&builder); - gboolean result = wpa_fi_w1_wpa_supplicant1_interface_call_add_network_sync(mWpaSupplicant.iface, args, - &mWpaSupplicant.networkPath, nullptr, &err); + gboolean result = wpa_fi_w1_wpa_supplicant1_interface_call_add_network_sync( + mWpaSupplicant.iface, args, &mWpaSupplicant.networkPath, nullptr, &MakeUniquePointerReceiver(err).Get()); if (result) { - GError * error = nullptr; + GAutoPtr error; ChipLogProgress(DeviceLayer, "wpa_supplicant: added network: SSID: %s: %s", ssid, mWpaSupplicant.networkPath); result = wpa_fi_w1_wpa_supplicant1_interface_call_select_network_sync(mWpaSupplicant.iface, mWpaSupplicant.networkPath, - nullptr, &error); + nullptr, &MakeUniquePointerReceiver(error).Get()); if (result) { ChipLogProgress(DeviceLayer, "wpa_supplicant: succeeded to start softAP: SSID: %s", ssid); @@ -952,9 +932,6 @@ CHIP_ERROR ConnectivityManagerImpl::ConfigureWiFiAP() ret = CHIP_ERROR_INTERNAL; } - - if (error != nullptr) - g_error_free(error); } else { @@ -969,9 +946,6 @@ CHIP_ERROR ConnectivityManagerImpl::ConfigureWiFiAP() ret = CHIP_ERROR_INTERNAL; } - if (err != nullptr) - g_error_free(err); - return ret; } @@ -993,8 +967,8 @@ CHIP_ERROR ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credentials, NetworkCommissioning::Internal::WirelessDriver::ConnectCallback * apCallback) { - CHIP_ERROR ret = CHIP_NO_ERROR; - GError * err = nullptr; + CHIP_ERROR ret = CHIP_NO_ERROR; + GAutoPtr err; GVariant * args = nullptr; GVariantBuilder builder; gboolean result; @@ -1015,9 +989,10 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent // wpa_supplicant DBus API: if network path of current network is not "/", means we have already selected some network. if (strcmp(networkPath, "/") != 0) { - GError * error = nullptr; + GAutoPtr error; - result = wpa_fi_w1_wpa_supplicant1_interface_call_remove_network_sync(mWpaSupplicant.iface, networkPath, nullptr, &error); + result = wpa_fi_w1_wpa_supplicant1_interface_call_remove_network_sync(mWpaSupplicant.iface, networkPath, nullptr, + &MakeUniquePointerReceiver(error).Get()); if (result) { @@ -1035,9 +1010,6 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent ret = CHIP_ERROR_INTERNAL; } - if (error != nullptr) - g_error_free(error); - SuccessOrExit(ret); } @@ -1050,7 +1022,7 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent args = g_variant_builder_end(&builder); result = wpa_fi_w1_wpa_supplicant1_interface_call_add_network_sync(mWpaSupplicant.iface, args, &mWpaSupplicant.networkPath, - nullptr, &err); + nullptr, &MakeUniquePointerReceiver(err).Get()); if (result) { @@ -1078,9 +1050,6 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent } exit: - if (err != nullptr) - g_error_free(err); - return ret; } @@ -1110,20 +1079,16 @@ void ConnectivityManagerImpl::_ConnectWiFiNetworkAsyncCallback(GObject * source_ return; } - GError * gerror = nullptr; - - result = wpa_fi_w1_wpa_supplicant1_interface_call_save_config_sync(mWpaSupplicant.iface, nullptr, &gerror); + result = wpa_fi_w1_wpa_supplicant1_interface_call_save_config_sync(mWpaSupplicant.iface, nullptr, + &MakeUniquePointerReceiver(err).Get()); if (result) { ChipLogProgress(DeviceLayer, "wpa_supplicant: save config succeeded!"); } else { - ChipLogProgress(DeviceLayer, "wpa_supplicant: failed to save config: %s", gerror ? gerror->message : "unknown error"); + ChipLogProgress(DeviceLayer, "wpa_supplicant: failed to save config: %s", err ? err->message : "unknown error"); } - - if (gerror != nullptr) - g_error_free(gerror); } } @@ -1388,8 +1353,8 @@ CHIP_ERROR ConnectivityManagerImpl::StartWiFiScan(ByteSpan ssid, WiFiDriver::Sca VerifyOrReturnError(mpScanCallback == nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(ssid.size() <= sizeof(sInterestedSSID), CHIP_ERROR_INVALID_ARGUMENT); - CHIP_ERROR ret = CHIP_NO_ERROR; - GError * err = nullptr; + CHIP_ERROR ret = CHIP_NO_ERROR; + GAutoPtr err; GVariant * args = nullptr; GVariantBuilder builder; gboolean result; @@ -1401,7 +1366,8 @@ CHIP_ERROR ConnectivityManagerImpl::StartWiFiScan(ByteSpan ssid, WiFiDriver::Sca g_variant_builder_add(&builder, "{sv}", "Type", g_variant_new_string("active")); args = g_variant_builder_end(&builder); - result = wpa_fi_w1_wpa_supplicant1_interface_call_scan_sync(mWpaSupplicant.iface, args, nullptr, &err); + result = wpa_fi_w1_wpa_supplicant1_interface_call_scan_sync(mWpaSupplicant.iface, args, nullptr, + &MakeUniquePointerReceiver(err).Get()); if (result) { @@ -1414,10 +1380,6 @@ CHIP_ERROR ConnectivityManagerImpl::StartWiFiScan(ByteSpan ssid, WiFiDriver::Sca ret = CHIP_ERROR_INTERNAL; } - if (err != nullptr) - { - g_error_free(err); - } return ret; } From 703ce5ec035ff527d864b1569e658f7a765eeda0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Wed, 9 Aug 2023 08:38:20 +0000 Subject: [PATCH 05/12] Replace glib GVariant with GAutoPtr in ConnectivityManagerImpl --- src/platform/Linux/ConnectivityManagerImpl.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp index 329fc93cd5995c..5d624a022c9e4f 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.cpp +++ b/src/platform/Linux/ConnectivityManagerImpl.cpp @@ -1552,12 +1552,12 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi return 0; } - GVariant * keyMgmt = g_variant_lookup_value(wpa, "KeyMgmt", nullptr); + GAutoPtr keyMgmt(g_variant_lookup_value(wpa, "KeyMgmt", nullptr)); if (keyMgmt == nullptr) { return 0; } - const gchar ** keyMgmts = g_variant_get_strv(keyMgmt, nullptr); + const gchar ** keyMgmts = g_variant_get_strv(MakeUniquePointerReceiver(keyMgmt).Get(), nullptr); const gchar ** keyMgmtsForFree = keyMgmts; uint8_t res = 0; for (const gchar * keyMgmtVal = (keyMgmts != nullptr ? *keyMgmts : nullptr); keyMgmtVal != nullptr; @@ -1572,7 +1572,6 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi res |= (kEAP); } } - g_variant_unref(keyMgmt); g_free(keyMgmtsForFree); return res; }; @@ -1581,12 +1580,12 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi { return 0; } - GVariant * keyMgmt = g_variant_lookup_value(rsn, "KeyMgmt", nullptr); + GAutoPtr keyMgmt(g_variant_lookup_value(rsn, "KeyMgmt", nullptr)); if (keyMgmt == nullptr) { return 0; } - const gchar ** keyMgmts = g_variant_get_strv(keyMgmt, nullptr); + const gchar ** keyMgmts = g_variant_get_strv(MakeUniquePointerReceiver(keyMgmt).Get(), nullptr); const gchar ** keyMgmtsForFree = keyMgmts; uint8_t res = 0; for (const gchar * keyMgmtVal = (keyMgmts != nullptr ? *keyMgmts : nullptr); keyMgmtVal != nullptr; @@ -1609,7 +1608,6 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi res |= (1 << 4); // SecurityType::WPA3_PERSONAL } } - g_variant_unref(keyMgmt); g_free(keyMgmtsForFree); return res; }; From b436674a79571cf7a3b0287f54cca8d73700997e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Wed, 9 Aug 2023 09:33:16 +0000 Subject: [PATCH 06/12] Replace glib GVariant with GAutoPtr in Bluez helper --- src/platform/Linux/bluez/Helper.cpp | 44 +++++++++++------------------ src/platform/Linux/bluez/Types.h | 3 +- 2 files changed, 19 insertions(+), 28 deletions(-) diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index 890a03c79f35ca..d8ea2ea9335c5b 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -379,8 +379,8 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; GVariantDict * options = nullptr; - GVariant * option_mtu = nullptr; - bool isSuccess = false; + GAutoPtr option_mtu; + bool isSuccess = false; BluezEndpoint * endpoint = static_cast(apEndpoint); VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); @@ -402,11 +402,11 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar } options = g_variant_dict_new(aOptions); - option_mtu = g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16); + option_mtu = GAutoPtr(g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16)); VerifyOrExit( option_mtu != nullptr, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); - conn->mMtu = g_variant_get_uint16(option_mtu); + conn->mMtu = g_variant_get_uint16(MakeUniquePointerReceiver(option_mtu).Get()); channel = g_io_channel_unix_new(fds[0]); g_io_channel_set_encoding(channel, nullptr, nullptr); @@ -430,10 +430,7 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar { g_variant_dict_unref(options); } - if (option_mtu != nullptr) - { - g_variant_unref(option_mtu); - } + return isSuccess ? TRUE : FALSE; } @@ -457,8 +454,8 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; GVariantDict * options = nullptr; - GVariant * option_mtu = nullptr; - bool isSuccess = false; + GAutoPtr option_mtu; + bool isSuccess = false; BluezEndpoint * endpoint = static_cast(apEndpoint); VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); @@ -474,11 +471,11 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); options = g_variant_dict_new(aOptions); - option_mtu = g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16); + option_mtu = GAutoPtr(g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16)); VerifyOrExit( option_mtu != nullptr, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); - conn->mMtu = g_variant_get_uint16(option_mtu); + conn->mMtu = g_variant_get_uint16(MakeUniquePointerReceiver(option_mtu).Get()); if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, 0, fds) < 0) { @@ -516,10 +513,7 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha { g_variant_dict_unref(options); } - if (option_mtu != nullptr) - { - g_variant_unref(option_mtu); - } + return isSuccess ? TRUE : FALSE; } @@ -1257,29 +1251,25 @@ static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * closure) if (bluez_gatt_characteristic1_get_notify_acquired(conn->mpC2) == TRUE) { - buf = (char *) g_variant_get_fixed_array(closure->mpVal, &len, sizeof(uint8_t)); + buf = (char *) g_variant_get_fixed_array(MakeUniquePointerReceiver(closure->mpVal).Get(), &len, sizeof(uint8_t)); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); status = g_io_channel_write_chars(conn->mC2Channel.mpChannel, buf, static_cast(len), &written, &MakeUniquePointerReceiver(error).Get()); - g_variant_unref(closure->mpVal); + g_variant_unref(MakeUniquePointerReceiver(closure->mpVal).Get()); closure->mpVal = nullptr; VerifyOrExit(status == G_IO_STATUS_NORMAL, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", error->message)); } else { - bluez_gatt_characteristic1_set_value(conn->mpC2, closure->mpVal); + bluez_gatt_characteristic1_set_value(conn->mpC2, MakeUniquePointerReceiver(closure->mpVal).Get()); closure->mpVal = nullptr; } exit: if (closure != nullptr) { - if (closure->mpVal) - { - g_variant_unref(closure->mpVal); - } g_free(closure); } @@ -1290,8 +1280,8 @@ static ConnectionDataBundle * MakeConnectionDataBundle(BLE_CONNECTION_OBJECT apC { ConnectionDataBundle * bundle = g_new(ConnectionDataBundle, 1); bundle->mpConn = static_cast(apConn); - bundle->mpVal = - g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength() * sizeof(uint8_t), sizeof(uint8_t)); + bundle->mpVal = GAutoPtr( + g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength() * sizeof(uint8_t), sizeof(uint8_t))); return bundle; } @@ -1458,8 +1448,8 @@ static CHIP_ERROR SendWriteRequestImpl(ConnectionDataBundle * data) g_variant_builder_add(&optionsBuilder, "{sv}", "type", g_variant_new_string("request")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, data->mpVal, options, nullptr, SendWriteRequestDone, - data->mpConn); + bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, MakeUniquePointerReceiver(data->mpVal).Get(), options, nullptr, + SendWriteRequestDone, data->mpConn); exit: g_free(data); diff --git a/src/platform/Linux/bluez/Types.h b/src/platform/Linux/bluez/Types.h index 3f61c430a5f7a2..3130c44c54506e 100644 --- a/src/platform/Linux/bluez/Types.h +++ b/src/platform/Linux/bluez/Types.h @@ -50,6 +50,7 @@ #if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE #include +#include #include #include @@ -187,7 +188,7 @@ struct BluezConnection struct ConnectionDataBundle { BluezConnection * mpConn; - GVariant * mpVal; + GAutoPtr mpVal; }; } // namespace Internal From 14440b9e656616ad252eca0d1a21eb6a8e499a04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Wed, 20 Sep 2023 09:37:12 +0200 Subject: [PATCH 07/12] Replace glib and char with GAutoPtr in Bluez helper --- src/platform/Linux/bluez/Helper.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index d8ea2ea9335c5b..95e84e89bfd03a 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -111,10 +111,10 @@ static BluezLEAdvertisement1 * BluezAdvertisingCreate(BluezEndpoint * apEndpoint BluezObjectSkeleton * object; GVariant * serviceData; GVariant * serviceUUID; - gchar * localName = nullptr; + GAutoPtr localName; GVariantBuilder serviceDataBuilder; GVariantBuilder serviceUUIDsBuilder; - char * debugStr; + GAutoPtr debugStr; VerifyOrExit(apEndpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); if (apEndpoint->mpAdvPath == nullptr) @@ -134,16 +134,15 @@ static BluezLEAdvertisement1 * BluezAdvertisingCreate(BluezEndpoint * apEndpoint g_variant_builder_add(&serviceUUIDsBuilder, "s", apEndpoint->mpAdvertisingUUID); if (apEndpoint->mpAdapterName != nullptr) - localName = g_strdup_printf("%s", apEndpoint->mpAdapterName); + localName = GAutoPtr(g_strdup_printf("%s", apEndpoint->mpAdapterName)); else - localName = g_strdup_printf("%s%04x", CHIP_DEVICE_CONFIG_BLE_DEVICE_NAME_PREFIX, getpid() & 0xffff); + localName = GAutoPtr(g_strdup_printf("%s%04x", CHIP_DEVICE_CONFIG_BLE_DEVICE_NAME_PREFIX, getpid() & 0xffff)); serviceData = g_variant_builder_end(&serviceDataBuilder); serviceUUID = g_variant_builder_end(&serviceUUIDsBuilder); - debugStr = g_variant_print(serviceData, TRUE); - ChipLogDetail(DeviceLayer, "SET service data to %s", StringOrNullMarker(debugStr)); - g_free(debugStr); + debugStr = GAutoPtr(g_variant_print(serviceData, TRUE)); + ChipLogDetail(DeviceLayer, "SET service data to %s", StringOrNullMarker(MakeUniquePointerReceiver(debugStr).Get())); bluez_leadvertisement1_set_type_(adv, (apEndpoint->mType & BLUEZ_ADV_TYPE_CONNECTABLE) ? "peripheral" : "broadcast"); // empty manufacturer data @@ -159,7 +158,7 @@ static BluezLEAdvertisement1 * BluezAdvertisingCreate(BluezEndpoint * apEndpoint bluez_leadvertisement1_set_discoverable_timeout(adv, UINT16_MAX); // advertising name corresponding to the PID and object path, for debug purposes - bluez_leadvertisement1_set_local_name(adv, localName); + bluez_leadvertisement1_set_local_name(adv, MakeUniquePointerReceiver(localName).Get()); bluez_leadvertisement1_set_service_uuids(adv, serviceUUID); // 0xffff means no appearance @@ -179,7 +178,6 @@ static BluezLEAdvertisement1 * BluezAdvertisingCreate(BluezEndpoint * apEndpoint BLEManagerImpl::NotifyBLEPeripheralAdvConfiguredComplete(true, nullptr); exit: - g_free(localName); return adv; } From 177a7bbb1bafddf27699a4102ae5981e83d18dd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Wed, 20 Sep 2023 14:27:22 +0200 Subject: [PATCH 08/12] Replace glib and GVariantDict with GAutoPtr in Bluez helper - review --- src/platform/GLibTypeDeleter.h | 11 +++++ src/platform/Linux/bluez/Helper.cpp | 68 ++++++++++++----------------- 2 files changed, 40 insertions(+), 39 deletions(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index 1e187aa275adb3..c10ee2dbc916d2 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -65,6 +65,11 @@ struct GVariantDeleter void operator()(GVariant * object) { g_variant_unref(object); } }; +struct GVariantDictDeleter +{ + void operator()(GVariantDict * object) { g_variant_dict_unref(object); } +}; + struct GVariantIterDeleter { void operator()(GVariantIter * object) { g_variant_iter_free(object); } @@ -116,6 +121,12 @@ struct GAutoPtrDeleter using deleter = GVariantDeleter; }; +template <> +struct GAutoPtrDeleter +{ + using deleter = GVariantDictDeleter; +}; + template <> struct GAutoPtrDeleter { diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index 95e84e89bfd03a..43cea897effd27 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -376,16 +376,17 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar char * errStr; #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; - GVariantDict * options = nullptr; + GAutoPtr options; GAutoPtr option_mtu; - bool isSuccess = false; BluezEndpoint * endpoint = static_cast(apEndpoint); - VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); + + VerifyOrReturnValue(endpoint != nullptr, FALSE, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); conn = GetBluezConnectionViaDevice(endpoint); - VerifyOrExit(conn != nullptr, - g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); + VerifyOrReturnValue( + conn != nullptr, FALSE, + g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); ChipLogDetail(DeviceLayer, "BluezCharacteristicAcquireWrite is called, conn: %p", conn); @@ -396,13 +397,14 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar #endif // CHIP_ERROR_LOGGING ChipLogError(DeviceLayer, "FAIL: socketpair: %s in %s", StringOrNullMarker(errStr), __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "FD creation failed"); - goto exit; + return FALSE; } - options = g_variant_dict_new(aOptions); - option_mtu = GAutoPtr(g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16)); - VerifyOrExit( - option_mtu != nullptr, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); + options = GAutoPtr(g_variant_dict_new(aOptions)); + option_mtu = + GAutoPtr(g_variant_dict_lookup_value(MakeUniquePointerReceiver(options).Get(), "mtu", G_VARIANT_TYPE_UINT16)); + VerifyOrReturnValue( + option_mtu != nullptr, FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); conn->mMtu = g_variant_get_uint16(MakeUniquePointerReceiver(option_mtu).Get()); @@ -421,15 +423,8 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->mMtu); close(fds[1]); - isSuccess = true; -exit: - if (options != nullptr) - { - g_variant_dict_unref(options); - } - - return isSuccess ? TRUE : FALSE; + return TRUE; } static gboolean BluezCharacteristicAcquireWriteError(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, @@ -451,28 +446,30 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha char * errStr; #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; - GVariantDict * options = nullptr; + GAutoPtr options; GAutoPtr option_mtu; - bool isSuccess = false; BluezEndpoint * endpoint = static_cast(apEndpoint); - VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); + VerifyOrReturnValue(endpoint != nullptr, FALSE, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); if (bluez_gatt_characteristic1_get_notifying(aChar)) { g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.NotPermitted", "Already notifying"); - goto exit; + return FALSE; } conn = GetBluezConnectionViaDevice(endpoint); - VerifyOrExit(conn != nullptr, - g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); - - options = g_variant_dict_new(aOptions); - option_mtu = GAutoPtr(g_variant_dict_lookup_value(options, "mtu", G_VARIANT_TYPE_UINT16)); - VerifyOrExit( - option_mtu != nullptr, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); - g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); + VerifyOrReturnValue( + conn != nullptr, FALSE, + g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); + + options = GAutoPtr(g_variant_dict_new(aOptions)); + option_mtu = + GAutoPtr(g_variant_dict_lookup_value(MakeUniquePointerReceiver(options).Get(), "mtu", G_VARIANT_TYPE_UINT16)); + VerifyOrReturnValue(option_mtu != nullptr, FALSE, { + ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); + g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed"); + }); conn->mMtu = g_variant_get_uint16(MakeUniquePointerReceiver(option_mtu).Get()); if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, 0, fds) < 0) @@ -482,7 +479,7 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha #endif // CHIP_ERROR_LOGGING ChipLogError(DeviceLayer, "FAIL: socketpair: %s in %s", StringOrNullMarker(errStr), __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "FD creation failed"); - goto exit; + return FALSE; } channel = g_io_channel_unix_new(fds[0]); @@ -504,15 +501,8 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha conn->mIsNotify = true; BLEManagerImpl::HandleTXCharCCCDWrite(conn); - isSuccess = true; -exit: - if (options != nullptr) - { - g_variant_dict_unref(options); - } - - return isSuccess ? TRUE : FALSE; + return TRUE; } static gboolean BluezCharacteristicAcquireNotifyError(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, From 5323bd4b75ea5ed291726da7f2c00173c18e4c1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Tue, 26 Sep 2023 13:58:45 +0200 Subject: [PATCH 09/12] Fix badly used MakeUniquePointerReceiver --- .../Linux/ConnectivityManagerImpl.cpp | 4 +-- src/platform/Linux/bluez/Helper.cpp | 28 +++++++++---------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp index 5d624a022c9e4f..388d5fc826d23a 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.cpp +++ b/src/platform/Linux/ConnectivityManagerImpl.cpp @@ -1557,7 +1557,7 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi { return 0; } - const gchar ** keyMgmts = g_variant_get_strv(MakeUniquePointerReceiver(keyMgmt).Get(), nullptr); + const gchar ** keyMgmts = g_variant_get_strv(keyMgmt.get(), nullptr); const gchar ** keyMgmtsForFree = keyMgmts; uint8_t res = 0; for (const gchar * keyMgmtVal = (keyMgmts != nullptr ? *keyMgmts : nullptr); keyMgmtVal != nullptr; @@ -1585,7 +1585,7 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi { return 0; } - const gchar ** keyMgmts = g_variant_get_strv(MakeUniquePointerReceiver(keyMgmt).Get(), nullptr); + const gchar ** keyMgmts = g_variant_get_strv(keyMgmt.get(), nullptr); const gchar ** keyMgmtsForFree = keyMgmts; uint8_t res = 0; for (const gchar * keyMgmtVal = (keyMgmts != nullptr ? *keyMgmts : nullptr); keyMgmtVal != nullptr; diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index 43cea897effd27..4e72014c53df06 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -142,7 +142,7 @@ static BluezLEAdvertisement1 * BluezAdvertisingCreate(BluezEndpoint * apEndpoint serviceUUID = g_variant_builder_end(&serviceUUIDsBuilder); debugStr = GAutoPtr(g_variant_print(serviceData, TRUE)); - ChipLogDetail(DeviceLayer, "SET service data to %s", StringOrNullMarker(MakeUniquePointerReceiver(debugStr).Get())); + ChipLogDetail(DeviceLayer, "SET service data to %s", StringOrNullMarker(debugStr.get())); bluez_leadvertisement1_set_type_(adv, (apEndpoint->mType & BLUEZ_ADV_TYPE_CONNECTABLE) ? "peripheral" : "broadcast"); // empty manufacturer data @@ -158,7 +158,7 @@ static BluezLEAdvertisement1 * BluezAdvertisingCreate(BluezEndpoint * apEndpoint bluez_leadvertisement1_set_discoverable_timeout(adv, UINT16_MAX); // advertising name corresponding to the PID and object path, for debug purposes - bluez_leadvertisement1_set_local_name(adv, MakeUniquePointerReceiver(localName).Get()); + bluez_leadvertisement1_set_local_name(adv, localName.get()); bluez_leadvertisement1_set_service_uuids(adv, serviceUUID); // 0xffff means no appearance @@ -400,13 +400,12 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar return FALSE; } - options = GAutoPtr(g_variant_dict_new(aOptions)); - option_mtu = - GAutoPtr(g_variant_dict_lookup_value(MakeUniquePointerReceiver(options).Get(), "mtu", G_VARIANT_TYPE_UINT16)); + options = GAutoPtr(g_variant_dict_new(aOptions)); + option_mtu = GAutoPtr(g_variant_dict_lookup_value(options.get(), "mtu", G_VARIANT_TYPE_UINT16)); VerifyOrReturnValue( option_mtu != nullptr, FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); - conn->mMtu = g_variant_get_uint16(MakeUniquePointerReceiver(option_mtu).Get()); + conn->mMtu = g_variant_get_uint16(option_mtu.get()); channel = g_io_channel_unix_new(fds[0]); g_io_channel_set_encoding(channel, nullptr, nullptr); @@ -463,14 +462,13 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); - options = GAutoPtr(g_variant_dict_new(aOptions)); - option_mtu = - GAutoPtr(g_variant_dict_lookup_value(MakeUniquePointerReceiver(options).Get(), "mtu", G_VARIANT_TYPE_UINT16)); + options = GAutoPtr(g_variant_dict_new(aOptions)); + option_mtu = GAutoPtr(g_variant_dict_lookup_value(options.get(), "mtu", G_VARIANT_TYPE_UINT16)); VerifyOrReturnValue(option_mtu != nullptr, FALSE, { ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed"); }); - conn->mMtu = g_variant_get_uint16(MakeUniquePointerReceiver(option_mtu).Get()); + conn->mMtu = g_variant_get_uint16(option_mtu.get()); if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, 0, fds) < 0) { @@ -1239,19 +1237,19 @@ static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * closure) if (bluez_gatt_characteristic1_get_notify_acquired(conn->mpC2) == TRUE) { - buf = (char *) g_variant_get_fixed_array(MakeUniquePointerReceiver(closure->mpVal).Get(), &len, sizeof(uint8_t)); + buf = (char *) g_variant_get_fixed_array(closure->mpVal.get(), &len, sizeof(uint8_t)); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); status = g_io_channel_write_chars(conn->mC2Channel.mpChannel, buf, static_cast(len), &written, &MakeUniquePointerReceiver(error).Get()); - g_variant_unref(MakeUniquePointerReceiver(closure->mpVal).Get()); + g_variant_unref(closure->mpVal.get()); closure->mpVal = nullptr; VerifyOrExit(status == G_IO_STATUS_NORMAL, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", error->message)); } else { - bluez_gatt_characteristic1_set_value(conn->mpC2, MakeUniquePointerReceiver(closure->mpVal).Get()); + bluez_gatt_characteristic1_set_value(conn->mpC2, closure->mpVal.get()); closure->mpVal = nullptr; } @@ -1436,8 +1434,8 @@ static CHIP_ERROR SendWriteRequestImpl(ConnectionDataBundle * data) g_variant_builder_add(&optionsBuilder, "{sv}", "type", g_variant_new_string("request")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, MakeUniquePointerReceiver(data->mpVal).Get(), options, nullptr, - SendWriteRequestDone, data->mpConn); + bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, data->mpVal.get(), options, nullptr, SendWriteRequestDone, + data->mpConn); exit: g_free(data); From e75c6b83629b15d632ff91f2e4a52995f4722b9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Mon, 2 Oct 2023 12:29:20 +0200 Subject: [PATCH 10/12] Review changes * Replace g_variant_dict_lookup_value to g_variant_lookup_value in src/platform/Linux/bluez/Helper.cpp * Add template <> struct GAutoPtrDeleter * Convert keyMgmts to GAutoPtr in src/platform/Linux/ConnectivityManagerImpl.cpp * Revert in ConnectionDataBundle GVariant convert to GAutoPtr --- src/platform/GLibTypeDeleter.h | 6 +++++ .../Linux/ConnectivityManagerImpl.cpp | 24 +++++++++---------- src/platform/Linux/bluez/Helper.cpp | 23 +++++++----------- src/platform/Linux/bluez/Types.h | 2 +- 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index c10ee2dbc916d2..4918d8b653220e 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -91,6 +91,12 @@ struct GAutoPtrDeleter using deleter = GFree; }; +template <> +struct GAutoPtrDeleter +{ + using deleter = GFree; +}; + template <> struct GAutoPtrDeleter { diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp index 388d5fc826d23a..34217cc65555aa 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.cpp +++ b/src/platform/Linux/ConnectivityManagerImpl.cpp @@ -1557,11 +1557,11 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi { return 0; } - const gchar ** keyMgmts = g_variant_get_strv(keyMgmt.get(), nullptr); - const gchar ** keyMgmtsForFree = keyMgmts; - uint8_t res = 0; - for (const gchar * keyMgmtVal = (keyMgmts != nullptr ? *keyMgmts : nullptr); keyMgmtVal != nullptr; - keyMgmtVal = *(++keyMgmts)) + GAutoPtr keyMgmts(g_variant_get_strv(keyMgmt.get(), nullptr)); + const gchar ** keyMgmtsHendle = keyMgmts.get(); + uint8_t res = 0; + for (const gchar * keyMgmtVal = (keyMgmtsHendle != nullptr ? *keyMgmtsHendle : nullptr); keyMgmtVal != nullptr; + keyMgmtVal = *(++keyMgmtsHendle)) { if (g_strcasecmp(keyMgmtVal, "wpa-psk") == 0 || g_strcasecmp(keyMgmtVal, "wpa-none") == 0) { @@ -1572,7 +1572,7 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi res |= (kEAP); } } - g_free(keyMgmtsForFree); + return res; }; auto IsNetworkWPA2PSK = [](GVariant * rsn) -> uint8_t { @@ -1585,11 +1585,11 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi { return 0; } - const gchar ** keyMgmts = g_variant_get_strv(keyMgmt.get(), nullptr); - const gchar ** keyMgmtsForFree = keyMgmts; - uint8_t res = 0; - for (const gchar * keyMgmtVal = (keyMgmts != nullptr ? *keyMgmts : nullptr); keyMgmtVal != nullptr; - keyMgmtVal = *(++keyMgmts)) + GAutoPtr keyMgmts(g_variant_get_strv(keyMgmt.get(), nullptr)); + const gchar ** keyMgmtsHendle = keyMgmts.get(); + uint8_t res = 0; + for (const gchar * keyMgmtVal = (keyMgmtsHendle != nullptr ? *keyMgmtsHendle : nullptr); keyMgmtVal != nullptr; + keyMgmtVal = *(++keyMgmtsHendle)) { if (g_strcasecmp(keyMgmtVal, "wpa-psk") == 0 || g_strcasecmp(keyMgmtVal, "wpa-psk-sha256") == 0 || g_strcasecmp(keyMgmtVal, "wpa-ft-psk") == 0) @@ -1608,7 +1608,7 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi res |= (1 << 4); // SecurityType::WPA3_PERSONAL } } - g_free(keyMgmtsForFree); + return res; }; auto GetNetworkSecurityType = [IsNetworkWPAPSK, IsNetworkWPA2PSK](WpaFiW1Wpa_supplicant1BSSProxy * proxy) -> uint8_t { diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index 4e72014c53df06..b73e3c5770b835 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -400,12 +400,10 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar return FALSE; } - options = GAutoPtr(g_variant_dict_new(aOptions)); - option_mtu = GAutoPtr(g_variant_dict_lookup_value(options.get(), "mtu", G_VARIANT_TYPE_UINT16)); VerifyOrReturnValue( - option_mtu != nullptr, FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); + g_variant_lookup(aOptions, "mtu", "q", &conn->mMtu), FALSE, + ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); - conn->mMtu = g_variant_get_uint16(option_mtu.get()); channel = g_io_channel_unix_new(fds[0]); g_io_channel_set_encoding(channel, nullptr, nullptr); @@ -462,13 +460,10 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); - options = GAutoPtr(g_variant_dict_new(aOptions)); - option_mtu = GAutoPtr(g_variant_dict_lookup_value(options.get(), "mtu", G_VARIANT_TYPE_UINT16)); - VerifyOrReturnValue(option_mtu != nullptr, FALSE, { + VerifyOrReturnValue(g_variant_lookup(aOptions, "mtu", "q", &conn->mMtu), FALSE, { ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed"); }); - conn->mMtu = g_variant_get_uint16(option_mtu.get()); if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, 0, fds) < 0) { @@ -1237,19 +1232,19 @@ static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * closure) if (bluez_gatt_characteristic1_get_notify_acquired(conn->mpC2) == TRUE) { - buf = (char *) g_variant_get_fixed_array(closure->mpVal.get(), &len, sizeof(uint8_t)); + buf = (char *) g_variant_get_fixed_array(closure->mpVal, &len, sizeof(uint8_t)); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); status = g_io_channel_write_chars(conn->mC2Channel.mpChannel, buf, static_cast(len), &written, &MakeUniquePointerReceiver(error).Get()); - g_variant_unref(closure->mpVal.get()); + g_variant_unref(closure->mpVal); closure->mpVal = nullptr; VerifyOrExit(status == G_IO_STATUS_NORMAL, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", error->message)); } else { - bluez_gatt_characteristic1_set_value(conn->mpC2, closure->mpVal.get()); + bluez_gatt_characteristic1_set_value(conn->mpC2, closure->mpVal); closure->mpVal = nullptr; } @@ -1266,8 +1261,8 @@ static ConnectionDataBundle * MakeConnectionDataBundle(BLE_CONNECTION_OBJECT apC { ConnectionDataBundle * bundle = g_new(ConnectionDataBundle, 1); bundle->mpConn = static_cast(apConn); - bundle->mpVal = GAutoPtr( - g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength() * sizeof(uint8_t), sizeof(uint8_t))); + bundle->mpVal = + g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength() * sizeof(uint8_t), sizeof(uint8_t)); return bundle; } @@ -1434,7 +1429,7 @@ static CHIP_ERROR SendWriteRequestImpl(ConnectionDataBundle * data) g_variant_builder_add(&optionsBuilder, "{sv}", "type", g_variant_new_string("request")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, data->mpVal.get(), options, nullptr, SendWriteRequestDone, + bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, data->mpVal, options, nullptr, SendWriteRequestDone, data->mpConn); exit: diff --git a/src/platform/Linux/bluez/Types.h b/src/platform/Linux/bluez/Types.h index 3130c44c54506e..df1485a5104989 100644 --- a/src/platform/Linux/bluez/Types.h +++ b/src/platform/Linux/bluez/Types.h @@ -188,7 +188,7 @@ struct BluezConnection struct ConnectionDataBundle { BluezConnection * mpConn; - GAutoPtr mpVal; + GVariant * mpVal; }; } // namespace Internal From 3dfd3ad91e5ca4b2c173f4e1e76009762b2c8f95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Fri, 6 Oct 2023 13:02:27 +0200 Subject: [PATCH 11/12] Review update - remove GVariantDict --- src/platform/GLibTypeDeleter.h | 11 ----------- src/platform/Linux/bluez/Helper.cpp | 2 -- 2 files changed, 13 deletions(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index 4918d8b653220e..565403566f60a8 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -65,11 +65,6 @@ struct GVariantDeleter void operator()(GVariant * object) { g_variant_unref(object); } }; -struct GVariantDictDeleter -{ - void operator()(GVariantDict * object) { g_variant_dict_unref(object); } -}; - struct GVariantIterDeleter { void operator()(GVariantIter * object) { g_variant_iter_free(object); } @@ -127,12 +122,6 @@ struct GAutoPtrDeleter using deleter = GVariantDeleter; }; -template <> -struct GAutoPtrDeleter -{ - using deleter = GVariantDictDeleter; -}; - template <> struct GAutoPtrDeleter { diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index b73e3c5770b835..47bfb51452b123 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -376,7 +376,6 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar char * errStr; #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; - GAutoPtr options; GAutoPtr option_mtu; BluezEndpoint * endpoint = static_cast(apEndpoint); @@ -443,7 +442,6 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha char * errStr; #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; - GAutoPtr options; GAutoPtr option_mtu; BluezEndpoint * endpoint = static_cast(apEndpoint); From a276d231f5ba7d597f9f391c982b365a912ece39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Michalak-Szmaci=C5=84ski?= Date: Fri, 6 Oct 2023 14:29:04 +0200 Subject: [PATCH 12/12] Add errorhandling for keyMgmtsHendle --- src/platform/Linux/ConnectivityManagerImpl.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp index 34217cc65555aa..37750fd9ca105b 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.cpp +++ b/src/platform/Linux/ConnectivityManagerImpl.cpp @@ -1560,8 +1560,10 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi GAutoPtr keyMgmts(g_variant_get_strv(keyMgmt.get(), nullptr)); const gchar ** keyMgmtsHendle = keyMgmts.get(); uint8_t res = 0; - for (const gchar * keyMgmtVal = (keyMgmtsHendle != nullptr ? *keyMgmtsHendle : nullptr); keyMgmtVal != nullptr; - keyMgmtVal = *(++keyMgmtsHendle)) + + VerifyOrReturnError(keyMgmtsHendle != nullptr, res); + + for (auto keyMgmtVal = *keyMgmtsHendle; keyMgmtVal != nullptr; keyMgmtVal = *(++keyMgmtsHendle)) { if (g_strcasecmp(keyMgmtVal, "wpa-psk") == 0 || g_strcasecmp(keyMgmtVal, "wpa-none") == 0) { @@ -1588,8 +1590,10 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi GAutoPtr keyMgmts(g_variant_get_strv(keyMgmt.get(), nullptr)); const gchar ** keyMgmtsHendle = keyMgmts.get(); uint8_t res = 0; - for (const gchar * keyMgmtVal = (keyMgmtsHendle != nullptr ? *keyMgmtsHendle : nullptr); keyMgmtVal != nullptr; - keyMgmtVal = *(++keyMgmtsHendle)) + + VerifyOrReturnError(keyMgmtsHendle != nullptr, res); + + for (auto keyMgmtVal = *keyMgmtsHendle; keyMgmtVal != nullptr; keyMgmtVal = *(++keyMgmtsHendle)) { if (g_strcasecmp(keyMgmtVal, "wpa-psk") == 0 || g_strcasecmp(keyMgmtVal, "wpa-psk-sha256") == 0 || g_strcasecmp(keyMgmtVal, "wpa-ft-psk") == 0)