Skip to content

Commit 2a7ffdc

Browse files
arkqarkq
authored
[Linux] C++ version of autoptr for glib objects (#28304)
* Remove CHIPOBLE ifdefs in files included by GN * Pass arguments as const if possible * Fix typo in enum name * Get rid of excessive casting * Fix memory leak on new CHIP device scanned * Free memory returned by g_variant_lookup_value * Move glib deleter helpers to lib/support * Define GAutoPtr for glib objects memory management * Build break fix * Fix build break without WPA * Add missing include * Move GLibTypeDeleter.h to src/platform
1 parent 95e3d94 commit 2a7ffdc

18 files changed

+268
-300
lines changed

‎src/controller/python/chip/ble/LinuxImpl.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,11 @@ class ScannerDelegateImpl : public ChipDeviceScannerDelegate
7878

7979
void SetScanner(std::unique_ptr<ChipDeviceScanner> scanner) { mScanner = std::move(scanner); }
8080

81-
void OnDeviceScanned(BluezDevice1 * device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info) override
81+
void OnDeviceScanned(BluezDevice1 & device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info) override
8282
{
8383
if (mScanCallback)
8484
{
85-
mScanCallback(mContext, bluez_device1_get_address(device), info.GetDeviceDiscriminator(), info.GetProductId(),
85+
mScanCallback(mContext, bluez_device1_get_address(&device), info.GetDeviceDiscriminator(), info.GetProductId(),
8686
info.GetVendorId());
8787
}
8888
}

‎src/platform/Linux/GlibTypeDeleter.h ‎src/platform/GLibTypeDeleter.h

+51
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,12 @@
1717

1818
#pragma once
1919

20+
#include <memory>
21+
2022
#include <gio/gio.h>
23+
#include <glib.h>
24+
25+
namespace chip {
2126

2227
template <typename T, typename Deleter>
2328
class UniquePointerReceiver
@@ -69,3 +74,49 @@ struct GBytesDeleter
6974
{
7075
void operator()(GBytes * object) { g_bytes_unref(object); }
7176
};
77+
78+
template <typename T>
79+
struct GAutoPtrDeleter
80+
{
81+
};
82+
83+
template <>
84+
struct GAutoPtrDeleter<char>
85+
{
86+
using deleter = GFree;
87+
};
88+
89+
template <>
90+
struct GAutoPtrDeleter<GBytes>
91+
{
92+
using deleter = GBytesDeleter;
93+
};
94+
95+
template <>
96+
struct GAutoPtrDeleter<GDBusConnection>
97+
{
98+
using deleter = GObjectDeleter;
99+
};
100+
101+
template <>
102+
struct GAutoPtrDeleter<GError>
103+
{
104+
using deleter = GErrorDeleter;
105+
};
106+
107+
template <>
108+
struct GAutoPtrDeleter<GVariant>
109+
{
110+
using deleter = GVariantDeleter;
111+
};
112+
113+
template <>
114+
struct GAutoPtrDeleter<GVariantIter>
115+
{
116+
using deleter = GVariantIterDeleter;
117+
};
118+
119+
template <typename T>
120+
using GAutoPtr = std::unique_ptr<T, typename GAutoPtrDeleter<T>::deleter>;
121+
122+
} // namespace chip

‎src/platform/Linux/BLEManagerImpl.cpp

+17-13
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,26 @@
2121
* Provides an implementation of the BLEManager singleton object
2222
* for Linux platforms.
2323
*/
24-
#include <platform/internal/CHIPDeviceLayerInternal.h>
2524

26-
#include <ble/CHIPBleServiceData.h>
27-
#include <lib/support/CodeUtils.h>
28-
#include <lib/support/SafeInt.h>
29-
#include <new>
30-
#include <platform/CommissionableDataProvider.h>
31-
#include <platform/internal/BLEManager.h>
25+
/**
26+
* Note: BLEManager requires ConnectivityManager to be defined beforehand,
27+
* otherwise we will face circular dependency between them. */
28+
#include <platform/ConnectivityManager.h>
29+
30+
/**
31+
* Note: Use public include for BLEManager which includes our local
32+
* platform/<PLATFORM>/BLEManagerImpl.h after defining interface class. */
33+
#include "platform/internal/BLEManager.h"
3234

3335
#include <cassert>
3436
#include <type_traits>
3537
#include <utility>
3638

37-
#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE
39+
#include <ble/CHIPBleServiceData.h>
40+
#include <lib/support/CodeUtils.h>
41+
#include <lib/support/SafeInt.h>
42+
#include <platform/CHIPDeviceLayer.h>
43+
#include <platform/CommissionableDataProvider.h>
3844

3945
#include "bluez/Helper.h"
4046

@@ -773,9 +779,9 @@ void BLEManagerImpl::NotifyBLEPeripheralAdvStopComplete(bool aIsSuccess, void *
773779
PlatformMgr().PostEventOrDie(&event);
774780
}
775781

776-
void BLEManagerImpl::OnDeviceScanned(BluezDevice1 * device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info)
782+
void BLEManagerImpl::OnDeviceScanned(BluezDevice1 & device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info)
777783
{
778-
ChipLogProgress(Ble, "New device scanned: %s", bluez_device1_get_address(device));
784+
ChipLogProgress(Ble, "New device scanned: %s", bluez_device1_get_address(&device));
779785

780786
if (mBLEScanConfig.mBleScanState == BleScanState::kScanForDiscriminator)
781787
{
@@ -787,7 +793,7 @@ void BLEManagerImpl::OnDeviceScanned(BluezDevice1 * device, const chip::Ble::Chi
787793
}
788794
else if (mBLEScanConfig.mBleScanState == BleScanState::kScanForAddress)
789795
{
790-
if (strcmp(bluez_device1_get_address(device), mBLEScanConfig.mAddress.c_str()) != 0)
796+
if (strcmp(bluez_device1_get_address(&device), mBLEScanConfig.mAddress.c_str()) != 0)
791797
{
792798
return;
793799
}
@@ -837,5 +843,3 @@ void BLEManagerImpl::OnScanError(CHIP_ERROR err)
837843
} // namespace Internal
838844
} // namespace DeviceLayer
839845
} // namespace chip
840-
841-
#endif // CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE

‎src/platform/Linux/BLEManagerImpl.h

+4-8
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@
2626
#include <ble/BleLayer.h>
2727
#include <platform/internal/BLEManager.h>
2828

29-
#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE
30-
3129
#include "bluez/ChipDeviceScanner.h"
3230
#include "bluez/Types.h"
3331

@@ -154,7 +152,7 @@ class BLEManagerImpl final : public BLEManager,
154152
CHIP_ERROR CancelConnection() override;
155153

156154
// ===== Members that implement virtual methods on ChipDeviceScannerDelegate
157-
void OnDeviceScanned(BluezDevice1 * device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info) override;
155+
void OnDeviceScanned(BluezDevice1 & device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info) override;
158156
void OnScanComplete() override;
159157

160158
// ===== Members for internal use by the following friends.
@@ -181,9 +179,9 @@ class BLEManagerImpl final : public BLEManager,
181179

182180
enum
183181
{
184-
kMaxConnections = 1, // TODO: right max connection
185-
kMaxDeviceNameLength = 20, // TODO: right-size this
186-
kMaxAdvertismentDataSetSize = 31 // TODO: verify this
182+
kMaxConnections = 1, // TODO: right max connection
183+
kMaxDeviceNameLength = 20, // TODO: right-size this
184+
kMaxAdvertisementDataSetSize = 31 // TODO: verify this
187185
};
188186

189187
CHIP_ERROR StartBLEAdvertising();
@@ -246,5 +244,3 @@ inline bool BLEManagerImpl::_IsAdvertising()
246244
} // namespace Internal
247245
} // namespace DeviceLayer
248246
} // namespace chip
249-
250-
#endif // CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE

