2023-01-05 22:11:14

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 1/4] shared/gatt-client: Use parent debug_callback if not set on clone

From: Luiz Augusto von Dentz <[email protected]>

If clone don't have a dedicated callback set use its parent so users of
bt_gatt_client_clone don't have to keep setting the same callback for
all clone instances.
---
src/shared/gatt-client.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index cb2e64b6cc6b..4aa5d7d92957 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -38,7 +38,8 @@
#define GATT_SVC_UUID 0x1801
#define SVC_CHNGD_UUID 0x2a05
#define DBG(_client, _format, arg...) \
- gatt_log(_client, "%s:%s() " _format, __FILE__, __func__, ## arg)
+ gatt_log(_client, "[%p] %s:%s() " _format, _client, __FILE__, \
+ __func__, ## arg)

struct ready_cb {
bt_gatt_client_callback_t callback;
@@ -357,15 +358,28 @@ static void discovery_op_free(struct discovery_op *op)

static bool read_db_hash(struct discovery_op *op);

+static void gatt_log_va(struct bt_gatt_client *client, const char *format,
+ va_list va)
+{
+ if (!client || !format)
+ return;
+
+ if (client->debug_callback)
+ util_debug_va(client->debug_callback, client->debug_data,
+ format, va);
+ else
+ gatt_log_va(client->parent, format, va);
+}
+
static void gatt_log(struct bt_gatt_client *client, const char *format, ...)
{
va_list ap;

- if (!client || !format || !client->debug_callback)
+ if (!client || !format)
return;

va_start(ap, format);
- util_debug_va(client->debug_callback, client->debug_data, format, ap);
+ gatt_log_va(client, format, ap);
va_end(ap);
}

--
2.37.3


2023-01-05 22:11:14

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 4/4] hog-lib: Fix not handling BT_ATT_OP_HANDLE_NFY_MULT

From: Luiz Augusto von Dentz <[email protected]>

This is a temporary fix for not handling BT_ATT_OP_HANDLE_NFY_MULT in
GAttr so the code will use g_attrib_attach_client to attach the
bt_gatt_client instance which is then used to register notifications
including those sent with BT_ATT_OP_HANDLE_NFY_MULT.

Fixes: https://github.com/bluez/bluez/issues/71
---
profiles/input/hog-lib.c | 23 +++++++++++++++++++++--
src/device.c | 1 +
2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 021db386f3b7..7ff1ede3db35 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -374,6 +374,15 @@ static void report_value_cb(const guint8 *pdu, guint16 len, gpointer user_data)
error("bt_uhid_send: %s (%d)", strerror(-err), -err);
}

+static void report_notify_destroy(void *user_data)
+{
+ struct report *report = user_data;
+
+ DBG("");
+
+ report->notifyid = 0;
+}
+
static void report_ccc_written_cb(guint8 status, const guint8 *pdu,
guint16 plen, gpointer user_data)
{
@@ -393,7 +402,13 @@ static void report_ccc_written_cb(guint8 status, const guint8 *pdu,
report->notifyid = g_attrib_register(hog->attrib,
ATT_OP_HANDLE_NOTIFY,
report->value_handle,
- report_value_cb, report, NULL);
+ report_value_cb, report,
+ report_notify_destroy);
+ if (!report->notifyid) {
+ error("Unable to register report notification: handle 0x%04x",
+ report->value_handle);
+ goto remove;
+ }

DBG("Report characteristic descriptor written: notifications enabled");

@@ -1798,7 +1813,11 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
r->notifyid = g_attrib_register(hog->attrib,
ATT_OP_HANDLE_NOTIFY,
r->value_handle,
- report_value_cb, r, NULL);
+ report_value_cb, r,
+ report_notify_destroy);
+ if (!r->notifyid)
+ error("Unable to register report notification: "
+ "handle 0x%04x", r->value_handle);
}

