[PATCH v2 1/2] android/bluetooth: Add support for get remote service record property cmd

[PATCH v2 1/2] android/bluetooth: Add support for get remote service record property cmd

[Grzegorz Kolodziejczyk]

This allows to get service record property by uuid of specified remote device.
---
 android/bluetooth.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 130 insertions(+), 3 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index 699e880..51c31d1 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -38,6 +38,7 @@
 #include "lib/bluetooth.h"
 #include "lib/sdp.h"
 #include "lib/mgmt.h"
+#include "lib/uuid.h"
 #include "src/shared/util.h"
 #include "src/shared/mgmt.h"
 #include "src/eir.h"
@@ -1067,6 +1068,128 @@ static uint8_t browse_remote_sdp(const bdaddr_t *addr)
 	return HAL_STATUS_SUCCESS;
 }
 
+static void send_remote_sdp_rec_notify(bt_uuid_t *uuid, int channel,
+					char *name, uint8_t name_len,
+					uint8_t status, bdaddr_t *bdaddr)
+{
+	struct hal_prop_device_service_rec *prop;
+	uint8_t buf[BASELEN_REMOTE_DEV_PROP + name_len + sizeof(*prop)];
+	struct hal_ev_remote_device_props *ev = (void *) buf;
+	int prop_len = sizeof(*prop) + name_len;
+
+	memset(buf, 0, sizeof(buf));
+
+	if (uuid && status == HAL_STATUS_SUCCESS) {
+		prop = (void *) &ev->props[0].val;
+		prop->name_len = name_len;
+		prop->channel = (uint16_t)channel;
+		memcpy(prop->name, name, name_len);
+		memcpy(prop->uuid, &uuid->value.u128, sizeof(prop->uuid));
+	}
+
+	ev->num_props = 1;
+	ev->status = status;
+	ev->props[0].len = prop_len;
+	bdaddr2android(bdaddr, ev->bdaddr);
+	ev->props[0].type = HAL_PROP_DEVICE_SERVICE_REC;
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_BLUETOOTH,
+						HAL_EV_REMOTE_DEVICE_PROPS,
+						sizeof(buf), buf);
+}
+
+static void find_remote_sdp_rec_cb(sdp_list_t *recs, int err,
+							gpointer user_data)
+{
+	uint8_t name_len = 0;
+	uint8_t status;
+	char name_buf[256];
+	int channel = 0;
+	bdaddr_t *addr = user_data;
+	bt_uuid_t uuid;
+	uuid_t uuid128_sdp;
+	sdp_list_t *protos;
+	sdp_record_t *sdp_rec;
+
+	if (err < 0) {
+		error("error while search remote sdp records");
+		status = HAL_STATUS_FAILED;
+		send_remote_sdp_rec_notify(NULL, 0, NULL, 0, status, addr);
+		goto done;
+	}
+
+	if (!recs) {
+		info("No service records found on remote");
+		status = HAL_STATUS_SUCCESS;
+		send_remote_sdp_rec_notify(NULL, 0, NULL, 0, status, addr);
+		goto done;
+	}
+
+	for ( ; recs; recs = recs->next) {
+		sdp_rec = recs->data;
+
+		switch (sdp_rec->svclass.type) {
+		case SDP_UUID16:
+			sdp_uuid16_to_uuid128(&uuid128_sdp,
+							&sdp_rec->svclass);
+			break;
+		case SDP_UUID32:
+			sdp_uuid32_to_uuid128(&uuid128_sdp,
+							&sdp_rec->svclass);
+			break;
+		case SDP_UUID128:
+			break;
+		default:
+			error("wrong sdp uuid type");
+			goto done;
+		}
+
+		if (!sdp_get_access_protos(sdp_rec, &protos)) {
+			channel = sdp_get_proto_port(protos, L2CAP_UUID);
+			if (channel < 0)
+				error("wrong channel");
+		}
+
+		if (!sdp_get_service_name(sdp_rec, name_buf, sizeof(name_buf)))
+			name_len = (uint8_t)strlen(name_buf);
+
+		uuid.type = BT_UUID128;
+		memcpy(&uuid.value.u128, uuid128_sdp.value.uuid128.data,
+						sizeof(uuid.value.u128));
+		status = HAL_STATUS_SUCCESS;
+
+		send_remote_sdp_rec_notify(&uuid, channel, name_buf, name_len,
+								status, addr);
+	}
+
+done:
+	free(addr);
+}
+
+static uint8_t find_remote_sdp_rec(const bdaddr_t *addr,
+						const uint8_t *find_uuid)
+{
+	uuid_t uuid;
+	bdaddr_t *bdaddr;
+
+	/* from android we always get full 128bit length uuid */
+	sdp_uuid128_create(&uuid, find_uuid);
+
+	bdaddr = malloc(sizeof(*bdaddr));
+	if (!bdaddr)
+		return HAL_STATUS_NOMEM;
+
+	memcpy(bdaddr, addr, sizeof(*bdaddr));
+
+	if (bt_search_service(&adapter.bdaddr, addr, &uuid,
+				find_remote_sdp_rec_cb, bdaddr, NULL, 0) < 0) {
+		free(bdaddr);
+		return HAL_STATUS_FAILED;
+	}
+
+	return HAL_STATUS_SUCCESS;
+}
+
 static void new_link_key_callback(uint16_t index, uint16_t length,
 					const void *param, void *user_data)
 {
@@ -4746,12 +4869,16 @@ failed:
 
 static void handle_get_remote_service_rec_cmd(const void *buf, uint16_t len)
 {
-	/* TODO */
+	const struct hal_cmd_get_remote_service_rec *cmd = buf;
+	uint8_t status;
+	bdaddr_t addr;
+
+	android2bdaddr(&cmd->bdaddr, &addr);
 
-	error("get_remote_service_record not supported");
+	status = find_remote_sdp_rec(&addr, cmd->uuid);
 
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_BLUETOOTH,
-			HAL_OP_GET_REMOTE_SERVICE_REC, HAL_STATUS_FAILED);
+					HAL_OP_GET_REMOTE_SERVICE_REC, status);
 }
 
 static void handle_start_discovery_cmd(const void *buf, uint16_t len)
