diff mbox series

bluetoothctl: Add support for discover characteristic by uuid

Message ID 20200605142541.28412-1-amitx.k.singh@intel.com
State New
Headers show
Series bluetoothctl: Add support for discover characteristic by uuid | expand

Commit Message

Singh, AmitX K June 5, 2020, 2:25 p.m. UTC
From: amit <amitx.k.singh@intel.com>

Changes made to add support for discovering gatt characteristic
by uuid.

Signed-off-by: amit <amitx.k.singh@intel.com>
---
 client/gatt.c            |  67 +++++++++++++++++++++
 client/gatt.h            |   1 +
 client/main.c            |  17 ++++++
 src/gatt-client.c        |  69 +++++++++++++++++++++
 src/shared/gatt-client.c | 125 +++++++++++++++++++++++++++++++++++++++
 src/shared/gatt-client.h |   8 +++
 6 files changed, 287 insertions(+)

Comments

Singh, AmitX K July 16, 2020, 8:40 a.m. UTC | #1
Hi Luiz

> -----Original Message-----

> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>

> Sent: Friday, June 5, 2020 10:46 PM

> To: Singh, AmitX K <amitx.k.singh@intel.com>

> Cc: linux-bluetooth@vger.kernel.org

> Subject: Re: [PATCH] bluez:update handle for large database

> 

> Hi Amit,

> 

> On Fri, Jun 5, 2020 at 7:30 AM Amitsi5x <amitx.k.singh@intel.com> wrote:

> >

> > From: amit <amitx.k.singh@intel.com>

> >

> > Update handle for large database and

> > added condition before free to avoid double free

> >

> > Signed-off-by: amit <amitx.k.singh@intel.com>

> > ---

> >  src/shared/gatt-client.c | 12 +++++++-----

> >  src/shared/gatt-db.c     | 15 +++++++++------

> >  2 files changed, 16 insertions(+), 11 deletions(-)

> >

> > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index

> > 19ff6ab65..3cb6ae443 100644

> > --- a/src/shared/gatt-client.c

> > +++ b/src/shared/gatt-client.c

> > @@ -1131,8 +1131,6 @@ static void discover_secondary_cb(bool success,

> uint8_t att_ecode,

> >                                 success = false;

> >                                 goto done;

> >                         }

> > -                       /* Database has changed adjust last handle */

> > -                       op->last = end;

> >                 }

> >

> >                 /* Update pending list */ @@ -1392,9 +1390,13 @@

> > static void db_hash_read_cb(bool success, uint8_t att_ecode,

> >         util_hexdump(' ', value, len, client->debug_callback,

> >                                                 client->debug_data);

> >

> > -       /* Store ithe new hash in the db */

> > -       gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL,

> > -                                       db_hash_write_value_cb, client);

> > +       /* Store the new hash in the db */

> > +       if(gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL,

> > +                                               db_hash_write_value_cb, client)) {

> > +               util_debug(client->debug_callback, client->debug_data,"DB Hash

> match write: skipping discovery");

> > +               queue_remove_all(op->pending_svcs, NULL, NULL, NULL);

> 

> Not following this change, if we got to write the db hash that means the old

> value did not match.

> 


When we verify the PTS test case GATT/CL/GAD/BV-02-C test case, the Test case demands to perform multiple connections to the PTS device with different database upon each connection, where the current code does not update the database hash on each connect iteration that yields to seg fault.
Added support for remove pending service if any after updating new hash in database.

> > +       }

> > +

> >

> >  discover:

> >         if (!op->success) {

> > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index

> > b44f7b5e9..15af4c20a 100644

> > --- a/src/shared/gatt-db.c

> > +++ b/src/shared/gatt-db.c

> > @@ -344,10 +344,15 @@ static bool db_hash_update(void *user_data)

> >         gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);

> >         bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle,

> > db->hash);

> >

> > -       for (i = 0; i < hash.i; i++)

> > -               free(hash.iov[i].iov_base);

> > +       for (i = 0; i < hash.i; i++) {

> > +               if(hash.iov[i].iov_base)

> > +                       free(hash.iov[i].iov_base);

> > +       }