return true;
diff --git a/src/device.c b/src/device.c
index 995d39f2ccee..28b93eb9ac38 100644
--- a/src/device.c
+++ b/src/device.c
@@ -5152,6 +5152,7 @@ static void gatt_client_init(struct btd_device *device)
}

bt_gatt_client_set_debug(device->client, gatt_debug, NULL, NULL);
+ g_attrib_attach_client(device->attrib, device->client);

/*
* Notify notify existing service about the new connection so they can
--
2.37.3

2023-01-05 22:11:41

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 2/4] shared/gatt-client: Allow registering with NULL callback

From: Luiz Augusto von Dentz <[email protected]>

This makes bt_gatt_client_register_notify allow registering with NULL
callback which is interpreted as the CCC write has already been
performed therefore it won't be written again.
---
src/shared/gatt-client.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 4aa5d7d92957..593b0f27f871 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1711,8 +1711,11 @@ static unsigned int register_notify(struct bt_gatt_client *client,
* descriptor.
*/
chrc = notify_chrc_create(client, handle);
- if (!chrc)
+ if (!chrc) {
+ DBG(client, "Unable to locate characteristic at 0x%04x",
+ handle);
return 0;
+ }
}

/* Fail if we've hit the maximum allowed notify sessions */
@@ -1750,9 +1753,10 @@ static unsigned int register_notify(struct bt_gatt_client *client,
}

/*
- * If the ref count > 1, then notifications are already enabled.
+ * If the ref count > 1, ccc handle cannot be found or registration
+ * callback is not set consider notifications are already enabled.
*/
- if (chrc->notify_count > 1 || !chrc->ccc_handle) {
+ if (chrc->notify_count > 1 || !chrc->ccc_handle || !callback) {
complete_notify_request(notify_data);
return notify_data->id;
}
@@ -2176,6 +2180,9 @@ static void notify_cb(struct bt_att_chan *chan, uint8_t opcode,
struct bt_gatt_client *client = user_data;
struct value_data data;

+ if (queue_isempty(client->notify_list))
+ return;
+
bt_gatt_client_ref(client);

memset(&data, 0, sizeof(data));
@@ -3670,7 +3677,8 @@ unsigned int bt_gatt_client_register_notify(struct bt_gatt_client *client,
void *user_data,
bt_gatt_client_destroy_func_t destroy)
{
- if (!client || !client->db || !chrc_value_handle || !callback)
+ if (!client || !client->db || !chrc_value_handle ||
+ (!callback && !notify))
return 0;

if (client->in_svc_chngd)
--
2.37.3

2023-01-05 22:12:01

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ 3/4] attrib: Introduce g_attrib_attach_client

From: Luiz Augusto von Dentz <[email protected]>

This introduces g_attrib_attach_client which can be used to attach a
bt_gatt_client instance to GAttr so it can be used to register
notifications.
---
Makefile.am | 6 +++---
attrib/gattrib.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
attrib/gattrib.h | 2 ++
3 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index aa3a5e053cd8..0e8cce249c7d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -565,9 +565,9 @@ unit_tests += unit/test-gattrib

unit_test_gattrib_SOURCES = unit/test-gattrib.c attrib/gattrib.c \
$(btio_sources) src/log.h src/log.c
-unit_test_gattrib_LDADD = lib/libbluetooth-internal.la \
- src/libshared-glib.la \
- $(GLIB_LIBS) $(DBUS_LIBS) -ldl -lrt
+unit_test_gattrib_LDADD = src/libshared-glib.la \
+ lib/libbluetooth-internal.la \
+ $(GLIB_LIBS) $(DBUS_LIBS) -ldl -lrt

unit_tests += unit/test-bap

diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 041b9d289c64..997af3699c51 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -21,17 +21,23 @@
#include <glib.h>

#include "lib/bluetooth.h"
+#include "lib/uuid.h"

#include "btio/btio.h"
#include "src/log.h"
#include "src/shared/util.h"
#include "src/shared/att.h"
+#include "src/shared/gatt-helpers.h"
#include "src/shared/queue.h"
+#include "src/shared/gatt-db.h"
+#include "src/shared/gatt-client.h"
+#include "attrib/att.h"
#include "attrib/gattrib.h"

struct _GAttrib {
int ref_count;
struct bt_att *att;
+ struct bt_gatt_client *client;
GIOChannel *io;
GDestroyNotify destroy;
gpointer destroy_user_data;
@@ -145,6 +151,7 @@ void g_attrib_unref(GAttrib *attrib)
if (attrib->destroy)
attrib->destroy(attrib->destroy_user_data);

+ bt_gatt_client_unref(attrib->client);
bt_att_unref(attrib->att);

queue_destroy(attrib->callbacks, attrib_callbacks_destroy);
@@ -338,6 +345,20 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
return TRUE;
}

+static void client_notify_cb(uint16_t value_handle, const uint8_t *value,
+ uint16_t length, void *user_data)
+{
+ uint8_t *buf = newa(uint8_t, length + 2);
+
+ put_le16(value_handle, buf);
+
+ if (length)
+ memcpy(buf + 2, value, length);
+
+ attrib_callback_notify(NULL, ATT_OP_HANDLE_NOTIFY, buf, length + 2,
+ user_data);
+}
+
guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle,
GAttribNotifyFunc func, gpointer user_data,
GDestroyNotify notify)
@@ -359,6 +380,16 @@ guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle,
queue_push_head(attrib->callbacks, cb);
}