-- 
1.9.3

[PATCH v2 2/2] android/pts: Update IOPT test results

[Grzegorz Kolodziejczyk]

---
 android/pts-iopt.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/android/pts-iopt.txt b/android/pts-iopt.txt
index 20962af..3065836 100644
--- a/android/pts-iopt.txt
+++ b/android/pts-iopt.txt
@@ -19,5 +19,7 @@ TC_COD_BV_02_I	N/A	Under PTS 5.1 test shall be disabled as there is
 			PICS settings for HFP shall be disabled for IOPT
 TC_SDSS_BV_02_I	PASS
 TC_SDSA_BV_03_I	FAIL	JIRA #BA-92
-TC_SDR_BV_04_I	FAIL	JIRA #BA-95
+TC_SDR_BV_04_I	PASS	for every PTS bt profile:
+			haltest: bluetooth get_remote_service_record <addr>
+							<requred 128 sdp uuid>
 -------------------------------------------------------------------------------
-- 
1.9.3

Re: [PATCH v2 1/2] android/bluetooth: Add support for get remote service record property cmd

[Johan Hedberg]

Hi Grzegorz,

On Thu, Aug 14, 2014, Grzegorz Kolodziejczyk wrote:
> +		prop->channel = (uint16_t)channel;

This typecast seems unnecessary to me. At least I don't get any compiler
warnings/errors if I remove it.

