Dear all,
in a previous email I reported leaking resources in qt-ble.c. Here is my
attempt at a fix. I reckon this ideally has no user-visible effect (i.e. is
gratuitous), yet is risky. Therefore some prudence is in order.
There are three patches with a decreasing degree of obviousness.
1) Delete created Service objects in destructor of BLEObject.
2) Let BLEObject take ownership of the Connection object so that the latter
can be deleted in the destructor of the former. Delete BLEObject in error
cases.
3) Optional: Move creation of the Connection object to the BLEObject to avoid
ownership subtleties.
Berthold
>From 93a12815e293952f25d7ec2cad7778804ae9ed6b Mon Sep 17 00:00:00 2001
From: Berthold Stoeger <bstoe...@mail.tuwien.ac.at>
Date: Tue, 24 Oct 2017 17:42:07 +0200
Subject: [PATCH 1/3] Fix resource leak in qt-ble.cpp
Destroy QLowEnergyService objects in destructor of BLEObject.
Signed-off-by: Berthold Stoeger <bstoe...@mail.tuwien.ac.at>
---
core/qt-ble.cpp | 3 +++
1 file changed, 3 insertions(+)
diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp
index a72736d4..de8771b1 100644
--- a/core/qt-ble.cpp
+++ b/core/qt-ble.cpp
@@ -140,6 +140,9 @@ BLEObject::BLEObject(QLowEnergyController *c, dc_user_device_t *d)
BLEObject::~BLEObject()
{
qDebug() << "Deleting BLE object";
+
+ foreach (QLowEnergyService *service, services)
+ delete service;
}
dc_status_t BLEObject::write(const void *data, size_t size, size_t *actual)
--
2.14.1
>From 2c77111270053580571e4a0ec501edfe7ba39083 Mon Sep 17 00:00:00 2001
From: Berthold Stoeger <bstoe...@mail.tuwien.ac.at>
Date: Tue, 24 Oct 2017 18:06:23 +0200
Subject: [PATCH 2/3] Fix resource leak in qt-ble.cpp
If qt_ble_open() succeeded, the controller was not deleted. The solution
here is to let the BLE object take ownership of controller and destroy
controller in its destructor.
Moreover, this patch fixes another resource leak, where the BLE object
would not be destroyed for two error conditions.
Signed-off-by: Berthold Stoeger <bstoe...@mail.tuwien.ac.at>
---
core/qt-ble.cpp | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp
index de8771b1..678296b2 100644
--- a/core/qt-ble.cpp
+++ b/core/qt-ble.cpp
@@ -143,6 +143,8 @@ BLEObject::~BLEObject()
foreach (QLowEnergyService *service, services)
delete service;
+
+ delete controller;
}
dc_status_t BLEObject::write(const void *data, size_t size, size_t *actual)
@@ -332,7 +334,9 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
return DC_STATUS_IO;
}
- /* We need to discover services etc here! */
+ // We need to discover services etc here!
+ // Note that ble takes ownership of controller and henceforth deleting ble will
+ // take care of deleting controller.
BLEObject *ble = new BLEObject(controller, io->user_device);
ble->connect(controller, SIGNAL(serviceDiscovered(QBluetoothUuid)), SLOT(addService(QBluetoothUuid)));
@@ -351,7 +355,7 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
qDebug() << "failed to find suitable service on" << devaddr;
report_error("Failed to find suitable service on '%s'", devaddr);
controller->disconnectFromDevice();
- delete controller;
+ delete ble;
return DC_STATUS_IO;
}
@@ -366,7 +370,7 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
qDebug() << "failed to find suitable service on" << devaddr;
report_error("Failed to find suitable service on '%s'", devaddr);
controller->disconnectFromDevice();
- delete controller;
+ delete ble;
return DC_STATUS_IO;
}
@@ -381,8 +385,10 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
if (IS_HW(io->user_device)) {
dc_status_t r = ble->setupHwTerminalIo(list);
- if (r != DC_STATUS_SUCCESS)
+ if (r != DC_STATUS_SUCCESS) {
+ delete ble;
return r;
+ }
} else {
QList<QLowEnergyDescriptor> l = c.descriptors();
--
2.14.1
>From 857e930c5664142fa7ed22cfda93b66ee50a674f Mon Sep 17 00:00:00 2001
From: Berthold Stoeger <bstoe...@mail.tuwien.ac.at>
Date: Tue, 24 Oct 2017 19:00:35 +0200
Subject: [PATCH 3/3] Move creation of BLE controller into BLEObject
Instead of taking ownership of the BLE controller, generate the controller
in BLEObject. Thus avoid ownership subtleties.
Signed-off-by: Berthold Stoeger <bstoe...@mail.tuwien.ac.at>
---
core/qt-ble.cpp | 162 ++++++++++++++++++++++++++++++--------------------------
core/qt-ble.h | 3 +-
2 files changed, 88 insertions(+), 77 deletions(-)
diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp
index 678296b2..93fac629 100644
--- a/core/qt-ble.cpp
+++ b/core/qt-ble.cpp
@@ -130,11 +130,11 @@ void BLEObject::addService(const QBluetoothUuid &newService)
}
}
-BLEObject::BLEObject(QLowEnergyController *c, dc_user_device_t *d)
+BLEObject::BLEObject(dc_user_device_t *d)
{
- controller = c;
device = d;
debugCounter = 0;
+ controller = nullptr;
}
BLEObject::~BLEObject()
@@ -144,7 +144,86 @@ BLEObject::~BLEObject()
foreach (QLowEnergyService *service, services)
delete service;
- delete controller;
+ if(controller)
+ delete controller;
+}
+
+#include <stdio.h>
+dc_status_t BLEObject::connect_and_discover_services(const char *devaddr)
+{
+#if defined(Q_OS_MACOS) || defined(Q_OS_IOS)
+ QBluetoothDeviceInfo remoteDevice = getBtDeviceInfo(devaddr);
+ controller = QLowEnergyController::createCentral(remoteDevice);
+#else
+ // this is deprecated but given that we don't use Qt to scan for
+ // devices on Android, we don't have QBluetoothDeviceInfo for the
+ // paired devices and therefore cannot use the newer interfaces
+ // that are preferred starting with Qt 5.7
+ QBluetoothAddress remoteDeviceAddress(devaddr);
+ controller = new QLowEnergyController(remoteDeviceAddress);
+#endif
+
+ if (IS_SHEARWATER(device))
+ controller->setRemoteAddressType(QLowEnergyController::RandomAddress);
+
+ // Try to connect to the device
+ controller->connectToDevice();
+
+ // Create a timer. If the connection doesn't succeed after five seconds or no error occurs then stop the opening step
+ int msec = BLE_TIMEOUT;
+ while (msec > 0 && controller->state() == QLowEnergyController::ConnectingState) {
+ waitFor(100);
+ msec -= 100;
+ };
+
+ switch (controller->state()) {
+ case QLowEnergyController::ConnectedState:
+ qDebug() << "connected to the controller for device" << devaddr;
+ break;
+ default:
+ qDebug() << "failed to connect to the controller " << devaddr << "with error" << controller->errorString();
+ report_error("Failed to connect to %s: '%s'", devaddr, controller->errorString().toUtf8().data());
+ controller->disconnectFromDevice();
+ return DC_STATUS_IO;
+ }
+
+ // Receive change notifications, etc.
+ connect(controller, SIGNAL(serviceDiscovered(QBluetoothUuid)), SLOT(addService(QBluetoothUuid)));
+
+ // We need to discover services etc here!
+ qDebug() << " .. discovering services";
+
+ controller->discoverServices();
+
+ msec = BLE_TIMEOUT;
+ while (msec > 0 && controller->state() == QLowEnergyController::DiscoveringState) {
+ waitFor(100);
+ msec -= 100;
+ };
+
+ qDebug() << " .. done discovering services";
+ if (services.isEmpty()) {
+ qDebug() << "failed to find suitable service on" << devaddr;
+ report_error("Failed to find suitable service on '%s'", devaddr);
+ controller->disconnectFromDevice();
+ return DC_STATUS_IO;
+ }
+
+ qDebug() << " .. discovering details";
+ msec = BLE_TIMEOUT;
+ while (msec > 0 && preferredService()->state() == QLowEnergyService::DiscoveringServices) {
+ waitFor(100);
+ msec -= 100;
+ };
+
+ if (preferredService()->state() != QLowEnergyService::ServiceDiscovered) {
+ qDebug() << "failed to find suitable service on" << devaddr;
+ report_error("Failed to find suitable service on '%s'", devaddr);
+ controller->disconnectFromDevice();
+ return DC_STATUS_IO;
+ }
+
+ return DC_STATUS_SUCCESS;
}
dc_status_t BLEObject::write(const void *data, size_t size, size_t *actual)
@@ -296,85 +375,16 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
// HACK ALERT! Qt 5.9 needs this for proper Bluez operation
qputenv("QT_DEFAULT_CENTRAL_SERVICES", "1");
-#if defined(Q_OS_MACOS) || defined(Q_OS_IOS)
- QBluetoothDeviceInfo remoteDevice = getBtDeviceInfo(devaddr);
- QLowEnergyController *controller = QLowEnergyController::createCentral(remoteDevice);
-#else
- // this is deprecated but given that we don't use Qt to scan for
- // devices on Android, we don't have QBluetoothDeviceInfo for the
- // paired devices and therefore cannot use the newer interfaces
- // that are preferred starting with Qt 5.7
- QBluetoothAddress remoteDeviceAddress(devaddr);
- QLowEnergyController *controller = new QLowEnergyController(remoteDeviceAddress);
-#endif
qDebug() << "qt_ble_open(" << devaddr << ")";
- if (IS_SHEARWATER(io->user_device))
- controller->setRemoteAddressType(QLowEnergyController::RandomAddress);
-
- // Try to connect to the device
- controller->connectToDevice();
-
- // Create a timer. If the connection doesn't succeed after five seconds or no error occurs then stop the opening step
- int msec = BLE_TIMEOUT;
- while (msec > 0 && controller->state() == QLowEnergyController::ConnectingState) {
- waitFor(100);
- msec -= 100;
- };
-
- switch (controller->state()) {
- case QLowEnergyController::ConnectedState:
- qDebug() << "connected to the controller for device" << devaddr;
- break;
- default:
- qDebug() << "failed to connect to the controller " << devaddr << "with error" << controller->errorString();
- report_error("Failed to connect to %s: '%s'", devaddr, controller->errorString().toUtf8().data());
- controller->disconnectFromDevice();
- delete controller;
- return DC_STATUS_IO;
- }
-
- // We need to discover services etc here!
- // Note that ble takes ownership of controller and henceforth deleting ble will
- // take care of deleting controller.
- BLEObject *ble = new BLEObject(controller, io->user_device);
- ble->connect(controller, SIGNAL(serviceDiscovered(QBluetoothUuid)), SLOT(addService(QBluetoothUuid)));
+ BLEObject *ble = new BLEObject(io->user_device);
- qDebug() << " .. discovering services";
-
- controller->discoverServices();
-
- msec = BLE_TIMEOUT;
- while (msec > 0 && controller->state() == QLowEnergyController::DiscoveringState) {
- waitFor(100);
- msec -= 100;
- };
-
- qDebug() << " .. done discovering services";
- if (ble->preferredService() == nullptr) {
- qDebug() << "failed to find suitable service on" << devaddr;
- report_error("Failed to find suitable service on '%s'", devaddr);
- controller->disconnectFromDevice();
- delete ble;
- return DC_STATUS_IO;
- }
-
- qDebug() << " .. discovering details";
- msec = BLE_TIMEOUT;
- while (msec > 0 && ble->preferredService()->state() == QLowEnergyService::DiscoveringServices) {
- waitFor(100);
- msec -= 100;
- };
-
- if (ble->preferredService()->state() != QLowEnergyService::ServiceDiscovered) {
- qDebug() << "failed to find suitable service on" << devaddr;
- report_error("Failed to find suitable service on '%s'", devaddr);
- controller->disconnectFromDevice();
+ dc_status_t r = ble->connect_and_discover_services(devaddr);
+ if (r != DC_STATUS_SUCCESS) {
delete ble;
- return DC_STATUS_IO;
+ return r;
}
-
qDebug() << " .. enabling notifications";
/* Enable notifications */
diff --git a/core/qt-ble.h b/core/qt-ble.h
index f366afe8..6aaa6416 100644
--- a/core/qt-ble.h
+++ b/core/qt-ble.h
@@ -18,8 +18,9 @@ class BLEObject : public QObject
Q_OBJECT
public:
- BLEObject(QLowEnergyController *c, dc_user_device_t *);
+ BLEObject(dc_user_device_t *);
~BLEObject();
+ dc_status_t connect_and_discover_services(const char *devaddr);
dc_status_t write(const void* data, size_t size, size_t *actual);
dc_status_t read(void* data, size_t size, size_t *actual);
--
2.14.1
_______________________________________________
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