> > +

> > +       if(hash.iov)

> > +               free(hash.iov);

> >

> > -       free(hash.iov);

> > +       hash.iov = NULL;

> 

> I believe this error was actually introduced by your changes actually, see

> below.

> 

> >         return false;

> >  }

> > @@ -689,7 +694,7 @@ struct gatt_db_attribute

> *gatt_db_insert_service(struct gatt_db *db,

> >         service->num_handles = num_handles;

> >

> >         /* Fast-forward next_handle if the new service was added to the end

> */

> > -       db->next_handle = MAX(handle + num_handles, db->next_handle);

> > +       db->next_handle += num_handles;

> 

> Note that if the service was not added to the end this starts adding gaps in

> between, so I'm afraid I will have to nack this change.

> 

> >         return service->attributes[0];

> >

> > @@ -811,8 +816,6 @@ service_insert_characteristic(struct gatt_db_service

> *service,

> >          * declaration. All characteristic definitions shall have a

> >          * Characteristic Value declaration.

> >          */

> > -       if (handle == UINT16_MAX)

> > -               return NULL;

> 

> This perhaps is the real reason, it seems to me that you have more than

> UINT16_MAX handles so the handles loop around and start over from

> 0 which is invalid and will most likely cause double frees etc and they can be

> multiple attributes assigned to the same handle. How big is the database you

> are trying to test? If that is going past UINT16_MAX it is probably broken and

> nothing can be done to fix it on the client side which is why we stop adding

> attributes after it.

> 


When we verify the PTS test case GATT/CL/GAD/BV-02-C test case, the TC demands to perform multiple connections to the PTS device with different database upon each connection. 
In one connection setup database having handle value is UINT16_MAX,  where the current code does not able when handle is max  .

> >         i = get_attribute_index(service, 1);

> >         if (!i)

> > --

> > 2.17.1

> >

> 

> 

> --

> Luiz Augusto von Dentz
diff mbox series

Patch

diff --git a/client/gatt.c b/client/gatt.c
index 21e251d2e..53f875050 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -681,6 +681,73 @@  void gatt_read_attribute(GDBusProxy *proxy, int argc, char *argv[])
 	return bt_shell_noninteractive_quit(EXIT_FAILURE);
 }
 
+static void charbyuuid_reply(DBusMessage *message, void *user_data)
+{
+	DBusError error;
+	DBusMessageIter iter, array;
+	uint8_t *value;
+	int len;
+
+	dbus_error_init(&error);
+
+	if (dbus_set_error_from_message(&error, message) == TRUE) {
+		bt_shell_printf("Failed to read: %s\n", error.name);
+		dbus_error_free(&error);
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
+	dbus_message_iter_init(message, &iter);
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
+		bt_shell_printf("Invalid response to read\n");
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
+	dbus_message_iter_recurse(&iter, &array);
+	dbus_message_iter_get_fixed_array(&array, &value, &len);
+
+	if (len < 0) {
+		bt_shell_printf("Unable to parse value\n");
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
+	return bt_shell_noninteractive_quit(EXIT_SUCCESS);
+}
+
+static void charbyuuid_setup(DBusMessageIter *iter, void *user_data)
+{
+	char *uuid = user_data;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &uuid);
+}
+
+static void charbyuuid_attribute(GDBusProxy *proxy, char *uuid)
+{
+	if (g_dbus_proxy_method_call(proxy, "CharByUUID", charbyuuid_setup, charbyuuid_reply,
+						uuid, NULL) == FALSE) {
+		bt_shell_printf("Failed to set uuid\n");
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
+	bt_shell_printf("Attempting to read service handle %s\n", g_dbus_proxy_get_path(proxy));
+}
+
+void gatt_charbyuuid_attribute(GDBusProxy *proxy, int argc, char *argv[])
+{
+	const char *iface;
+
+	iface = g_dbus_proxy_get_interface(proxy);
+
+	if (!strcmp(iface, "org.bluez.GattCharacteristic1")) {
+		charbyuuid_attribute(proxy, argv[1]);
+		return;
+	}
+
+	bt_shell_printf("Unable to read attribute %s\n",
+						g_dbus_proxy_get_path(proxy));
+
+	return bt_shell_noninteractive_quit(EXIT_FAILURE);
+}
+
 static void servbyuuid_reply(DBusMessage *message, void *user_data)
 {
 	DBusError error;
diff --git a/client/gatt.h b/client/gatt.h
index 8757d6b48..692fb5758 100644
--- a/client/gatt.h
+++ b/client/gatt.h
@@ -34,6 +34,7 @@  void gatt_list_attributes(const char *device);
 GDBusProxy *gatt_select_attribute(GDBusProxy *parent, const char *path);
 char *gatt_attribute_generator(const char *text, int state);
 void gatt_servbyuuid_attribute(GDBusProxy *proxy, int argc, char *argv[]);
+void gatt_charbyuuid_attribute(GDBusProxy *proxy, int argc, char *argv[]);
 void gatt_read_attribute(GDBusProxy *proxy, int argc, char *argv[]);
 void gatt_write_attribute(GDBusProxy *proxy, int argc, char *argv[]);
 void gatt_notify_attribute(GDBusProxy *proxy, bool enable);
diff --git a/client/main.c b/client/main.c
index 79a08728b..10e64e17b 100644
--- a/client/main.c
+++ b/client/main.c
@@ -2071,6 +2071,21 @@  static void cmd_attribute_info(int argc, char *argv[])
 	return bt_shell_noninteractive_quit(EXIT_SUCCESS);
 }
 
+static void cmd_char_by_uuid(int argc, char *argv[])
+{
+	GDBusProxy *proxy;
+
+	proxy = find_attribute(argc, argv);
+	set_default_attribute(proxy);
+
+	if (!default_attr) {
+		bt_shell_printf("No attribute selected\n");
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
+	gatt_charbyuuid_attribute(default_attr, argc, argv);
+}
+
 static void cmd_primary_by_uuid(int argc, char *argv[])
 {
 	GDBusProxy *proxy;
@@ -2701,6 +2716,8 @@  static const struct bt_shell_menu gatt_menu = {
 				"List attributes", dev_generator },
 	{ "primary-by-uuid", "[UUID]", cmd_primary_by_uuid,
 				"Discover Primary Services by UUID" },
+	{ "char-by-uuid", "[UUID]", cmd_char_by_uuid,
+				"Discover Characteristic Services by UUID" },
 	{ "select-attribute", "<attribute/UUID>",  cmd_select_attribute,
 				"Select attribute", attribute_generator },
 	{ "attribute-info", "[attribute/UUID]",  cmd_attribute_info,
diff --git a/src/gatt-client.c b/src/gatt-client.c
index daedae939..da811ea4f 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -444,6 +444,27 @@  static struct async_dbus_op *async_dbus_op_new(DBusMessage *msg, void *data)
 	return op;
 }
 
+static struct async_dbus_op *fetch_char_by_uuid(struct bt_gatt_client *gatt,
+					DBusMessage *msg,
+					char *uuid,
+					bt_gatt_client_char_by_uuid_callback_t callback,
+					void *data)
+{
+	struct async_dbus_op *op;
+
+	op = async_dbus_op_new(msg, data);
+	op->id = bt_gatt_client_char_by_uuid(gatt, uuid, callback,
+						async_dbus_op_ref(op),
+						async_dbus_op_unref);
+
+	if (op->id)
+		return op;
+
+	async_dbus_op_free(op);
+
+	return NULL;
+}
+
 static struct async_dbus_op *fetch_service_by_uuid(struct bt_gatt_client *gatt,
 					DBusMessage *msg,
 					char *uuid,
@@ -951,6 +972,51 @@  fail:
 	chrc->read_op = NULL;
 }
 
+static void characteristic_by_uuid_cb(bool success, uint8_t att_ecode, const uint8_t *value,
+					uint16_t length, void *user_data)
+{
+	struct async_dbus_op *op = user_data;
+	struct characteristic *opchar = op->data;
+
+	if (!success)
+		goto fail;
+
+	async_dbus_op_reply(op, att_ecode, value, length);
+
+	return;
+
+fail:
+	async_dbus_op_reply(op, att_ecode, NULL, 0);
+	opchar->type_op = NULL;
+}
+
+static DBusMessage *chardiscover_by_uuid(DBusConnection *conn,
+					DBusMessage *msg, void *user_data)
+{
+	struct characteristic *chardata = user_data;
+	struct bt_gatt_client *gatt = chardata->service->client->gatt;
+	DBusMessageIter iter;
+
+	char *uuid = 0;
+
+	if (!gatt)
+		return btd_error_failed(msg, "Not connected");
+
+	dbus_message_iter_init(msg, &iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_STRING)
+		dbus_message_iter_get_basic(&iter,&uuid);
+	else
+		return NULL;
+
+	chardata->type_op = fetch_char_by_uuid(gatt, msg,uuid, characteristic_by_uuid_cb, chardata);
+
+	if (!chardata->type_op)
+		return btd_error_failed(msg, "Failed to send read request");
+
+	return NULL;
+}
+
 static void serv_uuid_cb(bool success, uint8_t att_ecode, const uint8_t *value,
 					uint16_t length, void *user_data)
 {
@@ -1717,6 +1783,9 @@  static const GDBusPropertyTable characteristic_properties[] = {
 };
 
 static const GDBusMethodTable characteristic_methods[] = {
+	{ GDBUS_ASYNC_METHOD("CharByUUID", GDBUS_ARGS({ "options", "s" }),
+					GDBUS_ARGS({ "value", "ay" }),
+					chardiscover_by_uuid) },
 	{ GDBUS_ASYNC_METHOD("ReadValue", GDBUS_ARGS({ "options", "a{sv}" }),
 					GDBUS_ARGS({ "value", "ay" }),
 					characteristic_read_value) },
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 88257c054..8a696c77f 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -131,6 +131,13 @@  struct request {
 	void (*destroy)(void *);
 };
 
+struct char_by_uuid_op
+{
+	bt_gatt_client_char_by_uuid_callback_t callback;
+	bt_gatt_client_destroy_func_t destroy;
+	void *user_data;
+};
+
 struct service_by_uuid_op
 {
 	bt_gatt_client_service_by_uuid_callback_t callback;
@@ -2585,6 +2592,16 @@  bool bt_gatt_client_cancel_all(struct bt_gatt_client *client)
 	return true;
 }
 
+static void destroy_char_by_uuid_op(void *data)
+{
+	struct char_by_uuid_op *op = data;
+
+	if (op->destroy)
+		op->destroy(op->user_data);
+
+	free(op);
+}
+
 static void destroy_service_by_uuid_op(void *data)
 {
 	struct service_by_uuid_op *op = data;
@@ -2595,6 +2612,39 @@  static void destroy_service_by_uuid_op(void *data)
 	free(op);
 }
 
+static void char_by_uuid_cb(uint8_t opcode, const void *pdu, uint16_t length,
+								void *user_data)
+{
+	struct request *req = user_data;
+	struct char_by_uuid_op *op = req->data;
+	bool success;
+
+	uint8_t att_ecode = 0;
+	const uint8_t *value = NULL;
+	uint16_t value_len = 0;
+
+	if (opcode == BT_ATT_OP_ERROR_RSP) {
+		success = false;
+		att_ecode = process_error(pdu, length);
+		goto done;
+	}
+
+	if (opcode != BT_ATT_OP_READ_BY_TYPE_RSP || (!pdu && length)) {
+		success = false;
+		goto done;
+	}
+
+	success = true;
+	value_len = length;
+
+	if (value_len)
+		value = pdu;
+
+done:
+	if (op->callback)
+		op->callback(success, att_ecode, value, length, op->user_data);
+}
+
 static void service_by_uuid_cb(uint8_t opcode, const void *pdu, uint16_t length,
 								void *user_data)
 {
@@ -2675,6 +2725,81 @@  done:
 		op->callback(success, att_ecode, value, length, op->user_data);
 }
 
+unsigned int bt_gatt_client_char_by_uuid(struct bt_gatt_client *client,
+                                              char *uuid,
+                                              bt_gatt_client_char_by_uuid_callback_t callback,
+					      void *user_data,
+                                              bt_gatt_client_destroy_func_t destroy)
+{
+	struct request *req;
+	struct char_by_uuid_op *op;
+	unsigned char *pdu;
+	uint16_t len ;
+	uint16_t start_handle = 0x0001;
+	uint16_t end_handle = 0xffff;
+	bt_uuid_t btuuid;
+	uint8_t uuid128[16];
+
+	/* Length of pdu will be vary according to uuid type
+	for 2 byte uuid total length  is 8 (start handle(2) + end handle(2)  + uuid(2))
+	for 16 byte uuid total length  is 22 (start handle(2) + end handle(2)  + uuid(16))
+	*/
+	uint16_t pdu_len_16bit_uuid = 6;
+	uint16_t pdu_len_128bit_uuid = 20;
+
+	if (bt_string_to_uuid(&btuuid, uuid) < 0) {
+		return 0;
+	}
+
+	if (btuuid.type == BT_UUID16){
+		pdu = (unsigned char *) malloc(pdu_len_16bit_uuid);
+		len = pdu_len_16bit_uuid;
+	} else {
+		pdu = (unsigned char *) malloc(pdu_len_128bit_uuid);
+		len = pdu_len_128bit_uuid;
+	}
+
+	if (!client)
+		return 0;
+
+	op = new0(struct char_by_uuid_op, 1);
+	req = request_create(client);
+
+	if (!req) {
+		free(op);
+		return 0;
+	}
+
+	op->callback = callback;
+	op->user_data = user_data;
+	op->destroy = destroy;
+	req->data = op;
+	req->destroy = destroy_char_by_uuid_op;
+
+	put_le16(start_handle, pdu);
+	put_le16(end_handle, pdu+2);
+
+	if (btuuid.type == BT_UUID16)
+		put_le16(btuuid.value.u16, pdu+4);
+	else {
+		for (int i =0 ; i<16 ; i++)
+			uuid128[15-i]=btuuid.value.u128.data[i];
+		memcpy(pdu + 4, uuid128, 16);
+	}
+
+	req->att_id = bt_att_send(client->att, BT_ATT_OP_READ_BY_TYPE_REQ,
+							pdu, len,
+							char_by_uuid_cb, req,
+							request_unref);
+	if (!req->att_id) {
+		op->destroy = NULL;
+		request_unref(req);
+		return 0;
+	}
+
+	return req->id;
+}
+
 unsigned int bt_gatt_client_service_by_uuid(struct bt_gatt_client *client,
 						char *uuid,
 						bt_gatt_client_service_by_uuid_callback_t callback,
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 599e98556..f5d5169ce 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -45,6 +45,9 @@  typedef void (*bt_gatt_client_debug_func_t)(const char *str, void *user_data);
 typedef void (*bt_gatt_client_service_by_uuid_callback_t)(bool success, uint8_t att_ecode,
 					const uint8_t *value, uint16_t length,
 					void *user_data);
+typedef void (*bt_gatt_client_char_by_uuid_callback_t)(bool success, uint8_t att_ecode,
+					const uint8_t *value, uint16_t length,
+					void *user_data);
 typedef void (*bt_gatt_client_read_callback_t)(bool success, uint8_t att_ecode,
 					const uint8_t *value, uint16_t length,
 					void *user_data);
@@ -89,6 +92,11 @@  unsigned int bt_gatt_client_service_by_uuid(struct bt_gatt_client *client,
 					bt_gatt_client_read_callback_t callback,
 					void *user_data,
 					bt_gatt_client_destroy_func_t destroy);
+unsigned int bt_gatt_client_char_by_uuid(struct bt_gatt_client *client,
+					char *uuid,
+					bt_gatt_client_read_callback_t callback,
+					void *user_data,
+					bt_gatt_client_destroy_func_t destroy);
 unsigned int bt_gatt_client_read_value(struct bt_gatt_client *client,
 					uint16_t value_handle,
 					bt_gatt_client_read_callback_t callback,