> +static void find_remote_sdp_rec_cb(sdp_list_t *recs, int err,
> +							gpointer user_data)
> +{
> +	uint8_t name_len = 0;
> +	uint8_t status;
> +	char name_buf[256];
> +	int channel = 0;

Since both name_len and channel are tied to the specific record that
you're iterating it seems wrong to me to initialize these here.

> +	for ( ; recs; recs = recs->next) {
> +		sdp_rec = recs->data;
> +
> +		switch (sdp_rec->svclass.type) {
> +		case SDP_UUID16:
> +			sdp_uuid16_to_uuid128(&uuid128_sdp,
> +							&sdp_rec->svclass);
> +			break;
> +		case SDP_UUID32:
> +			sdp_uuid32_to_uuid128(&uuid128_sdp,
> +							&sdp_rec->svclass);
> +			break;
> +		case SDP_UUID128:
> +			break;
> +		default:
> +			error("wrong sdp uuid type");
> +			goto done;
> +		}
> +
> +		if (!sdp_get_access_protos(sdp_rec, &protos)) {
> +			channel = sdp_get_proto_port(protos, L2CAP_UUID);
> +			if (channel < 0)
> +				error("wrong channel");
> +		}

Probably this should be here:

		} else {
			channel = -1;
		}

> +		if (!sdp_get_service_name(sdp_rec, name_buf, sizeof(name_buf)))
> +			name_len = (uint8_t)strlen(name_buf);

And here:

		else
			name_len = 0;

Otherwise you'll end up including the name from previous records as the
name of subsequent ones which don't have a service name attribute. Also,
the (uint8_t) typecast seems unnecessary here (compiles fine for me
without it).

> +static uint8_t find_remote_sdp_rec(const bdaddr_t *addr,
> +						const uint8_t *find_uuid)
> +{
> +	uuid_t uuid;
> +	bdaddr_t *bdaddr;
> +
> +	/* from android we always get full 128bit length uuid */
> +	sdp_uuid128_create(&uuid, find_uuid);
> +
> +	bdaddr = malloc(sizeof(*bdaddr));

Could you tell me what's the rationale of choosing malloc vs g_malloc in
this code. In most places this c-file is using the GLib allocators and
having a mixed set is just begging for trouble (e.g. you might end up
calling g_free on something allocated with malloc, or free for something
allocated with g_malloc).

Johan

Re: [PATCH v2 1/2] android/bluetooth: Add support for get remote service record property cmd

[Grzegorz Kolodziejczyk]

Hi Johan,

On 14 August 2014 14:28, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Grzegorz,
>
> On Thu, Aug 14, 2014, Grzegorz Kolodziejczyk wrote:
>> +             prop->channel = (uint16_t)channel;
>
> This typecast seems unnecessary to me. At least I don't get any compiler
> warnings/errors if I remove it.
>
>> +static void find_remote_sdp_rec_cb(sdp_list_t *recs, int err,
>> +                                                     gpointer user_data)
>> +{
>> +     uint8_t name_len = 0;
>> +     uint8_t status;
>> +     char name_buf[256];
>> +     int channel = 0;
>
> Since both name_len and channel are tied to the specific record that
> you're iterating it seems wrong to me to initialize these here.
>
Right
>> +     for ( ; recs; recs = recs->next) {
>> +             sdp_rec = recs->data;
>> +
>> +             switch (sdp_rec->svclass.type) {
>> +             case SDP_UUID16:
>> +                     sdp_uuid16_to_uuid128(&uuid128_sdp,
>> +                                                     &sdp_rec->svclass);
>> +                     break;
>> +             case SDP_UUID32:
>> +                     sdp_uuid32_to_uuid128(&uuid128_sdp,
>> +                                                     &sdp_rec->svclass);
>> +                     break;
>> +             case SDP_UUID128:
>> +                     break;
>> +             default:
>> +                     error("wrong sdp uuid type");
>> +                     goto done;
>> +             }
>> +
>> +             if (!sdp_get_access_protos(sdp_rec, &protos)) {
>> +                     channel = sdp_get_proto_port(protos, L2CAP_UUID);
>> +                     if (channel < 0)
>> +                             error("wrong channel");
>> +             }
>
> Probably this should be here:
>
>                 } else {
>                         channel = -1;
>                 }
>
I'll follow this idea
>> +             if (!sdp_get_service_name(sdp_rec, name_buf, sizeof(name_buf)))
>> +                     name_len = (uint8_t)strlen(name_buf);
>
> And here:
>
>                 else
>                         name_len = 0;
>
> Otherwise you'll end up including the name from previous records as the
> name of subsequent ones which don't have a service name attribute. Also,
> the (uint8_t) typecast seems unnecessary here (compiles fine for me
> without it).
>
yes, right. I'll fix it
>> +static uint8_t find_remote_sdp_rec(const bdaddr_t *addr,
>> +                                             const uint8_t *find_uuid)
>> +{
>> +     uuid_t uuid;
>> +     bdaddr_t *bdaddr;
>> +
>> +     /* from android we always get full 128bit length uuid */
>> +     sdp_uuid128_create(&uuid, find_uuid);
>> +
>> +     bdaddr = malloc(sizeof(*bdaddr));
>
> Could you tell me what's the rationale of choosing malloc vs g_malloc in
> this code. In most places this c-file is using the GLib allocators and
> having a mixed set is just begging for trouble (e.g. you might end up
> calling g_free on something allocated with malloc, or free for something
> allocated with g_malloc).
>
I'll change to Glib allocators
> Johan

BR,
Grzegorz