‎src/platform/Linux/BUILD.gn

+5-8
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,8 @@ static_library("Linux") {
4141
sources = [
4242
"../DeviceSafeQueue.cpp",
4343
"../DeviceSafeQueue.h",
44+
"../GLibTypeDeleter.h",
4445
"../SingletonConfigurationManager.cpp",
45-
"BLEManagerImpl.cpp",
46-
"BLEManagerImpl.h",
47-
"BlePlatformConfig.h",
4846
"CHIPDevicePlatformConfig.h",
4947
"CHIPDevicePlatformEvent.h",
5048
"CHIPLinuxStorage.cpp",
@@ -85,6 +83,9 @@ static_library("Linux") {
8583

8684
if (chip_enable_ble) {
8785
sources += [
86+
"BLEManagerImpl.cpp",
87+
"BLEManagerImpl.h",
88+
"BlePlatformConfig.h",
8889
"bluez/AdapterIterator.cpp",
8990
"bluez/AdapterIterator.h",
9091
"bluez/ChipDeviceScanner.cpp",
@@ -125,7 +126,6 @@ static_library("Linux") {
125126

126127
if (chip_enable_openthread) {
127128
sources += [
128-
"GlibTypeDeleter.h",
129129
"ThreadStackManagerImpl.cpp",
130130
"ThreadStackManagerImpl.h",
131131
]
@@ -138,10 +138,7 @@ static_library("Linux") {
138138
}
139139

140140
if (chip_enable_wifi) {
141-
sources += [
142-
"GlibTypeDeleter.h",
143-
"NetworkCommissioningWiFiDriver.cpp",
144-
]
141+
sources += [ "NetworkCommissioningWiFiDriver.cpp" ]
145142

146143
public_deps += [ "dbus/wpa" ]
147144
}

‎src/platform/Linux/ConnectivityManagerImpl.cpp

+36-19
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@
5858
#endif
5959

6060
#if CHIP_DEVICE_CONFIG_ENABLE_WPA
61-
#include <platform/Linux/GlibTypeDeleter.h>
61+
#include <platform/GLibTypeDeleter.h>
6262
#include <platform/internal/GenericConnectivityManagerImpl_WiFi.ipp>
6363
#endif
6464

@@ -76,6 +76,23 @@ using namespace ::chip::app::Clusters::WiFiNetworkDiagnostics;
7676
using namespace ::chip::DeviceLayer::NetworkCommissioning;
7777

7878
namespace chip {
79+
80+
#if CHIP_DEVICE_CONFIG_ENABLE_WPA
81+
82+
template <>
83+
struct GAutoPtrDeleter<WpaFiW1Wpa_supplicant1BSS>
84+
{
85+
using deleter = GObjectDeleter;
86+
};
87+
88+
template <>
89+
struct GAutoPtrDeleter<WpaFiW1Wpa_supplicant1Network>
90+
{
91+
using deleter = GObjectDeleter;
92+
};
93+
94+
#endif // CHIP_DEVICE_CONFIG_ENABLE_WPA
95+
7996
namespace DeviceLayer {
8097

8198
ConnectivityManagerImpl ConnectivityManagerImpl::sInstance;
@@ -1070,8 +1087,7 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent
10701087
void ConnectivityManagerImpl::_ConnectWiFiNetworkAsyncCallback(GObject * source_object, GAsyncResult * res, gpointer user_data)
10711088
{
10721089
ConnectivityManagerImpl * this_ = reinterpret_cast<ConnectivityManagerImpl *>(user_data);
1073-
std::unique_ptr<GVariant, GVariantDeleter> attachRes;
1074-
std::unique_ptr<GError, GErrorDeleter> err;
1090+
GAutoPtr<GError> err;
10751091

10761092
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);
10771093

@@ -1159,7 +1175,7 @@ void ConnectivityManagerImpl::PostNetworkConnect()
11591175
CHIP_ERROR ConnectivityManagerImpl::CommitConfig()
11601176
{
11611177
gboolean result;
1162-
std::unique_ptr<GError, GErrorDeleter> err;
1178+
GAutoPtr<GError> err;
11631179

11641180
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);
11651181

@@ -1290,7 +1306,7 @@ CHIP_ERROR ConnectivityManagerImpl::GetWiFiVersion(WiFiVersionEnum & wiFiVersion
12901306
int32_t ConnectivityManagerImpl::GetDisconnectReason()
12911307
{
12921308
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);
1293-
std::unique_ptr<GError, GErrorDeleter> err;
1309+
GAutoPtr<GError> err;
12941310

12951311
gint errorValue = wpa_fi_w1_wpa_supplicant1_interface_get_disconnect_reason(mWpaSupplicant.iface);
12961312
// wpa_supplicant DBus API: DisconnectReason: The most recent IEEE 802.11 reason code for disconnect. Negative value
@@ -1306,7 +1322,7 @@ CHIP_ERROR ConnectivityManagerImpl::GetConfiguredNetwork(NetworkCommissioning::N
13061322
// with the proxy object.
13071323

13081324
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);
1309-
std::unique_ptr<GError, GErrorDeleter> err;
1325+
GAutoPtr<GError> err;
13101326

13111327
if (mWpaSupplicant.iface == nullptr)
13121328
{
@@ -1322,20 +1338,19 @@ CHIP_ERROR ConnectivityManagerImpl::GetConfiguredNetwork(NetworkCommissioning::N
13221338
return CHIP_ERROR_KEY_NOT_FOUND;
13231339
}
13241340

1325-
std::unique_ptr<WpaFiW1Wpa_supplicant1Network, GObjectDeleter> networkInfo(
1326-
wpa_fi_w1_wpa_supplicant1_network_proxy_new_for_bus_sync(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE,
1327-
kWpaSupplicantServiceName, networkPath, nullptr,
1328-
&MakeUniquePointerReceiver(err).Get()));
1341+
GAutoPtr<WpaFiW1Wpa_supplicant1Network> networkInfo(wpa_fi_w1_wpa_supplicant1_network_proxy_new_for_bus_sync(
1342+
G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, kWpaSupplicantServiceName, networkPath, nullptr,
1343+
&MakeUniquePointerReceiver(err).Get()));
13291344
if (networkInfo == nullptr)
13301345
{
13311346
return CHIP_ERROR_INTERNAL;
13321347
}
13331348

13341349
network.connected = wpa_fi_w1_wpa_supplicant1_network_get_enabled(networkInfo.get());
13351350
GVariant * properties = wpa_fi_w1_wpa_supplicant1_network_get_properties(networkInfo.get());
1336-
GVariant * ssid = g_variant_lookup_value(properties, "ssid", nullptr);
1351+
GAutoPtr<GVariant> ssid(g_variant_lookup_value(properties, "ssid", nullptr));
13371352
gsize length;
1338-
const gchar * ssidStr = g_variant_get_string(ssid, &length);
1353+
const gchar * ssidStr = g_variant_get_string(ssid.get(), &length);
13391354
// TODO: wpa_supplicant will return ssid with quotes! We should have a better way to get the actual ssid in bytes.
13401355
gsize length_actual = length - 2;
13411356
VerifyOrReturnError(length_actual <= sizeof(network.networkID), CHIP_ERROR_INTERNAL);
@@ -1350,7 +1365,7 @@ CHIP_ERROR ConnectivityManagerImpl::StopAutoScan()
13501365
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);
13511366
VerifyOrReturnError(mWpaSupplicant.iface != nullptr, CHIP_ERROR_INCORRECT_STATE);
13521367

1353-
std::unique_ptr<GError, GErrorDeleter> err;
1368+
GAutoPtr<GError> err;
13541369
gboolean result;
13551370

13561371
ChipLogDetail(DeviceLayer, "wpa_supplicant: disabling auto scan");
@@ -1407,6 +1422,7 @@ CHIP_ERROR ConnectivityManagerImpl::StartWiFiScan(ByteSpan ssid, WiFiDriver::Sca
14071422
}
14081423

14091424
namespace {
1425+
14101426
// wpa_supplicant's scan results don't contains the channel infomation, so we need this lookup table for resolving the band and
14111427
// channel infomation.
14121428
std::pair<WiFiBand, uint16_t> GetBandAndChannelFromFrequency(uint32_t freq)
@@ -1505,6 +1521,7 @@ std::pair<WiFiBand, uint16_t> GetBandAndChannelFromFrequency(uint32_t freq)
15051521
}
15061522
return ret;
15071523
}
1524+
15081525
} // namespace
15091526

15101527
bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissioning::WiFiScanResponse & result)
@@ -1514,8 +1531,8 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi
15141531
// completed before this function returns. Also, no external callbacks are registered
15151532
// with the proxy object.
15161533

1517-
std::unique_ptr<GError, GErrorDeleter> err;
1518-
std::unique_ptr<WpaFiW1Wpa_supplicant1BSS, GObjectDeleter> bss(
1534+
GAutoPtr<GError> err;
1535+
GAutoPtr<WpaFiW1Wpa_supplicant1BSS> bss(
15191536
wpa_fi_w1_wpa_supplicant1_bss_proxy_new_for_bus_sync(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, kWpaSupplicantServiceName,
15201537
bssPath, nullptr, &MakeUniquePointerReceiver(err).Get()));
15211538

@@ -1526,8 +1543,8 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi
15261543

15271544
WpaFiW1Wpa_supplicant1BSSProxy * bssProxy = WPA_FI_W1_WPA_SUPPLICANT1_BSS_PROXY(bss.get());
15281545

1529-
std::unique_ptr<GVariant, GVariantDeleter> ssid(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(bssProxy), "SSID"));
1530-
std::unique_ptr<GVariant, GVariantDeleter> bssid(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(bssProxy), "BSSID"));
1546+
GAutoPtr<GVariant> ssid(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(bssProxy), "SSID"));
1547+
GAutoPtr<GVariant> bssid(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(bssProxy), "BSSID"));
15311548

15321549
// Network scan is performed in the background, so the BSS
15331550
// may be gone when we try to get the properties.
@@ -1635,8 +1652,8 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi
16351652
return res;
16361653
};
16371654
auto GetNetworkSecurityType = [IsNetworkWPAPSK, IsNetworkWPA2PSK](WpaFiW1Wpa_supplicant1BSSProxy * proxy) -> uint8_t {
1638-
std::unique_ptr<GVariant, GVariantDeleter> wpa(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(proxy), "WPA"));
1639-
std::unique_ptr<GVariant, GVariantDeleter> rsn(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(proxy), "RSN"));
1655+
GAutoPtr<GVariant> wpa(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(proxy), "WPA"));
1656+
GAutoPtr<GVariant> rsn(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(proxy), "RSN"));
16401657

16411658
uint8_t res = IsNetworkWPAPSK(wpa.get()) | IsNetworkWPA2PSK(rsn.get());
16421659
if (res == 0)

0 commit comments

Comments
 (0)