+ if (opcode == ATT_OP_HANDLE_NOTIFY && attrib->client) {
+ unsigned int id;
+
+ id = bt_gatt_client_register_notify(attrib->client, handle,
+ NULL, client_notify_cb, cb,
+ attrib_callbacks_remove);
+ if (id)
+ return id;
+ }
+
if (opcode == GATTRIB_ALL_REQS)
opcode = BT_ATT_ALL_REQUESTS;

@@ -410,6 +441,21 @@ gboolean g_attrib_set_mtu(GAttrib *attrib, int mtu)
return bt_att_set_mtu(attrib->att, mtu);
}

+gboolean g_attrib_attach_client(GAttrib *attrib, struct bt_gatt_client *client)
+{
+ if (!attrib || !client)
+ return FALSE;
+
+ if (attrib->client)
+ bt_gatt_client_unref(attrib->client);
+
+ attrib->client = bt_gatt_client_clone(client);
+ if (!attrib->client)
+ return FALSE;
+
+ return TRUE;
+}
+
gboolean g_attrib_unregister(GAttrib *attrib, guint id)
{
if (!attrib)
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index c2877d757342..0111bfc3f2fa 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -19,6 +19,7 @@ extern "C" {
#define GATTRIB_ALL_HANDLES 0x0000

struct bt_att; /* Forward declaration for compatibility */
+struct bt_gatt_client; /* Forward declaration for compatibility */
struct _GAttrib;
typedef struct _GAttrib GAttrib;

@@ -53,6 +54,7 @@ guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle,

uint8_t *g_attrib_get_buffer(GAttrib *attrib, size_t *len);
gboolean g_attrib_set_mtu(GAttrib *attrib, int mtu);
+gboolean g_attrib_attach_client(GAttrib *attrib, struct bt_gatt_client *client);

gboolean g_attrib_unregister(GAttrib *attrib, guint id);
gboolean g_attrib_unregister_all(GAttrib *attrib);
--
2.37.3

2023-01-06 00:17:38

by bluez.test.bot

[permalink] [raw]
Subject: RE: [BlueZ,1/4] shared/gatt-client: Use parent debug_callback if not set on clone

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=709312

---Test result---

Test Summary:
CheckPatch PASS 2.39 seconds
GitLint PASS 1.39 seconds
BuildEll PASS 33.02 seconds
BluezMake PASS 998.52 seconds
MakeCheck PASS 12.62 seconds
MakeDistcheck PASS 178.17 seconds
CheckValgrind PASS 289.84 seconds
CheckSmatch WARNING 383.95 seconds
bluezmakeextell PASS 116.46 seconds
IncrementalBuild PASS 3284.64 seconds
ScanBuild WARNING 1193.87 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/gatt-client.c:2764:33: warning: Variable length array is used.src/shared/gatt-client.c:2994:23: warning: Variable length array is used.src/shared/gatt-client.c:3075:23: warning: Variable length array is used.src/shared/gatt-client.c:3514:23: warning: Variable length array is used.src/shared/gatt-client.c:2764:33: warning: Variable length array is used.src/shared/gatt-client.c:2994:23: warning: Variable length array is used.src/shared/gatt-client.c:3075:23: warning: Variable length array is used.src/shared/gatt-client.c:3514:23: warning: Variable length array is used.src/shared/gatt-client.c:2764:33: warning: Variable length array is used.src/shared/gatt-client.c:2994:23: warning: Variable length array is used.src/shared/gatt-client.c:3075:23: warning: Variable length array is used.src/shared/gatt-client.c:3514:23: warning: Variable length array is used.src/shared/gatt-client.c:2764:33: warning: Variable length array is used.src/shared/gatt-client.c:2994:23: warning: Variab
le length array is used.src/shared/gatt-client.c:3075:23: warning: Variable length array is used.src/shared/gatt-client.c:3514:23: warning: Variable length array is used.src/shared/gatt-client.c:2764:33: warning: Variable length array is used.src/shared/gatt-client.c:2994:23: warning: Variable length array is used.src/shared/gatt-client.c:3075:23: warning: Variable length array is used.src/shared/gatt-client.c:3514:23: warning: Variable length array is used.src/shared/gatt-client.c:2764:33: warning: Variable length array is used.src/shared/gatt-client.c:2994:23: warning: Variable length array is used.src/shared/gatt-client.c:3075:23: warning: Variable length array is used.src/shared/gatt-client.c:3514:23: warning: Variable length array is used.
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
src/shared/gatt-client.c:401:21: warning: Use of memory after it is freed
gatt_db_unregister(op->client->db, op->db_id);
^~~~~~~~~~
src/shared/gatt-client.c:646:2: warning: Use of memory after it is freed
discovery_op_complete(op, false, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:943:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1049:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1241:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1306:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1577:6: warning: Use of memory after it is freed
if (read_db_hash(op)) {
^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1582:2: warning: Use of memory after it is freed
discover_all(op);
^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2088:6: warning: Use of memory after it is freed
if (read_db_hash(op)) {
^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2096:8: warning: Use of memory after it is freed
discovery_op_ref(op),
^~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3182:2: warning: Use of memory after it is freed
complete_write_long_op(req, success, 0, false);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3204:2: warning: Use of memory after it is freed
request_unref(req);
^~~~~~~~~~~~~~~~~~
12 warnings generated.



---
Regards,
Linux Bluetooth

2023-01-06 20:11:21

by patchwork-bot+bluetooth

[permalink] [raw]
Subject: Re: [PATCH BlueZ 1/4] shared/gatt-client: Use parent debug_callback if not set on clone

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:

On Thu, 5 Jan 2023 14:09:41 -0800 you wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> If clone don't have a dedicated callback set use its parent so users of
> bt_gatt_client_clone don't have to keep setting the same callback for
> all clone instances.
> ---
> src/shared/gatt-client.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)

Here is the summary with links:
- [BlueZ,1/4] shared/gatt-client: Use parent debug_callback if not set on clone
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=6e3059ae8cac
- [BlueZ,2/4] shared/gatt-client: Allow registering with NULL callback
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=fde32ff9c9c0
- [BlueZ,3/4] attrib: Introduce g_attrib_attach_client
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=47b6cfeee333
- [BlueZ,4/4] hog-lib: Fix not handling BT_ATT_OP_HANDLE_NFY_MULT
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=2f4c576ad243

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html