* GATT + DBus API Questions
From: Ryan Du Bois @ 2014-10-23 23:41 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]
Hey There,
I've been trying to create a GATT service via the DBus BlueZ API (org.bluez.GattManager1 + org.bluez.GattService1/GattCharacteristic1). I'm currently using dbus-python to do this.
What I have been able to accomplish thus far (with much exploring of the source code, and additional DBG() messages added to illuminate where I had been going wrong) is the following:
- Created proper DBus Proxy objects for GattService1 & GattCharacteristic1
- Each object vends the org.freedesktop.DBus.Properties interface, to allow BlueZ to retrieve the UUID and other properties on each instance.
- Created a proper ObjectManager proxy object to allow BlueZ to retrieve the aforementioned GattService1 and GattCharacteristic1 objects.
- Called GattManager1's RegisterService() method with appropriate arguments.
- Observed that BlueZ's GattManager1 instance calls my ObjectManager's GetManagedObjects() method, and receives the appropriate object paths/handles/properties.
- Observed that BlueZ accomplishes all the internal machinery necessary to register the GATT Service in src/gatt-dbus.c (namely, I get the "Added GATT service /com/kamama/blargh..." printout on the console, when running in debug mode).
What I'm not able to see is the GATT service being listed when I explore the GATT services on my device via a BTLE exploration tool (I'm using LightBlue on Mac OS X to explore this device).
Looking closer at the code, it looks to me like src/gatt-dbus.c is not hooked up to attrib/gatt-service.c, which is probably why my GATT Service and Characteristic are not showing up when exploring the device.
Am I missing something, or is this not yet implemented?
Thanks!
--
+Ryan Du Bois
rdub@kamama.com
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 496 bytes --]
^ permalink raw reply
* Re: [PATCH] Add new method - service discovery to mgmt
From: Marcel Holtmann @ 2014-10-23 22:29 UTC (permalink / raw)
To: Jakub Pawlowski; +Cc: linux-bluetooth
In-Reply-To: <1414084535-23864-2-git-send-email-jpawlowski@google.com>
Hi Jakub,
I am doing a basic review first here. I will do a more thorough one once we get the basics in place.
Every single kernel patch needs a proper commit message explain in detail what you are doing. I want to be able read the commit message and then compare that you implement exactly the feature / fix you are describing here.
> ---
> include/net/bluetooth/hci_core.h | 16 ++
> include/net/bluetooth/mgmt.h | 26 ++
> net/bluetooth/hci_core.c | 42 ++++
> net/bluetooth/hci_event.c | 3 +-
> net/bluetooth/mgmt.c | 499 ++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 582 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index b8685a7..d2b6661 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -143,6 +143,14 @@ struct oob_data {
> u8 randomizer256[16];
> };
>
> +struct uuid_filter {
> + struct list_head list;
> + u8 range_method;
> + s8 pathloss;
> + s8 rssi;
> + u8 uuid[16];
> +};
> +
> #define HCI_MAX_SHORT_NAME_LENGTH 10
>
> /* Default LE RPA expiry time, 15 minutes */
> @@ -307,6 +315,10 @@ struct hci_dev {
> struct discovery_state discovery;
> struct hci_conn_hash conn_hash;
>
> + struct list_head discov_uuid_filter;
> + bool service_filter_enabled;
> + bool scan_restart;
> +
> struct list_head mgmt_pending;
> struct list_head blacklist;
> struct list_head whitelist;
> @@ -334,6 +346,7 @@ struct hci_dev {
> unsigned long dev_flags;
>
> struct delayed_work le_scan_disable;
> + struct delayed_work le_scan_restart;
the scan restart handling should be its own separate patch. I rather not have that mixed into this. I know it is related, but could be also useful without it. In addition you want a HCI_QUIRK setting to control it since on a lot of chips this is not even needed. And where it is not needed, you are just wasting energy to stop and restart scanning.
> __s8 adv_tx_power;
> __u8 adv_data[HCI_MAX_AD_LENGTH];
> @@ -1303,6 +1316,8 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
> #define DISCOV_INTERLEAVED_INQUIRY_LEN 0x04
> #define DISCOV_BREDR_INQUIRY_LEN 0x08
>
> +#define DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
> +
> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
> int mgmt_new_settings(struct hci_dev *hdev);
> void mgmt_index_added(struct hci_dev *hdev);
> @@ -1369,6 +1384,7 @@ void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
> u16 max_interval, u16 latency, u16 timeout);
> void mgmt_reenable_advertising(struct hci_dev *hdev);
> void mgmt_smp_complete(struct hci_conn *conn, bool complete);
> +void clean_up_service_discovery(struct hci_dev *hdev);
>
> u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> u16 to_multiplier);
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 414cd2f..8d652b3 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -495,6 +495,32 @@ struct mgmt_cp_set_public_address {
> } __packed;
> #define MGMT_SET_PUBLIC_ADDRESS_SIZE 6
>
> +#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
> +
> +#define MGMT_RANGE_NONE 0X00
> +#define MGMT_RANGE_RSSI 0X01
> +#define MGMT_RANGE_PATHLOSS 0X02
> +
> +struct mgmt_uuid_filter {
> + __u8 range_method;
> + __s8 pathloss;
> + __s8 rssi;
> + __u8 uuid[16];
> +} __packed;
This is something we really need to discuss. I am not convinced that pathloss calculation should be done in the kernel. However that is something to discuss with a patch changing doc/mgmt-api.txt.
> +
> +struct mgmt_cp_start_service_discovery {
> + __u8 type;
> + __le16 filter_count;
> + struct mgmt_uuid_filter filter[0];
> +} __packed;
> +#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
> +
> +#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
> +struct mgmt_cp_stop_service_discovery {
> + __u8 type;
> +} __packed;
> +#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
> +
> #define MGMT_EV_CMD_COMPLETE 0x0001
> struct mgmt_ev_cmd_complete {
> __le16 opcode;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index cb05d7f..af6bf39 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2580,6 +2580,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> cancel_delayed_work(&hdev->service_cache);
>
> cancel_delayed_work_sync(&hdev->le_scan_disable);
> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>
> if (test_bit(HCI_MGMT, &hdev->dev_flags))
> cancel_delayed_work_sync(&hdev->rpa_expired);
> @@ -3846,6 +3847,7 @@ static void le_scan_disable_work(struct work_struct *work)
>
> BT_DBG("%s", hdev->name);
>
> + clean_up_service_discovery(hdev);
> hci_req_init(&req, hdev);
>
> hci_req_add_le_scan_disable(&req);
> @@ -3855,6 +3857,41 @@ static void le_scan_disable_work(struct work_struct *work)
> BT_ERR("Disable LE scanning request failed: err %d", err);
> }
>
> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
> +{
> + hdev->scan_restart = false;
> +
> + if (status) {
> + BT_ERR("Failed to restart LE scanning: status %d", status);
> + return;
> + }
The return is useless here. Might want to just print the error only.
> +}
> +
> +static void le_scan_restart_work(struct work_struct *work)
> +{
> + struct hci_dev *hdev = container_of(work, struct hci_dev,
> + le_scan_restart.work);
> + struct hci_request req;
> + struct hci_cp_le_set_scan_enable cp;
> + int err;
> +
> + BT_DBG("%s", hdev->name);
> +
> + hci_req_init(&req, hdev);
> +
> + hci_req_add_le_scan_disable(&req);
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.enable = LE_SCAN_ENABLE;
> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> +
> + hdev->scan_restart = true;
> +
> + err = hci_req_run(&req, le_scan_restart_work_complete);
> + if (err)
> + BT_ERR("Disable LE scanning request failed: err %d", err);
> +}
> +
> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
> {
> struct hci_dev *hdev = req->hdev;
> @@ -4010,6 +4047,10 @@ struct hci_dev *hci_alloc_dev(void)
> mutex_init(&hdev->lock);
> mutex_init(&hdev->req_lock);
>
> + INIT_LIST_HEAD(&hdev->discov_uuid_filter);
> + hdev->service_filter_enabled = false;
> + hdev->scan_restart = false;
> +
> INIT_LIST_HEAD(&hdev->mgmt_pending);
> INIT_LIST_HEAD(&hdev->blacklist);
> INIT_LIST_HEAD(&hdev->whitelist);
> @@ -4032,6 +4073,7 @@ struct hci_dev *hci_alloc_dev(void)
> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
> INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
> INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
> + INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
>
> skb_queue_head_init(&hdev->rx_q);
> skb_queue_head_init(&hdev->cmd_q);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 9629153..b372b02 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1155,7 +1155,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> /* Cancel this timer so that we don't try to disable scanning
> * when it's already disabled.
> */
> - cancel_delayed_work(&hdev->le_scan_disable);
> + if(!hdev->scan_restart)
> + cancel_delayed_work(&hdev->le_scan_disable);
Please obey the coding style "if (!hdev..)".
>
> clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 9c4daf7..08d48a7 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -93,6 +93,8 @@ static const u16 mgmt_commands[] = {
> MGMT_OP_READ_CONFIG_INFO,
> MGMT_OP_SET_EXTERNAL_CONFIG,
> MGMT_OP_SET_PUBLIC_ADDRESS,
> + MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_OP_STOP_SERVICE_DISCOVERY
> };
>
> static const u16 mgmt_events[] = {
> @@ -1258,6 +1260,26 @@ static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
> }
> }
>
> +void clean_up_service_discovery(struct hci_dev *hdev)
> +{
> + if (hdev->service_filter_enabled) {
> + struct uuid_filter *filterptr = NULL;
> + struct uuid_filter *tmp = NULL;
> +
> + hdev->service_filter_enabled = false;
> + cancel_delayed_work_sync(&hdev->le_scan_restart);
> +
> + if(!list_empty(&hdev->discov_uuid_filter)) {
Here as well. Space after if.
> + list_for_each_entry_safe(filterptr, tmp,
> + &hdev->discov_uuid_filter, list)
> + {
The { has to be on the previous line and the multi-line parameters need to be properly aligned.
> + __list_del_entry(&(filterptr->list));
> + kfree(filterptr);
> + }
> + }
> + }
> +}
The whole function could need some restructure. For example.
if (!hdev->service_filter_enabled)
return;
And of course service_filter_enabled could be just a hdev->dev_flags.
Same as with this:
if (list_empty())
return;
> +
> static bool hci_stop_discovery(struct hci_request *req)
> {
> struct hci_dev *hdev = req->hdev;
> @@ -1270,6 +1292,7 @@ static bool hci_stop_discovery(struct hci_request *req)
> hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> } else {
> cancel_delayed_work(&hdev->le_scan_disable);
> + clean_up_service_discovery(hdev);
> hci_req_add_le_scan_disable(req);
> }
>
> @@ -5641,6 +5664,345 @@ unlock:
> return err;
> }
>
> +static int mgmt_start_service_discovery_failed(struct hci_dev *hdev, u8 status)
> +{
> + struct pending_cmd *cmd;
> + u8 type;
> + int err;
> +
> + hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +
> + cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY, hdev);
> + if (!cmd)
> + return -ENOENT;
> +
> + type = hdev->discovery.type;
> +
> + err = cmd_complete(cmd->sk, hdev->id, cmd->opcode, mgmt_status(status),
> + &type, sizeof(type));
> + mgmt_pending_remove(cmd);
> +
> + return err;
> +}
> +
> +static void start_service_discovery_complete(struct hci_dev *hdev, u8 status)
> +{
> + unsigned long timeout = 0;
> +
> + BT_DBG("status %d", status);
> +
> + if (status) {
> + hci_dev_lock(hdev);
> + mgmt_start_service_discovery_failed(hdev, status);
> + hci_dev_unlock(hdev);
> + return;
> + }
> +
> + hci_dev_lock(hdev);
> + hci_discovery_set_state(hdev, DISCOVERY_FINDING);
> + hci_dev_unlock(hdev);
> +
> + switch (hdev->discovery.type) {
> + case DISCOV_TYPE_LE:
> + timeout = msecs_to_jiffies(DISCOV_LE_TIMEOUT);
> + break;
> +
> + case DISCOV_TYPE_INTERLEAVED:
> + timeout = msecs_to_jiffies(hdev->discov_interleaved_timeout);
> + break;
> +
> + case DISCOV_TYPE_BREDR:
> + break;
> +
> + default:
> + BT_ERR("Invalid discovery type %d", hdev->discovery.type);
> + }
> +
> + if (!timeout)
> + return;
> +
> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable, timeout);
> +}
> +
> +static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 len)
> +{
> + struct mgmt_cp_start_service_discovery *cp = data;
> + struct pending_cmd *cmd;
> + struct hci_cp_le_set_scan_param param_cp;
> + struct hci_cp_le_set_scan_enable enable_cp;
> + struct hci_cp_inquiry inq_cp;
> + struct hci_request req;
> + /* General inquiry access code (GIAC) */
> + u8 lap[3] = { 0x33, 0x8b, 0x9e };
> + u8 status, own_addr_type;
> + int i, err;
> + u16 filter_count, expected_len;
> +
> + BT_DBG("%s", hdev->name);
> +
> + filter_count = __le16_to_cpu(cp->filter_count);
> +
> + expected_len = sizeof(*cp) + filter_count * sizeof(struct mgmt_uuid_filter);
> + if (expected_len != len) {
> + BT_ERR("start_service_discovery: expected %u bytes, got %u bytes",
> + expected_len, len);
> +
> + return cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_INVALID_PARAMS);
> + }
> +
> + hci_dev_lock(hdev);
> +
> + if (!hdev_is_powered(hdev)) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_NOT_POWERED);
> + goto failed;
> + }
> +
> + if (test_bit(HCI_PERIODIC_INQ, &hdev->dev_flags)) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_BUSY);
> + goto failed;
> + }
> +
> + if (hdev->discovery.state != DISCOVERY_STOPPED) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_BUSY);
> + goto failed;
> + }
> +
> + cmd = mgmt_pending_add(sk, MGMT_OP_START_SERVICE_DISCOVERY, hdev, NULL, 0);
> + if (!cmd) {
> + err = -ENOMEM;
> + goto failed;
> + }
> +
> + hdev->discovery.type = cp->type;
> +
> + hci_req_init(&req, hdev);
> +
> + switch (hdev->discovery.type) {
> + case DISCOV_TYPE_BREDR:
> + status = mgmt_bredr_support(hdev);
> + if (status) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + status);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
> +
> + if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_BUSY);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
> +
> + hci_inquiry_cache_flush(hdev);
> +
> + memset(&inq_cp, 0, sizeof(inq_cp));
> + memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
> + inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;
> + hci_req_add(&req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
> + break;
> +
> + case DISCOV_TYPE_LE:
> + case DISCOV_TYPE_INTERLEAVED:
> + status = mgmt_le_support(hdev);
> + if (status) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + status);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
> +
> + if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
> + !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_NOT_SUPPORTED);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
> +
> + if (test_bit(HCI_LE_ADV, &hdev->dev_flags)) {
> + /* Don't let discovery abort an outgoing
> + * connection attempt that's using directed
> + * advertising.
> + */
> + if (hci_conn_hash_lookup_state(hdev, LE_LINK,
> + BT_CONNECT)) {
> + err = cmd_status(sk, hdev->id,
> + MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_REJECTED);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
> +
> + disable_advertising(&req);
> + }
> +
> + /* If controller is scanning, it means the background scanning
> + * is running. Thus, we should temporarily stop it in order to
> + * set the discovery scanning parameters.
> + */
> + if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> + hci_req_add_le_scan_disable(&req);
> +
> + memset(¶m_cp, 0, sizeof(param_cp));
> +
> + /* All active scans will be done with either a resolvable
> + * private address (when privacy feature has been enabled)
> + * or unresolvable private address.
> + */
> + err = hci_update_random_address(&req, true, &own_addr_type);
> + if (err < 0) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_FAILED);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
> +
> + param_cp.type = LE_SCAN_ACTIVE;
> + param_cp.interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
> + param_cp.window = cpu_to_le16(DISCOV_LE_SCAN_WIN);
> + param_cp.own_address_type = own_addr_type;
> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
> + ¶m_cp);
> +
> + memset(&enable_cp, 0, sizeof(enable_cp));
> + enable_cp.enable = LE_SCAN_ENABLE;
> + enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
> + &enable_cp);
> + break;
> +
> + default:
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_STATUS_INVALID_PARAMS);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
Feels like we are duplicated a lot of code. A separate patch splitting this out into its own function seems to be needed here.
> +
> + for (i = 0; i < filter_count; i++) {
> + struct mgmt_uuid_filter *key = &cp->filter[i];
> + struct uuid_filter *tmp_uuid;
> +
> + tmp_uuid = kmalloc(sizeof(struct uuid_filter), GFP_KERNEL);
> + if (!tmp_uuid)
> + return -ENOMEM;
> +
> + memcpy(tmp_uuid->uuid, key->uuid, 16);
> + tmp_uuid->range_method = key->range_method;
> + tmp_uuid->pathloss = key->pathloss;
> + tmp_uuid->rssi = key->rssi;
> + INIT_LIST_HEAD( &tmp_uuid->list ) ;
> +
> + list_add(&tmp_uuid->list, &hdev->discov_uuid_filter);
> + }
> + hdev->service_filter_enabled = true;
> +
> + err = hci_req_run(&req, start_service_discovery_complete);
> + if (err < 0)
> + mgmt_pending_remove(cmd);
> + else
> + hci_discovery_set_state(hdev, DISCOVERY_STARTING);
> +
> +failed:
> + hci_dev_unlock(hdev);
> + return err;
> +}
> +
> +static int mgmt_stop_service_discovery_failed(struct hci_dev *hdev, u8 status)
> +{
> + struct pending_cmd *cmd;
> + int err;
> +
> + cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY, hdev);
> + if (!cmd)
> + return -ENOENT;
> +
> + hdev->service_filter_enabled = false;
> + err = cmd_complete(cmd->sk, hdev->id, cmd->opcode, mgmt_status(status),
> + &hdev->discovery.type, sizeof(hdev->discovery.type));
> + mgmt_pending_remove(cmd);
> +
> + return err;
> +}
> +
> +static void stop_service_discovery_complete(struct hci_dev *hdev, u8 status)
> +{
> + BT_DBG("status %d", status);
> +
> + hci_dev_lock(hdev);
> +
> + if (status) {
> + mgmt_stop_service_discovery_failed(hdev, status);
> + goto unlock;
> + }
> +
> + hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +
> +unlock:
> + hci_dev_unlock(hdev);
> +}
> +
> +static int stop_service_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
> + u16 len)
> +{
> + struct mgmt_cp_stop_service_discovery *mgmt_cp = data;
> + struct pending_cmd *cmd;
> + struct hci_request req;
> + int err;
> +
> + BT_DBG("%s", hdev->name);
> +
> + hci_dev_lock(hdev);
> +
> + if (!hci_discovery_active(hdev)) {
> + err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_SERVICE_DISCOVERY,
> + MGMT_STATUS_REJECTED, &mgmt_cp->type,
> + sizeof(mgmt_cp->type));
> + goto unlock;
> + }
> +
> + if (hdev->discovery.type != mgmt_cp->type) {
> + err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_SERVICE_DISCOVERY,
> + MGMT_STATUS_INVALID_PARAMS, &mgmt_cp->type,
> + sizeof(mgmt_cp->type));
> + goto unlock;
> + }
> +
> + cmd = mgmt_pending_add(sk, MGMT_OP_STOP_SERVICE_DISCOVERY, hdev, NULL, 0);
> + if (!cmd) {
> + err = -ENOMEM;
> + goto unlock;
> + }
> +
> + hci_req_init(&req, hdev);
> +
> + hci_stop_discovery(&req);
> +
> + err = hci_req_run(&req, stop_service_discovery_complete);
> + if (!err) {
> + hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
> + goto unlock;
> + }
> +
> + mgmt_pending_remove(cmd);
> +
> + /* If no HCI commands were sent we're done */
> + if (err == -ENODATA) {
> + err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_SERVICE_DISCOVERY, 0,
> + &mgmt_cp->type, sizeof(mgmt_cp->type));
> + hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> + }
> +
> +unlock:
> + hci_dev_unlock(hdev);
> + return err;
> +}
> +
> static const struct mgmt_handler {
> int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
> u16 data_len);
> @@ -5705,6 +6067,8 @@ static const struct mgmt_handler {
> { read_config_info, false, MGMT_READ_CONFIG_INFO_SIZE },
> { set_external_config, false, MGMT_SET_EXTERNAL_CONFIG_SIZE },
> { set_public_address, false, MGMT_SET_PUBLIC_ADDRESS_SIZE },
> + { start_service_discovery,true, MGMT_START_SERVICE_DISCOVERY_SIZE },
> + { stop_service_discovery, false, MGMT_STOP_SERVICE_DISCOVERY_SIZE },
Please the fields here.
> };
>
> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
> @@ -6774,6 +7138,85 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
> mgmt_pending_remove(cmd);
> }
>
> +struct parsed_uuid {
> + struct list_head list;
> + u8 uuid[16];
> +};
> +
> +//static u8 baseUUID[16] = {0x00,0x00,0x00,0x00,/*-*/0x00,0x00,/*-*/0x10,0x00,/*-*/ 0x80,0x00,/*-*/0x00,0x80,0x5f,0x9b,0x34,0xfb};
> +static u8 reverse_base_uuid[16] = {0xfb,0x34,0x9b,0x5f,0x80,0x00,0x00,0x80,0x00,0x10,0x00,0x00,0x00,0x00,0x00,0x00};
No. Leftover comment, not const, wrong coding style and generally no explanation on why we would be doing this in the first place.
> +
> +static void add_uuid_to_list(struct list_head *uuids, u8 *uuid) {
Coding style. The { belongs in the next line here.
> + struct parsed_uuid *tmp_uuid;
Empty line here.
> + tmp_uuid = kmalloc(sizeof(struct parsed_uuid), GFP_KERNEL);
> + if (tmp_uuid == NULL)
> + return; //TODO: how to handle if there's no memory?
> +
> + memcpy(tmp_uuid->uuid, uuid, 16);
> + INIT_LIST_HEAD(&tmp_uuid->list);
> + list_add(&tmp_uuid->list, uuids);
> +}
> +
> +static void eir_parse(u8 *eir, u8 eir_len, struct list_head *uuids,
> + u8 *txpwr_present, s8 *txpwr)
Coding style. Align the multi-line parameters.
> +{
> + size_t offset;
> + u8 uuid[16];
> + int loop;
> +
> + offset = 0;
> + while (offset < eir_len) {
> + uint8_t field_len = eir[0];
> +
> + /* Check for the end of EIR */
> + if (field_len == 0)
> + break;
> +
> + if (offset + field_len > eir_len)
> + goto failed;
> +
> + switch (eir[1]) {
> + case EIR_TX_POWER:
> + *txpwr_present = 1;
> + *txpwr = eir[2];
> + break;
> + case EIR_UUID16_ALL:
> + case EIR_UUID16_SOME:
> + for(loop = 0; loop+3<=field_len;loop+=2) {
Empty space after for. And spaces between + and <=. And after ;.
> + memcpy(uuid, reverse_base_uuid, 16);
> + uuid[13] = eir[loop + 3];
> + uuid[12] = eir[loop + 2];
> + add_uuid_to_list(uuids, uuid);
> + }
> + break;
> + case EIR_UUID32_ALL:
> + case EIR_UUID32_SOME:
> + for(loop = 0; loop+5<=field_len;loop+=4) {
> + memcpy(uuid, reverse_base_uuid, 16);
> + uuid[15] = eir[loop + 5];
> + uuid[14] = eir[loop + 4];
> + uuid[13] = eir[loop + 3];
> + uuid[12] = eir[loop + 2];
> + add_uuid_to_list(uuids, uuid);
> + }
> + break;
> + case EIR_UUID128_ALL:
> + case EIR_UUID128_SOME:
> + for(loop = 0; loop+17<=field_len;loop+=4) {
> + memcpy(uuid, eir + loop + 2, 16);
> + add_uuid_to_list(uuids, uuid);
> + }
> + break;
> + }
> +
> + offset += field_len + 1;
> + eir += field_len + 1;
> + }
And as a site note, LE has service data and service solicitation that also has UUIDs in there.
> +
> +failed:
> + offset = 8;
> +}
> +
> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
> u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
> @@ -6819,7 +7262,52 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
> ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
>
> - mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> + if(hdev->service_filter_enabled) {
Space after if. You have a few of these, please fix them all.
> + LIST_HEAD(uuids);
> + s8 txpwr;
> + u8 txpwr_present = 0;
> + struct uuid_filter *filterptr = NULL;
> + struct parsed_uuid *uuidptr = NULL;
> + struct parsed_uuid *tmp = NULL;
> + bool service_match = false, full_match = false;
> +
> + eir_parse(eir, eir_len, &uuids, &txpwr_present, &txpwr);
> + if(!list_empty(&uuids)) {
> + list_for_each_entry_safe(uuidptr, tmp, &uuids, list)
> + {
> + list_for_each_entry(filterptr, &hdev->discov_uuid_filter, list)
> + {
> + if(memcmp(filterptr->uuid, uuidptr->uuid, 16) == 0) {
> + service_match = true;
> +
> + if( filterptr->range_method == MGMT_RANGE_NONE) {
No empty space after (
> + full_match = true;
> + } else if ( (filterptr->range_method & MGMT_RANGE_PATHLOSS)
> + && txpwr_present
> + && (txpwr - rssi) <= filterptr->pathloss ) {
> + full_match = true;
> + } else if( (filterptr->range_method & MGMT_RANGE_RSSI)
> + && rssi >= filterptr->rssi ) {
> + full_match = true;
> + }
> +
> + }
> + }
This needs to be changed to fit into the 80 chars per line.
> + __list_del_entry(&(uuidptr->list));
> + kfree(uuidptr);
> + }
> + }
> +
> + if(full_match) {
> + mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> + }
> + if(service_match) {
> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
> + DISCOV_LE_RESTART_DELAY);
Coding style for parameter alignment on second line. Please make sure you get them all fixed.
> + }
> + } else {
> + mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> + }
> }
>
> void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> @@ -6852,10 +7340,15 @@ void mgmt_discovering(struct hci_dev *hdev, u8 discovering)
>
> BT_DBG("%s discovering %u", hdev->name, discovering);
>
> - if (discovering)
> + if (discovering) {
> cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
> - else
> + if(cmd == NULL)
> + cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY, hdev);
> + } else {
> cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
> + if(cmd == NULL)
> + cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY, hdev);
> + }
>
> if (cmd != NULL) {
> u8 type = hdev->discovery.type;
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/1 net-next] Bluetooth: fix shadow warning in hci_disconnect()
From: Marcel Holtmann @ 2014-10-23 22:09 UTC (permalink / raw)
To: Fabian Frederick
Cc: Gustavo F. Padovan, Network Development, linux-kernel,
David S. Miller, BlueZ development, Johan Hedberg
In-Reply-To: <20326343.74480.1414085453988.open-xchange@webmail.nmp.skynet.be>
Hi Fabian,
>>> use cpr for hci_cp_read_clock_offset instead of cp
>>> (already defined above).
>>>
>>> Signed-off-by: Fabian Frederick <fabf@skynet.be>
>>> ---
>>> net/bluetooth/hci_conn.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index b9517bd..6151e09 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -141,10 +141,10 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason)
>>> */
>>> if (conn->type == ACL_LINK && conn->role == HCI_ROLE_MASTER) {
>>> struct hci_dev *hdev = conn->hdev;
>>> - struct hci_cp_read_clock_offset cp;
>>> + struct hci_cp_read_clock_offset cpr;
>>>
>>> - cp.handle = cpu_to_le16(conn->handle);
>>> - hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cp), &cp);
>>> + cpr.handle = cpu_to_le16(conn->handle);
>>> + hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cpr), &cpr);
>>
>> valid change, but I do not like cpr as variable name. We need to come up with
>> a better one. There are other places in the code where we had the same thing.
>> Please have a look there.
>
> Maybe read_cp (like commit c1f23a2bfc89 with struct hci_cp_auth_requested
> auth_cp) ?
lets use clkoff_cp unless someone comes up with a better name.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH BlueZ 1/8] shared/att: bt_att_cancel should not cancel pending requests/indications.
From: Arman Uguray @ 2014-10-23 18:55 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Arman Uguray, linux-bluetooth@vger.kernel.org
In-Reply-To: <CAHrH25R-SFMYOB+E2VzFogr0NRSB52XHfiQ+JQj0W+oAYdXRLw@mail.gmail.com>
Hi Luiz,
On Wed, Oct 22, 2014 at 8:40 AM, Arman Uguray <armansito@google.com> wrote:
> Hi Luiz,
>
>
> On Wed, Oct 22, 2014 at 7:39 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Arman,
>>
>> On Tue, Oct 21, 2014 at 12:00 AM, Arman Uguray <armansito@chromium.org> wrote:
>>> bt_att_cancel and bt_att_cancel_all no longer destroy any pending
>>> request or indication, since this would cause a later bt_att_send to
>>> erroneously send out a request or indication before a
>>> response/confirmation for a pending one is received. Instead, they now
>>> keep the pending operations but clear their response and destroy
>>> callbacks since the upper layer is no longer interested in them.
>>> ---
>>> src/shared/att.c | 24 ++++++++++++++----------
>>> 1 file changed, 14 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>> index 503e06c..c70d396 100644
>>> --- a/src/shared/att.c
>>> +++ b/src/shared/att.c
>>> @@ -1029,15 +1029,17 @@ bool bt_att_cancel(struct bt_att *att, unsigned int id)
>>> return false;
>>>
>>> if (att->pending_req && att->pending_req->id == id) {
>>> - op = att->pending_req;
>>> - att->pending_req = NULL;
>>> - goto done;
>>> + /* Don't cancel the pending request; remove it's handlers */
>>> + att->pending_req->callback = NULL;
>>> + att->pending_req->destroy = NULL;
>>> + return true;
>>> }
>>>
>>> if (att->pending_ind && att->pending_ind->id == id) {
>>> - op = att->pending_ind;
>>> - att->pending_ind = NULL;
>>> - goto done;
>>> + /* Don't cancel the pending indication; remove it's handlers */
>>> + att->pending_ind->callback = NULL;
>>> + att->pending_ind->destroy = NULL;
>>> + return true;
>>> }
>>>
>>> op = queue_remove_if(att->req_queue, match_op_id, UINT_TO_PTR(id));
>>> @@ -1073,13 +1075,15 @@ bool bt_att_cancel_all(struct bt_att *att)
>>> queue_remove_all(att->write_queue, NULL, NULL, destroy_att_send_op);
>>>
>>> if (att->pending_req) {
>>> - destroy_att_send_op(att->pending_req);
>>> - att->pending_req = NULL;
>>> + /* Don't cancel the pending request; remove it's handlers */
>>> + att->pending_req->callback = NULL;
>>> + att->pending_req->destroy = NULL;
>>> }
>>>
>>> if (att->pending_ind) {
>>> - destroy_att_send_op(att->pending_ind);
>>> - att->pending_ind = NULL;
>>> + /* Don't cancel the pending indication; remove it's handlers */
>>> + att->pending_ind->callback = NULL;
>>> + att->pending_ind->destroy = NULL;
>>> }
>>>
>>> return true;
>>> --
>>> 2.1.0.rc2.206.gedb03e5
>>
>> I would like to first finish with Marcin's patches, are you okay with
>> v5? Btw, please make sure you follow the HACKING document when
>> submitting patches so we can be consistent from now on.
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> I'm fine with Marcin's v5, though we may have to wait for v6 since I
> saw some comments fly by from Szymon.
>
> Yeah I'm following HACKING from now on. So far I used the code around
> me (shared/mgmt, etc) as the style guide and might have repeated
> mistakes that already existed in the code, so it might makes sense to
> do a general style fix pass within src/shared at some point.
>
> -Arman
Looks like Marcin's patches have been merged so should we go ahead
with my patch set? Also, I'm adding TODO items for adding packed
structs for ATT PDUs and adding complete GATT unit test coverage in
unit/test-gatt in the next patch set. We should probably wait until we
have enough unit tests before we convert the code to use packed
structs to make sure that we don't break anything.
-Arman
^ permalink raw reply
* Re: [PATCH BlueZ 0/5] Unit tests for GAttrib
From: Michael Janssen @ 2014-10-23 17:50 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1414082009-40903-1-git-send-email-jamuraa@chromium.org>
On Thu Oct 23 2014 at 9:33:36 AM Michael Janssen <jamuraa@chromium.org> wrote:
>
> Since we want to shim GAttrib for a smooth transition to bt_att
> profiles, this patch cleans up the API and adds unit tests for
> GAttrib functions.
>
> This adds tests for all but the register / unregister functions, which
> I will send in a separate patch since they are slightly more complicated due
> to catchalls.
>
> Michael Janssen (5):
> Remove unused g_attrib_set_debug function
> attrib: Remove MTU-probing code
> attrib: Add mtu argument to g_attrib_new
> attrib: remove g_attrib_is_encrypted
> Add unit test for gattrib
>
> .gitignore | 1 +
> Makefile.am | 7 +
> attrib/gattrib.c | 38 +---
> attrib/gattrib.h | 7 +-
> attrib/gatttool.c | 17 +-
> attrib/interactive.c | 17 +-
> src/attrib-server.c | 9 +-
> src/device.c | 11 +-
> unit/test-gattrib.c | 584 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 9 files changed, 645 insertions(+), 46 deletions(-)
> create mode 100644 unit/test-gattrib.c
>
> --
> 2.1.0.rc2.206.gedb03e5
>
I just realized this patchset is missing a bunch of style fixes (also,
it has a massive block comment) in unit/test-gattrib.c. A v2 will
come which fixes style to conform with HACKING. If there are other
comments I will incorporate those too.
^ permalink raw reply
* Re: [PATCH 1/1 net-next] Bluetooth: fix shadow warning in hci_disconnect()
From: Fabian Frederick @ 2014-10-23 17:30 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo F. Padovan, Network Development, linux-kernel,
David S. Miller, BlueZ development, Johan Hedberg
In-Reply-To: <2F67F6E8-CA99-4998-AE68-89E60D62C39D@holtmann.org>
> On 23 October 2014 at 18:29 Marcel Holtmann <marcel@holtmann.org> wrote:
>
>
> Hi Fabian,
>
> > use cpr for hci_cp_read_clock_offset instead of cp
> > (already defined above).
> >
> > Signed-off-by: Fabian Frederick <fabf@skynet.be>
> > ---
> > net/bluetooth/hci_conn.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index b9517bd..6151e09 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -141,10 +141,10 @@ int hci_disconnect(struct hci_conn *conn, __u8 re=
ason)
> >=C2=A0 =C2=A0 =C2=A0 */
> >=C2=A0 =C2=A0 =C2=A0if (conn->type =3D=3D ACL_LINK && conn->role =3D=3D =
HCI_ROLE_MASTER) {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct hci_dev *hdev =3D=
conn->hdev;
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct hci_cp_read_clock_offs=
et cp;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct hci_cp_read_clock_offs=
et cpr;
> >
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cp.handle =3D cpu_to_le16(con=
n->handle);
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0hci_send_cmd(hdev, HCI_OP_REA=
D_CLOCK_OFFSET, sizeof(cp), &cp);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cpr.handle =3D cpu_to_le16(co=
nn->handle);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0hci_send_cmd(hdev, HCI_OP_REA=
D_CLOCK_OFFSET, sizeof(cpr), &cpr);
>
> valid change, but I do not like cpr as variable name. We need to come up =
with
> a better one. There are other places in the code where we had the same th=
ing.
> Please have a look there.
>
> Regards
>
> Marcel
>
Hi Marcel,
=C2=A0 Maybe read_cp (like commit c1f23a2bfc89 with struct hci_cp_auth_requ=
ested
auth_cp) ?
Regards,
Fabian
^ permalink raw reply
* [PATCH] Add new method - service discovery
From: Jakub Pawlowski @ 2014-10-23 17:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jakub Pawlowski
In-Reply-To: <1414084874-24359-1-git-send-email-jpawlowski@google.com>
---
lib/mgmt.h | 26 ++++++++++++
tools/btmgmt.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 154 insertions(+)
diff --git a/lib/mgmt.h b/lib/mgmt.h
index 46766a9..8f37937 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -445,6 +445,32 @@ struct mgmt_cp_set_public_address {
bdaddr_t bdaddr;
} __packed;
+#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
+
+#define MGMT_RANGE_NONE 0X00
+#define MGMT_RANGE_RSSI 0X01
+#define MGMT_RANGE_PATHLOSS 0X02
+
+struct mgmt_uuid_filter {
+ uint8_t range_method;
+ int8_t pathloss;
+ int8_t rssi;
+ uint8_t uuid[16];
+} __packed;
+
+struct mgmt_cp_start_service_discovery {
+ uint8_t type;
+ uint16_t filter_count;
+ struct mgmt_uuid_filter filter[0];
+} __packed;
+#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
+
+#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
+struct mgmt_cp_stop_service_discovery {
+ uint8_t type;
+} __packed;
+#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
uint16_t opcode;
diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 03d0a05..2a919a7 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -1546,6 +1546,133 @@ static void cmd_con(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
}
}
+static void find_service_rsp(uint8_t status, uint16_t len, const void *param,
+ void *user_data)
+{
+ free(user_data);
+ if (status != 0) {
+ fprintf(stderr,
+ "Unable to start service discovery. status 0x%02x (%s)\n",
+ status, mgmt_errstr(status));
+ mainloop_quit();
+ return;
+ }
+
+ printf("Service discovery started\n");
+ discovery = true;
+}
+
+static void find_service_usage(void)
+{
+ printf("Usage: btmgmt find-service -u UUID [-r RSSI_Threshold] [-p Pathloss_Threshold] [-l|-b]>\n");
+}
+
+static struct option find_service_options[] = {
+ { "help", no_argument, 0, 'h' },
+ { "le-only", no_argument, 0, 'l' },
+ { "bredr-only", no_argument, 0, 'b' },
+ { "uuid", required_argument, 0, 'u' },
+ { "rssi", required_argument, 0, 'r' },
+ { "pathloss", required_argument, 0, 'p' },
+ { 0, 0, 0, 0 }
+};
+
+static void uuid_to_uuid128(uuid_t *uuid128, const uuid_t *uuid);
+
+static void cmd_find_service(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
+{
+ struct mgmt_cp_start_service_discovery *cp;
+ struct mgmt_uuid_filter *filter;
+ uint8_t type;
+ int opt;
+ int total_size = sizeof(struct mgmt_cp_start_service_discovery) + sizeof(struct mgmt_uuid_filter);
+ uuid_t uuid;
+ uint128_t uint128;
+ uuid_t uuid128;
+
+ if (index == MGMT_INDEX_NONE)
+ index = 0;
+
+ type = 0;
+ hci_set_bit(BDADDR_BREDR, &type);
+ hci_set_bit(BDADDR_LE_PUBLIC, &type);
+ hci_set_bit(BDADDR_LE_RANDOM, &type);
+
+ cp = malloc(total_size);
+ if(cp == NULL) {
+ fprintf(stderr, "Unable to allocate memory for mgmt_cp_start_service_discovery structure.\n");
+ }
+
+ memset(cp, 0, total_size);
+
+ filter = cp->filter;
+
+ if(argc == 1) {
+ find_service_usage();
+ exit(EXIT_FAILURE);
+ }
+
+ while ((opt = getopt_long(argc, argv, "+lbu:r:p:h", find_service_options,
+ NULL)) != -1) {
+ switch (opt) {
+ case 'l':
+ hci_clear_bit(BDADDR_BREDR, &type);
+ hci_set_bit(BDADDR_LE_PUBLIC, &type);
+ hci_set_bit(BDADDR_LE_RANDOM, &type);
+ break;
+ case 'b':
+ hci_set_bit(BDADDR_BREDR, &type);
+ hci_clear_bit(BDADDR_LE_PUBLIC, &type);
+ hci_clear_bit(BDADDR_LE_RANDOM, &type);
+ break;
+ case 'u':
+ if (bt_string2uuid(&uuid, optarg) < 0) {
+ printf("Invalid UUID: %s\n", optarg);
+ exit(EXIT_FAILURE);
+ }
+ uuid_to_uuid128(&uuid128, &uuid);
+ ntoh128((uint128_t *) uuid128.value.uuid128.data, &uint128);
+ htob128(&uint128, (uint128_t *) filter->uuid);
+ break;
+ case 'r':
+ filter->rssi = atoi(optarg);
+ filter->range_method |= MGMT_RANGE_RSSI;
+ printf("rssi filter: %d %d", filter->rssi, filter->range_method);
+ break;
+ case 'p':
+ filter->pathloss = atoi(optarg);
+ filter->range_method |= MGMT_RANGE_PATHLOSS;
+ printf("pathloss filter: %d %d", filter->pathloss, filter->range_method);
+ break;
+ case 'h':
+ find_service_usage();
+ exit(EXIT_SUCCESS);
+ default:
+ find_service_usage();
+ exit(EXIT_FAILURE);
+ }
+ }
+
+ argc -= optind;
+ argv += optind;
+ optind = 0;
+
+ if (argc > 0) {
+ find_service_usage();
+ exit(EXIT_FAILURE);
+ }
+
+ cp->type = type;
+ cp->filter_count = 1;
+
+ if (mgmt_send(mgmt, MGMT_OP_START_SERVICE_DISCOVERY, index, total_size, cp,
+ find_service_rsp, cp, NULL) == 0) {
+ free(cp);
+ fprintf(stderr, "Unable to send start_service_discovery cmd\n");
+ exit(EXIT_FAILURE);
+ }
+}
+
static void find_rsp(uint8_t status, uint16_t len, const void *param,
void *user_data)
{
@@ -2866,6 +2993,7 @@ static struct {
{ "disconnect", cmd_disconnect, "Disconnect device" },
{ "con", cmd_con, "List connections" },
{ "find", cmd_find, "Discover nearby devices" },
+ { "find-service", cmd_find_service, "Discover nearby service" },
{ "name", cmd_name, "Set local name" },
{ "pair", cmd_pair, "Pair with a remote device" },
{ "cancelpair", cmd_cancel_pair,"Cancel pairing" },
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH] Add new method - service discovery
From: Jakub Pawlowski @ 2014-10-23 17:21 UTC (permalink / raw)
To: linux-bluetooth
This path enable usage of start service discovery method. Example usage:
Find all low energy devices advertising service with UUID 'abcd' and TxPower, that have pathloss less than 60 dB
btmgmt find-service -u abcd -p 60 -l
Find all low energy devices advertising service with UUID 'abcd', and dont advertise TxPower and with RSSI over -50, or advertising TxPower and pathloss less than 60 dB
btmgmt find-service -u abcd -r -50 -p 60 -l
^ permalink raw reply
* [PATCH] Add new method - service discovery to mgmt
From: Jakub Pawlowski @ 2014-10-23 17:15 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jakub Pawlowski
In-Reply-To: <1414084535-23864-1-git-send-email-jpawlowski@google.com>
---
include/net/bluetooth/hci_core.h | 16 ++
include/net/bluetooth/mgmt.h | 26 ++
net/bluetooth/hci_core.c | 42 ++++
net/bluetooth/hci_event.c | 3 +-
net/bluetooth/mgmt.c | 499 ++++++++++++++++++++++++++++++++++++++-
5 files changed, 582 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b8685a7..d2b6661 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -143,6 +143,14 @@ struct oob_data {
u8 randomizer256[16];
};
+struct uuid_filter {
+ struct list_head list;
+ u8 range_method;
+ s8 pathloss;
+ s8 rssi;
+ u8 uuid[16];
+};
+
#define HCI_MAX_SHORT_NAME_LENGTH 10
/* Default LE RPA expiry time, 15 minutes */
@@ -307,6 +315,10 @@ struct hci_dev {
struct discovery_state discovery;
struct hci_conn_hash conn_hash;
+ struct list_head discov_uuid_filter;
+ bool service_filter_enabled;
+ bool scan_restart;
+
struct list_head mgmt_pending;
struct list_head blacklist;
struct list_head whitelist;
@@ -334,6 +346,7 @@ struct hci_dev {
unsigned long dev_flags;
struct delayed_work le_scan_disable;
+ struct delayed_work le_scan_restart;
__s8 adv_tx_power;
__u8 adv_data[HCI_MAX_AD_LENGTH];
@@ -1303,6 +1316,8 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
#define DISCOV_INTERLEAVED_INQUIRY_LEN 0x04
#define DISCOV_BREDR_INQUIRY_LEN 0x08
+#define DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
+
int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
int mgmt_new_settings(struct hci_dev *hdev);
void mgmt_index_added(struct hci_dev *hdev);
@@ -1369,6 +1384,7 @@ void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
u16 max_interval, u16 latency, u16 timeout);
void mgmt_reenable_advertising(struct hci_dev *hdev);
void mgmt_smp_complete(struct hci_conn *conn, bool complete);
+void clean_up_service_discovery(struct hci_dev *hdev);
u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
u16 to_multiplier);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 414cd2f..8d652b3 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -495,6 +495,32 @@ struct mgmt_cp_set_public_address {
} __packed;
#define MGMT_SET_PUBLIC_ADDRESS_SIZE 6
+#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
+
+#define MGMT_RANGE_NONE 0X00
+#define MGMT_RANGE_RSSI 0X01
+#define MGMT_RANGE_PATHLOSS 0X02
+
+struct mgmt_uuid_filter {
+ __u8 range_method;
+ __s8 pathloss;
+ __s8 rssi;
+ __u8 uuid[16];
+} __packed;
+
+struct mgmt_cp_start_service_discovery {
+ __u8 type;
+ __le16 filter_count;
+ struct mgmt_uuid_filter filter[0];
+} __packed;
+#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
+
+#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
+struct mgmt_cp_stop_service_discovery {
+ __u8 type;
+} __packed;
+#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cb05d7f..af6bf39 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2580,6 +2580,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
cancel_delayed_work(&hdev->service_cache);
cancel_delayed_work_sync(&hdev->le_scan_disable);
+ cancel_delayed_work_sync(&hdev->le_scan_restart);
if (test_bit(HCI_MGMT, &hdev->dev_flags))
cancel_delayed_work_sync(&hdev->rpa_expired);
@@ -3846,6 +3847,7 @@ static void le_scan_disable_work(struct work_struct *work)
BT_DBG("%s", hdev->name);
+ clean_up_service_discovery(hdev);
hci_req_init(&req, hdev);
hci_req_add_le_scan_disable(&req);
@@ -3855,6 +3857,41 @@ static void le_scan_disable_work(struct work_struct *work)
BT_ERR("Disable LE scanning request failed: err %d", err);
}
+static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
+{
+ hdev->scan_restart = false;
+
+ if (status) {
+ BT_ERR("Failed to restart LE scanning: status %d", status);
+ return;
+ }
+}
+
+static void le_scan_restart_work(struct work_struct *work)
+{
+ struct hci_dev *hdev = container_of(work, struct hci_dev,
+ le_scan_restart.work);
+ struct hci_request req;
+ struct hci_cp_le_set_scan_enable cp;
+ int err;
+
+ BT_DBG("%s", hdev->name);
+
+ hci_req_init(&req, hdev);
+
+ hci_req_add_le_scan_disable(&req);
+
+ memset(&cp, 0, sizeof(cp));
+ cp.enable = LE_SCAN_ENABLE;
+ hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+
+ hdev->scan_restart = true;
+
+ err = hci_req_run(&req, le_scan_restart_work_complete);
+ if (err)
+ BT_ERR("Disable LE scanning request failed: err %d", err);
+}
+
static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
{
struct hci_dev *hdev = req->hdev;
@@ -4010,6 +4047,10 @@ struct hci_dev *hci_alloc_dev(void)
mutex_init(&hdev->lock);
mutex_init(&hdev->req_lock);
+ INIT_LIST_HEAD(&hdev->discov_uuid_filter);
+ hdev->service_filter_enabled = false;
+ hdev->scan_restart = false;
+
INIT_LIST_HEAD(&hdev->mgmt_pending);
INIT_LIST_HEAD(&hdev->blacklist);
INIT_LIST_HEAD(&hdev->whitelist);
@@ -4032,6 +4073,7 @@ struct hci_dev *hci_alloc_dev(void)
INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
+ INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
skb_queue_head_init(&hdev->rx_q);
skb_queue_head_init(&hdev->cmd_q);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 9629153..b372b02 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1155,7 +1155,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
/* Cancel this timer so that we don't try to disable scanning
* when it's already disabled.
*/
- cancel_delayed_work(&hdev->le_scan_disable);
+ if(!hdev->scan_restart)
+ cancel_delayed_work(&hdev->le_scan_disable);
clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 9c4daf7..08d48a7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -93,6 +93,8 @@ static const u16 mgmt_commands[] = {
MGMT_OP_READ_CONFIG_INFO,
MGMT_OP_SET_EXTERNAL_CONFIG,
MGMT_OP_SET_PUBLIC_ADDRESS,
+ MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_OP_STOP_SERVICE_DISCOVERY
};
static const u16 mgmt_events[] = {
@@ -1258,6 +1260,26 @@ static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
}
}
+void clean_up_service_discovery(struct hci_dev *hdev)
+{
+ if (hdev->service_filter_enabled) {
+ struct uuid_filter *filterptr = NULL;
+ struct uuid_filter *tmp = NULL;
+
+ hdev->service_filter_enabled = false;
+ cancel_delayed_work_sync(&hdev->le_scan_restart);
+
+ if(!list_empty(&hdev->discov_uuid_filter)) {
+ list_for_each_entry_safe(filterptr, tmp,
+ &hdev->discov_uuid_filter, list)
+ {
+ __list_del_entry(&(filterptr->list));
+ kfree(filterptr);
+ }
+ }
+ }
+}
+
static bool hci_stop_discovery(struct hci_request *req)
{
struct hci_dev *hdev = req->hdev;
@@ -1270,6 +1292,7 @@ static bool hci_stop_discovery(struct hci_request *req)
hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
} else {
cancel_delayed_work(&hdev->le_scan_disable);
+ clean_up_service_discovery(hdev);
hci_req_add_le_scan_disable(req);
}
@@ -5641,6 +5664,345 @@ unlock:
return err;
}
+static int mgmt_start_service_discovery_failed(struct hci_dev *hdev, u8 status)
+{
+ struct pending_cmd *cmd;
+ u8 type;
+ int err;
+
+ hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+
+ cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY, hdev);
+ if (!cmd)
+ return -ENOENT;
+
+ type = hdev->discovery.type;
+
+ err = cmd_complete(cmd->sk, hdev->id, cmd->opcode, mgmt_status(status),
+ &type, sizeof(type));
+ mgmt_pending_remove(cmd);
+
+ return err;
+}
+
+static void start_service_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ unsigned long timeout = 0;
+
+ BT_DBG("status %d", status);
+
+ if (status) {
+ hci_dev_lock(hdev);
+ mgmt_start_service_discovery_failed(hdev, status);
+ hci_dev_unlock(hdev);
+ return;
+ }
+
+ hci_dev_lock(hdev);
+ hci_discovery_set_state(hdev, DISCOVERY_FINDING);
+ hci_dev_unlock(hdev);
+
+ switch (hdev->discovery.type) {
+ case DISCOV_TYPE_LE:
+ timeout = msecs_to_jiffies(DISCOV_LE_TIMEOUT);
+ break;
+
+ case DISCOV_TYPE_INTERLEAVED:
+ timeout = msecs_to_jiffies(hdev->discov_interleaved_timeout);
+ break;
+
+ case DISCOV_TYPE_BREDR:
+ break;
+
+ default:
+ BT_ERR("Invalid discovery type %d", hdev->discovery.type);
+ }
+
+ if (!timeout)
+ return;
+
+ queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable, timeout);
+}
+
+static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len)
+{
+ struct mgmt_cp_start_service_discovery *cp = data;
+ struct pending_cmd *cmd;
+ struct hci_cp_le_set_scan_param param_cp;
+ struct hci_cp_le_set_scan_enable enable_cp;
+ struct hci_cp_inquiry inq_cp;
+ struct hci_request req;
+ /* General inquiry access code (GIAC) */
+ u8 lap[3] = { 0x33, 0x8b, 0x9e };
+ u8 status, own_addr_type;
+ int i, err;
+ u16 filter_count, expected_len;
+
+ BT_DBG("%s", hdev->name);
+
+ filter_count = __le16_to_cpu(cp->filter_count);
+
+ expected_len = sizeof(*cp) + filter_count * sizeof(struct mgmt_uuid_filter);
+ if (expected_len != len) {
+ BT_ERR("start_service_discovery: expected %u bytes, got %u bytes",
+ expected_len, len);
+
+ return cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_INVALID_PARAMS);
+ }
+
+ hci_dev_lock(hdev);
+
+ if (!hdev_is_powered(hdev)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_NOT_POWERED);
+ goto failed;
+ }
+
+ if (test_bit(HCI_PERIODIC_INQ, &hdev->dev_flags)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_BUSY);
+ goto failed;
+ }
+
+ if (hdev->discovery.state != DISCOVERY_STOPPED) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_BUSY);
+ goto failed;
+ }
+
+ cmd = mgmt_pending_add(sk, MGMT_OP_START_SERVICE_DISCOVERY, hdev, NULL, 0);
+ if (!cmd) {
+ err = -ENOMEM;
+ goto failed;
+ }
+
+ hdev->discovery.type = cp->type;
+
+ hci_req_init(&req, hdev);
+
+ switch (hdev->discovery.type) {
+ case DISCOV_TYPE_BREDR:
+ status = mgmt_bredr_support(hdev);
+ if (status) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ status);
+ mgmt_pending_remove(cmd);
+ goto failed;
+ }
+
+ if (test_bit(HCI_INQUIRY, &hdev->flags)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_BUSY);
+ mgmt_pending_remove(cmd);
+ goto failed;
+ }
+
+ hci_inquiry_cache_flush(hdev);
+
+ memset(&inq_cp, 0, sizeof(inq_cp));
+ memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
+ inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;
+ hci_req_add(&req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
+ break;
+
+ case DISCOV_TYPE_LE:
+ case DISCOV_TYPE_INTERLEAVED:
+ status = mgmt_le_support(hdev);
+ if (status) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ status);
+ mgmt_pending_remove(cmd);
+ goto failed;
+ }
+
+ if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
+ !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_NOT_SUPPORTED);
+ mgmt_pending_remove(cmd);
+ goto failed;
+ }
+
+ if (test_bit(HCI_LE_ADV, &hdev->dev_flags)) {
+ /* Don't let discovery abort an outgoing
+ * connection attempt that's using directed
+ * advertising.
+ */
+ if (hci_conn_hash_lookup_state(hdev, LE_LINK,
+ BT_CONNECT)) {
+ err = cmd_status(sk, hdev->id,
+ MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_REJECTED);
+ mgmt_pending_remove(cmd);
+ goto failed;
+ }
+
+ disable_advertising(&req);
+ }
+
+ /* If controller is scanning, it means the background scanning
+ * is running. Thus, we should temporarily stop it in order to
+ * set the discovery scanning parameters.
+ */
+ if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+ hci_req_add_le_scan_disable(&req);
+
+ memset(¶m_cp, 0, sizeof(param_cp));
+
+ /* All active scans will be done with either a resolvable
+ * private address (when privacy feature has been enabled)
+ * or unresolvable private address.
+ */
+ err = hci_update_random_address(&req, true, &own_addr_type);
+ if (err < 0) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_FAILED);
+ mgmt_pending_remove(cmd);
+ goto failed;
+ }
+
+ param_cp.type = LE_SCAN_ACTIVE;
+ param_cp.interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
+ param_cp.window = cpu_to_le16(DISCOV_LE_SCAN_WIN);
+ param_cp.own_address_type = own_addr_type;
+ hci_req_add(&req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
+ ¶m_cp);
+
+ memset(&enable_cp, 0, sizeof(enable_cp));
+ enable_cp.enable = LE_SCAN_ENABLE;
+ enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
+ hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
+ &enable_cp);
+ break;
+
+ default:
+ err = cmd_status(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_INVALID_PARAMS);
+ mgmt_pending_remove(cmd);
+ goto failed;
+ }
+
+ for (i = 0; i < filter_count; i++) {
+ struct mgmt_uuid_filter *key = &cp->filter[i];
+ struct uuid_filter *tmp_uuid;
+
+ tmp_uuid = kmalloc(sizeof(struct uuid_filter), GFP_KERNEL);
+ if (!tmp_uuid)
+ return -ENOMEM;
+
+ memcpy(tmp_uuid->uuid, key->uuid, 16);
+ tmp_uuid->range_method = key->range_method;
+ tmp_uuid->pathloss = key->pathloss;
+ tmp_uuid->rssi = key->rssi;
+ INIT_LIST_HEAD( &tmp_uuid->list ) ;
+
+ list_add(&tmp_uuid->list, &hdev->discov_uuid_filter);
+ }
+ hdev->service_filter_enabled = true;
+
+ err = hci_req_run(&req, start_service_discovery_complete);
+ if (err < 0)
+ mgmt_pending_remove(cmd);
+ else
+ hci_discovery_set_state(hdev, DISCOVERY_STARTING);
+
+failed:
+ hci_dev_unlock(hdev);
+ return err;
+}
+
+static int mgmt_stop_service_discovery_failed(struct hci_dev *hdev, u8 status)
+{
+ struct pending_cmd *cmd;
+ int err;
+
+ cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY, hdev);
+ if (!cmd)
+ return -ENOENT;
+
+ hdev->service_filter_enabled = false;
+ err = cmd_complete(cmd->sk, hdev->id, cmd->opcode, mgmt_status(status),
+ &hdev->discovery.type, sizeof(hdev->discovery.type));
+ mgmt_pending_remove(cmd);
+
+ return err;
+}
+
+static void stop_service_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ BT_DBG("status %d", status);
+
+ hci_dev_lock(hdev);
+
+ if (status) {
+ mgmt_stop_service_discovery_failed(hdev, status);
+ goto unlock;
+ }
+
+ hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+
+unlock:
+ hci_dev_unlock(hdev);
+}
+
+static int stop_service_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
+ u16 len)
+{
+ struct mgmt_cp_stop_service_discovery *mgmt_cp = data;
+ struct pending_cmd *cmd;
+ struct hci_request req;
+ int err;
+
+ BT_DBG("%s", hdev->name);
+
+ hci_dev_lock(hdev);
+
+ if (!hci_discovery_active(hdev)) {
+ err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_SERVICE_DISCOVERY,
+ MGMT_STATUS_REJECTED, &mgmt_cp->type,
+ sizeof(mgmt_cp->type));
+ goto unlock;
+ }
+
+ if (hdev->discovery.type != mgmt_cp->type) {
+ err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_SERVICE_DISCOVERY,
+ MGMT_STATUS_INVALID_PARAMS, &mgmt_cp->type,
+ sizeof(mgmt_cp->type));
+ goto unlock;
+ }
+
+ cmd = mgmt_pending_add(sk, MGMT_OP_STOP_SERVICE_DISCOVERY, hdev, NULL, 0);
+ if (!cmd) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ hci_req_init(&req, hdev);
+
+ hci_stop_discovery(&req);
+
+ err = hci_req_run(&req, stop_service_discovery_complete);
+ if (!err) {
+ hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
+ goto unlock;
+ }
+
+ mgmt_pending_remove(cmd);
+
+ /* If no HCI commands were sent we're done */
+ if (err == -ENODATA) {
+ err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_SERVICE_DISCOVERY, 0,
+ &mgmt_cp->type, sizeof(mgmt_cp->type));
+ hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+ }
+
+unlock:
+ hci_dev_unlock(hdev);
+ return err;
+}
+
static const struct mgmt_handler {
int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
u16 data_len);
@@ -5705,6 +6067,8 @@ static const struct mgmt_handler {
{ read_config_info, false, MGMT_READ_CONFIG_INFO_SIZE },
{ set_external_config, false, MGMT_SET_EXTERNAL_CONFIG_SIZE },
{ set_public_address, false, MGMT_SET_PUBLIC_ADDRESS_SIZE },
+ { start_service_discovery,true, MGMT_START_SERVICE_DISCOVERY_SIZE },
+ { stop_service_discovery, false, MGMT_STOP_SERVICE_DISCOVERY_SIZE },
};
int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
@@ -6774,6 +7138,85 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
mgmt_pending_remove(cmd);
}
+struct parsed_uuid {
+ struct list_head list;
+ u8 uuid[16];
+};
+
+//static u8 baseUUID[16] = {0x00,0x00,0x00,0x00,/*-*/0x00,0x00,/*-*/0x10,0x00,/*-*/ 0x80,0x00,/*-*/0x00,0x80,0x5f,0x9b,0x34,0xfb};
+static u8 reverse_base_uuid[16] = {0xfb,0x34,0x9b,0x5f,0x80,0x00,0x00,0x80,0x00,0x10,0x00,0x00,0x00,0x00,0x00,0x00};
+
+static void add_uuid_to_list(struct list_head *uuids, u8 *uuid) {
+ struct parsed_uuid *tmp_uuid;
+ tmp_uuid = kmalloc(sizeof(struct parsed_uuid), GFP_KERNEL);
+ if (tmp_uuid == NULL)
+ return; //TODO: how to handle if there's no memory?
+
+ memcpy(tmp_uuid->uuid, uuid, 16);
+ INIT_LIST_HEAD(&tmp_uuid->list);
+ list_add(&tmp_uuid->list, uuids);
+}
+
+static void eir_parse(u8 *eir, u8 eir_len, struct list_head *uuids,
+ u8 *txpwr_present, s8 *txpwr)
+{
+ size_t offset;
+ u8 uuid[16];
+ int loop;
+
+ offset = 0;
+ while (offset < eir_len) {
+ uint8_t field_len = eir[0];
+
+ /* Check for the end of EIR */
+ if (field_len == 0)
+ break;
+
+ if (offset + field_len > eir_len)
+ goto failed;
+
+ switch (eir[1]) {
+ case EIR_TX_POWER:
+ *txpwr_present = 1;
+ *txpwr = eir[2];
+ break;
+ case EIR_UUID16_ALL:
+ case EIR_UUID16_SOME:
+ for(loop = 0; loop+3<=field_len;loop+=2) {
+ memcpy(uuid, reverse_base_uuid, 16);
+ uuid[13] = eir[loop + 3];
+ uuid[12] = eir[loop + 2];
+ add_uuid_to_list(uuids, uuid);
+ }
+ break;
+ case EIR_UUID32_ALL:
+ case EIR_UUID32_SOME:
+ for(loop = 0; loop+5<=field_len;loop+=4) {
+ memcpy(uuid, reverse_base_uuid, 16);
+ uuid[15] = eir[loop + 5];
+ uuid[14] = eir[loop + 4];
+ uuid[13] = eir[loop + 3];
+ uuid[12] = eir[loop + 2];
+ add_uuid_to_list(uuids, uuid);
+ }
+ break;
+ case EIR_UUID128_ALL:
+ case EIR_UUID128_SOME:
+ for(loop = 0; loop+17<=field_len;loop+=4) {
+ memcpy(uuid, eir + loop + 2, 16);
+ add_uuid_to_list(uuids, uuid);
+ }
+ break;
+ }
+
+ offset += field_len + 1;
+ eir += field_len + 1;
+ }
+
+failed:
+ offset = 8;
+}
+
void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
@@ -6819,7 +7262,52 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
- mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
+ if(hdev->service_filter_enabled) {
+ LIST_HEAD(uuids);
+ s8 txpwr;
+ u8 txpwr_present = 0;
+ struct uuid_filter *filterptr = NULL;
+ struct parsed_uuid *uuidptr = NULL;
+ struct parsed_uuid *tmp = NULL;
+ bool service_match = false, full_match = false;
+
+ eir_parse(eir, eir_len, &uuids, &txpwr_present, &txpwr);
+ if(!list_empty(&uuids)) {
+ list_for_each_entry_safe(uuidptr, tmp, &uuids, list)
+ {
+ list_for_each_entry(filterptr, &hdev->discov_uuid_filter, list)
+ {
+ if(memcmp(filterptr->uuid, uuidptr->uuid, 16) == 0) {
+ service_match = true;
+
+ if( filterptr->range_method == MGMT_RANGE_NONE) {
+ full_match = true;
+ } else if ( (filterptr->range_method & MGMT_RANGE_PATHLOSS)
+ && txpwr_present
+ && (txpwr - rssi) <= filterptr->pathloss ) {
+ full_match = true;
+ } else if( (filterptr->range_method & MGMT_RANGE_RSSI)
+ && rssi >= filterptr->rssi ) {
+ full_match = true;
+ }
+
+ }
+ }
+ __list_del_entry(&(uuidptr->list));
+ kfree(uuidptr);
+ }
+ }
+
+ if(full_match) {
+ mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
+ }
+ if(service_match) {
+ queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
+ DISCOV_LE_RESTART_DELAY);
+ }
+ } else {
+ mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
+ }
}
void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
@@ -6852,10 +7340,15 @@ void mgmt_discovering(struct hci_dev *hdev, u8 discovering)
BT_DBG("%s discovering %u", hdev->name, discovering);
- if (discovering)
+ if (discovering) {
cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
- else
+ if(cmd == NULL)
+ cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY, hdev);
+ } else {
cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
+ if(cmd == NULL)
+ cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY, hdev);
+ }
if (cmd != NULL) {
u8 type = hdev->discovery.type;
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH] Add new method - service discovery to mgmt
From: Jakub Pawlowski @ 2014-10-23 17:15 UTC (permalink / raw)
To: linux-bluetooth
This path adds start service discovery and stop service discovery methods.
Path for BlueZ that uses those new methods will follow.
^ permalink raw reply
* [PATCH BlueZ 5/5] Add unit test for gattrib
From: Michael Janssen @ 2014-10-23 16:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414082009-40903-1-git-send-email-jamuraa@chromium.org>
This will ensure that the API behavior of gattrib is preserved.
---
.gitignore | 1 +
Makefile.am | 7 +
unit/test-gattrib.c | 584 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 592 insertions(+)
create mode 100644 unit/test-gattrib.c
diff --git a/.gitignore b/.gitignore
index 97daa9f..164cc97 100644
--- a/.gitignore
+++ b/.gitignore
@@ -121,6 +121,7 @@ unit/test-avdtp
unit/test-avctp
unit/test-avrcp
unit/test-gatt
+unit/test-gattrib
unit/test-*.log
unit/test-*.trs
diff --git a/Makefile.am b/Makefile.am
index 2dfea28..3fddb80 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -377,6 +377,13 @@ unit_test_gatt_SOURCES = unit/test-gatt.c
unit_test_gatt_LDADD = src/libshared-glib.la \
lib/libbluetooth-internal.la @GLIB_LIBS@
+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
+
if MAINTAINER_MODE
noinst_PROGRAMS += $(unit_tests)
endif
diff --git a/unit/test-gattrib.c b/unit/test-gattrib.c
new file mode 100644
index 0000000..3e14ac1
--- /dev/null
+++ b/unit/test-gattrib.c
@@ -0,0 +1,584 @@
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <inttypes.h>
+#include <string.h>
+#include <fcntl.h>
+#include <sys/socket.h>
+
+#include <glib.h>
+
+#include "src/shared/util.h"
+#include "attrib/gattrib.h"
+#include "src/log.h"
+
+#define DEFAULT_MTU 23
+
+struct test_pdu {
+ bool valid;
+ const uint8_t *data;
+ size_t size;
+};
+
+struct test_data {
+ char *test_name;
+ struct test_pdu *pdu_list;
+};
+
+struct context {
+ GMainLoop *main_loop;
+ guint source;
+ guint process;
+ int fd;
+ unsigned int pdu_offset;
+ const struct test_data *data;
+};
+
+#define data(args...) ((const unsigned char[]) { args })
+
+#define raw_pdu(args...) \
+ { \
+ .valid = true, \
+ .data = data(args), \
+ .size = sizeof(data(args)), \
+ }
+
+#define define_test(name, function, args...) \
+ do { \
+ const struct test_pdu pdus[] = { \
+ args, { } \
+ }; \
+ static struct test_data data; \
+ data.test_name = g_strdup(name); \
+ data.pdu_list = g_malloc(sizeof(pdus)); \
+ memcpy(data.pdu_list, pdus, sizeof(pdus)); \
+ g_test_add_data_func(name, &data, function); \
+ } while (0)
+
+
+static void test_debug(const char *str, void *user_data)
+{
+ const char *prefix = user_data;
+
+ g_print("%s%s\n", prefix, str);
+}
+
+/*
+static void test_free(gconstpointer user_data)
+{
+ const struct test_data *data = user_data;
+
+ g_free(data->test_name);
+ g_free(data->pdu_list);
+}
+
+static gboolean context_quit(gpointer user_data)
+{
+ struct context *context = user_data;
+
+ if (context->process > 0)
+ g_source_remove(context->process);
+
+ g_main_loop_quit(context->main_loop);
+
+ return FALSE;
+}
+
+static gboolean send_pdu(gpointer user_data)
+{
+ struct context *context = user_data;
+ const struct test_pdu *pdu;
+ ssize_t len;
+
+ pdu = &context->data->pdu_list[context->pdu_offset++];
+
+ len = write(context->fd, pdu->data, pdu->size);
+
+ if (g_test_verbose())
+ util_hexdump('<', pdu->data, len, test_debug, "GATT: ");
+
+ g_assert_cmpint(len, ==, pdu->size);
+
+ context->process = 0;
+ return FALSE;
+}
+
+static void context_process(struct context *context)
+{
+ if (!context->data->pdu_list[context->pdu_offset].valid) {
+ context_quit(context);
+ return;
+ }
+
+ context->process = g_idle_add(send_pdu, context);
+}
+
+static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
+ gpointer user_data)
+{
+ struct context *context = user_data;
+ const struct test_pdu *pdu;
+ unsigned char buf[512];
+ ssize_t len;
+ int fd;
+
+ pdu = &context->data->pdu_list[context->pdu_offset++];
+
+ if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
+ context->source = 0;
+ g_print("%s: cond %x\n", __func__, cond);
+ return FALSE;
+ }
+
+ fd = g_io_channel_unix_get_fd(channel);
+
+ len = read(fd, buf, sizeof(buf));
+
+ g_assert(len > 0);
+
+ if (g_test_verbose())
+ util_hexdump('>', buf, len, test_debug, "GATT: ");
+
+ g_assert_cmpint(len, ==, pdu->size);
+
+ g_assert(memcmp(buf, pdu->data, pdu->size) == 0);
+
+ context_process(context);
+
+ return TRUE;
+}
+
+static void gatt_debug(const char *str, void *user_data)
+{
+ const char *prefix = user_data;
+
+ g_print("%s%s\n", prefix, str);
+}
+
+static struct context *create_context(uint16_t mtu, gconstpointer data)
+{
+ struct context *context = g_new0(struct context, 1);
+ GIOChannel *channel;
+ int err, sv[2];
+ struct bt_att *att;
+
+ context->main_loop = g_main_loop_new(NULL, FALSE);
+ g_assert(context->main_loop);
+
+ err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+ g_assert(err == 0);
+
+ att = bt_att_new(sv[0]);
+ g_assert(att);
+
+ context->client = bt_gatt_client_new(att, mtu);
+ g_assert(context->client);
+
+ if (g_test_verbose())
+ bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
+ NULL);
+
+ channel = g_io_channel_unix_new(sv[1]);
+
+ g_io_channel_set_close_on_unref(channel, TRUE);
+ g_io_channel_set_encoding(channel, NULL, NULL);
+ g_io_channel_set_buffered(channel, FALSE);
+
+ context->source = g_io_add_watch(channel,
+ G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ test_handler, context);
+ g_assert(context->source > 0);
+
+ g_io_channel_unref(channel);
+ bt_att_unref(att);
+
+
+ return context;
+}
+
+static void destroy_context(struct context *context)
+{
+ if (context->source > 0)
+ g_source_remove(context->source);
+
+ bt_gatt_client_unref(context->client);
+
+ g_main_loop_unref(context->main_loop);
+
+ test_free(context->data);
+ g_free(context);
+}
+
+static void execute_context(struct context *context)
+{
+ g_main_loop_run(context->main_loop);
+
+ destroy_context(context);
+}
+
+static void test_client(gconstpointer data)
+{
+ struct context *context = create_context(512, data);
+
+ execute_context(context);
+}
+
+*/
+
+static void destroy_canary_increment(gpointer data) {
+ int *canary = data;
+ (*canary)++;
+}
+
+static void test_refcounts(void)
+{
+ int err, sv[2];
+ GAttrib *att, *extra_ref;
+ GIOChannel *channel;
+ int destroy_canary;
+
+ err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+ g_assert(err == 0);
+
+ channel = g_io_channel_unix_new(sv[1]);
+
+ g_io_channel_set_close_on_unref(channel, TRUE);
+
+ att = g_attrib_new(channel, DEFAULT_MTU);
+
+ destroy_canary = 0;
+
+ g_attrib_set_destroy_function(att, destroy_canary_increment, &destroy_canary);
+
+ extra_ref = g_attrib_ref(att);
+
+ g_assert(extra_ref == att);
+
+ g_attrib_unref(extra_ref);
+
+ g_assert(destroy_canary == 0);
+
+ g_attrib_unref(att);
+
+ g_assert(destroy_canary == 1);
+
+}
+
+static void test_get_channel(void) {
+
+ int err, sv[2];
+ GAttrib *att;
+ GIOChannel *channel, *result;
+
+ err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+ g_assert(err == 0);
+
+ channel = g_io_channel_unix_new(sv[1]);
+ g_assert(channel != NULL);
+
+ g_io_channel_set_close_on_unref(channel, TRUE);
+
+ att = g_attrib_new(channel, DEFAULT_MTU);
+ g_assert(att != 0);
+
+ result = g_attrib_get_channel(att);
+
+ g_assert(result == channel);
+
+ g_attrib_unref(att);
+}
+
+struct challenge_response {
+ const uint8_t *expect;
+ const size_t expect_len;
+ const uint8_t *respond;
+ const size_t respond_len;
+ bool received;
+};
+
+static gboolean test_client(GIOChannel *channel, GIOCondition cond, gpointer data) {
+ struct challenge_response *cr = data;
+ int fd;
+ uint8_t buf[256];
+ ssize_t len;
+
+ if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
+ g_print("%s: no good cond %x\n", __func__, cond);
+ return FALSE;
+ }
+
+ fd = g_io_channel_unix_get_fd(channel);
+
+ len = read(fd, buf, sizeof(buf));
+
+ g_assert(len > 0);
+ g_assert_cmpint(len, ==, cr->expect_len);
+
+ if (g_test_verbose())
+ util_hexdump('>', buf, len, test_debug, "test_client: ");
+
+ cr->received = true;
+
+ if (cr->respond != NULL) {
+ if (g_test_verbose())
+ util_hexdump('<', cr->respond, cr->respond_len, test_debug, "test_client: ");
+ len = write(fd, cr->respond, cr->respond_len);
+
+ g_assert_cmpint(len, ==, cr->respond_len);
+ }
+
+ return TRUE;
+}
+
+struct result_data {
+ guint8 status;
+ const guint8 *pdu;
+ guint16 len;
+ bool done;
+};
+
+static void result_canary(guint8 status, const guint8 *pdu, guint16 len, gpointer data) {
+ struct result_data *result = data;
+ result->status = status;
+ result->pdu = pdu;
+ result->len = len;
+
+ if (g_test_verbose())
+ util_hexdump('<', pdu, len, test_debug, "result_canary: ");
+
+ result->done = true;
+
+}
+
+static void test_send(void) {
+ int err, sv[2];
+ GIOChannel *att_io, *server_io;
+ GAttrib *att;
+ GMainLoop *main_loop;
+ GMainContext *context;
+ struct result_data results;
+
+ main_loop = g_main_loop_new(NULL, FALSE);
+
+ err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+ g_assert(err == 0);
+
+ att_io = g_io_channel_unix_new(sv[0]);
+ g_assert(att_io != 0);
+
+ g_io_channel_set_close_on_unref(att_io, TRUE);
+
+ att = g_attrib_new(att_io, DEFAULT_MTU);
+ g_assert(att != 0);
+
+ server_io = g_io_channel_unix_new(sv[1]);
+
+ g_io_channel_set_close_on_unref(server_io, TRUE);
+ g_io_channel_set_encoding(server_io, NULL, NULL);
+ g_io_channel_set_buffered(server_io, FALSE);
+
+ do {
+ struct challenge_response data = {
+ .expect = ((unsigned char[]) { 0x02, 0x00, 0x02 }),
+ .expect_len = 3,
+ .respond = ((unsigned char[]) { 0x01, 0x02, 0x03, 0x04 }),
+ .respond_len = 4,
+ .received = false,
+ };
+
+ g_io_add_watch(server_io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ test_client, &data);
+
+ results.done = false;
+
+ g_attrib_send(att, 0, data.expect, data.expect_len, result_canary,
+ (gpointer) &results, NULL);
+
+ // Spin the main loop until we are ready.
+ context = g_main_loop_get_context(main_loop);
+ do {
+ g_main_context_iteration(context, TRUE);
+ } while (!results.done);
+ } while (0);
+
+ g_io_channel_unref(server_io);
+
+ g_attrib_unref(att);
+}
+
+static void test_cancel(void) {
+ int err, sv[2];
+ GIOChannel *att_io, *server_io;
+ GAttrib *att;
+ guint event_id;
+ GMainLoop *main_loop;
+ GMainContext *context;
+ gboolean canceled;
+ struct result_data results;
+
+ main_loop = g_main_loop_new(NULL, FALSE);
+
+ err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+ g_assert(err == 0);
+
+ att_io = g_io_channel_unix_new(sv[0]);
+ g_assert(att_io != 0);
+
+ g_io_channel_set_close_on_unref(att_io, TRUE);
+
+ att = g_attrib_new(att_io, DEFAULT_MTU);
+ g_assert(att != 0);
+
+ server_io = g_io_channel_unix_new(sv[1]);
+
+ do {
+ struct challenge_response data = {
+ .expect = ((unsigned char[]) { 0x02, 0x00, 0x02 }),
+ .expect_len = 3,
+ .respond = ((unsigned char[]) { 0x01, 0x02, 0x03, 0x04 }),
+ .respond_len = 4,
+ .received = false,
+ };
+
+ g_io_add_watch(server_io,
+ G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ test_client, &data);
+
+ results.done = false;
+ event_id = g_attrib_send(att, 0, data.expect,
+ data.expect_len, result_canary,
+ &results, NULL);
+
+ /* Spin the main loop until we receive the first message */
+ context = g_main_loop_get_context(main_loop);
+ do {
+ g_main_context_iteration(context, FALSE);
+ } while (!data.received);
+
+ canceled = g_attrib_cancel(att, event_id);
+ g_assert(canceled);
+
+ /*
+ * Spin the main loop until all events are processed,
+ * no result should be called */
+ do {
+ g_main_context_iteration(context, TRUE);
+ } while (g_main_context_pending(context));
+
+ g_assert(!results.done);
+
+
+ results.done = false;
+ data.received = false;
+ event_id = g_attrib_send(att, 0, data.expect, data.expect_len, result_canary,
+ &results, NULL);
+
+ canceled = g_attrib_cancel(att, event_id);
+ g_assert(canceled);
+
+ /*
+ * Spin the main loop until all events are processed,
+ * the message should never be delivered.
+ */
+ do {
+ g_main_context_iteration(context, TRUE);
+ } while (g_main_context_pending(context));
+
+ g_assert(!data.received);
+ g_assert(!results.done);
+
+ /* Invalid ID */
+ canceled = g_attrib_cancel(att, 42);
+ g_assert(!canceled);
+ } while (0);
+
+ g_io_channel_unref(server_io);
+
+ g_attrib_unref(att);
+}
+
+static void test_register(void) {
+ /* TODO */
+}
+
+static void test_buffers(void) {
+ int err, sv[2];
+ GIOChannel *att_io;
+ GAttrib *att;
+ size_t buflen;
+ uint8_t *buf;
+ gboolean success;
+
+ err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+ g_assert(err == 0);
+
+ att_io = g_io_channel_unix_new(sv[0]);
+ g_assert(att_io != 0);
+
+ g_io_channel_set_close_on_unref(att_io, TRUE);
+
+ att = g_attrib_new(att_io, DEFAULT_MTU);
+ g_assert(att != 0);
+
+ buf = g_attrib_get_buffer(att, &buflen);
+ g_assert(buf != 0);
+ g_assert_cmpint(buflen, ==, DEFAULT_MTU);
+
+ success = g_attrib_set_mtu(att, 5);
+ g_assert(!success);
+
+ success = g_attrib_set_mtu(att, 255);
+ g_assert(success);
+
+ buf = g_attrib_get_buffer(att, &buflen);
+ g_assert(buf != 0);
+ g_assert_cmpint(buflen, ==, 255);
+
+ g_attrib_unref(att);
+}
+
+int main(int argc, char *argv[])
+{
+ g_test_init(&argc, &argv, NULL);
+
+ __btd_log_init("*", 0);
+
+ /*
+ * Test the GAttrib API behavior
+ */
+ g_test_add_func("/gattrib/refcount", test_refcounts);
+ g_test_add_func("/gattrib/get_channel", test_get_channel);
+ g_test_add_func("/gattrib/send", test_send);
+ g_test_add_func("/gattrib/cancel", test_cancel);
+ g_test_add_func("/gattrib/register", test_register);
+ g_test_add_func("/gattrib/buffers", test_buffers);
+
+ return g_test_run();
+}
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ 4/5] attrib: remove g_attrib_is_encrypted
From: Michael Janssen @ 2014-10-23 16:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414082009-40903-1-git-send-email-jamuraa@chromium.org>
This function is only used in one place and encryption is the
responsibility of the channel, not the attribute.
---
attrib/gattrib.c | 12 ------------
attrib/gattrib.h | 2 --
src/attrib-server.c | 9 +++++++--
3 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index a6511a2..a65d1ca 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -690,18 +690,6 @@ static int event_cmp_by_id(gconstpointer a, gconstpointer b)
return evt->id - id;
}
-gboolean g_attrib_is_encrypted(GAttrib *attrib)
-{
- BtIOSecLevel sec_level;
-
- if (!bt_io_get(attrib->io, NULL,
- BT_IO_OPT_SEC_LEVEL, &sec_level,
- BT_IO_OPT_INVALID))
- return FALSE;
-
- return sec_level > BT_IO_SEC_LOW;
-}
-
gboolean g_attrib_unregister(GAttrib *attrib, guint id)
{
struct event *evt;
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 99b8b37..1557b99 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -62,8 +62,6 @@ guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle,
GAttribNotifyFunc func, gpointer user_data,
GDestroyNotify notify);
-gboolean g_attrib_is_encrypted(GAttrib *attrib);
-
uint8_t *g_attrib_get_buffer(GAttrib *attrib, size_t *len);
gboolean g_attrib_set_mtu(GAttrib *attrib, int mtu);
diff --git a/src/attrib-server.c b/src/attrib-server.c
index e65fff2..1ccfc65 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -375,12 +375,17 @@ static struct attribute *attrib_db_add_new(struct gatt_server *server,
static uint8_t att_check_reqs(struct gatt_channel *channel, uint8_t opcode,
int reqs)
{
+ BtIOSecLevel sec_level;
+ GIOChannel *io = g_attrib_get_channel(channel->attrib);
+
/* FIXME: currently, it is assumed an encrypted link is enough for
* authentication. This will allow to enable the SMP negotiation once
* it is on upstream kernel. High security level should be mapped
* to authentication and medium to encryption permission. */
- if (!channel->encrypted)
- channel->encrypted = g_attrib_is_encrypted(channel->attrib);
+ if (!channel->encrypted &&
+ bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
+ BT_IO_OPT_INVALID))
+ channel->encrypted = sec_level > BT_IO_SEC_LOW;
if (reqs == ATT_AUTHENTICATION && !channel->encrypted)
return ATT_ECODE_AUTHENTICATION;
else if (reqs == ATT_AUTHORIZATION)
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ 3/5] attrib: Add mtu argument to g_attrib_new
From: Michael Janssen @ 2014-10-23 16:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414082009-40903-1-git-send-email-jamuraa@chromium.org>
Instead of using the default MTU, use one passed
in by the user, and detect it from the channel when
it is created.
---
attrib/gattrib.c | 10 +++-------
attrib/gattrib.h | 2 +-
attrib/gatttool.c | 17 ++++++++++++++++-
attrib/interactive.c | 17 ++++++++++++++++-
src/device.c | 11 ++++++++++-
5 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index fa41ade..a6511a2 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -473,11 +473,9 @@ done:
return TRUE;
}
-GAttrib *g_attrib_new(GIOChannel *io)
+GAttrib *g_attrib_new(GIOChannel *io, guint16 mtu)
{
struct _GAttrib *attrib;
- uint16_t att_mtu;
- uint16_t cid;
g_io_channel_set_encoding(io, NULL, NULL);
g_io_channel_set_buffered(io, FALSE);
@@ -486,10 +484,8 @@ GAttrib *g_attrib_new(GIOChannel *io)
if (attrib == NULL)
return NULL;
- att_mtu = ATT_DEFAULT_LE_MTU;
-
- attrib->buf = g_malloc0(att_mtu);
- attrib->buflen = att_mtu;
+ attrib->buf = g_malloc0(mtu);
+ attrib->buflen = mtu;
attrib->io = g_io_channel_ref(io);
attrib->requests = g_queue_new();
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 18df2ad..99b8b37 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -42,7 +42,7 @@ typedef void (*GAttribDebugFunc)(const char *str, gpointer user_data);
typedef void (*GAttribNotifyFunc)(const guint8 *pdu, guint16 len,
gpointer user_data);
-GAttrib *g_attrib_new(GIOChannel *io);
+GAttrib *g_attrib_new(GIOChannel *io, guint16 mtu);
GAttrib *g_attrib_ref(GAttrib *attrib);
void g_attrib_unref(GAttrib *attrib);
diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index db5da62..8f92d62 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -123,6 +123,9 @@ static gboolean listen_start(gpointer user_data)
static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
{
GAttrib *attrib;
+ uint16_t mtu;
+ uint16_t cid;
+ GError *gerr = NULL;
if (err) {
g_printerr("%s\n", err->message);
@@ -130,7 +133,19 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
g_main_loop_quit(event_loop);
}
- attrib = g_attrib_new(io);
+ bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &mtu,
+ BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID);
+
+ if (gerr) {
+ g_printerr("Can't detect MTU, using default: %s", gerr->message);
+ g_error_free(gerr);
+ mtu = ATT_DEFAULT_LE_MTU;
+ }
+
+ if (cid == ATT_CID)
+ mtu = ATT_DEFAULT_LE_MTU;
+
+ attrib = g_attrib_new(io, mtu);
if (opt_listen)
g_idle_add(listen_start, attrib);
diff --git a/attrib/interactive.c b/attrib/interactive.c
index 08f39f7..7911ba5 100644
--- a/attrib/interactive.c
+++ b/attrib/interactive.c
@@ -150,13 +150,28 @@ static void events_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
{
+ uint16_t mtu;
+ uint16_t cid;
+
if (err) {
set_state(STATE_DISCONNECTED);
error("%s\n", err->message);
return;
}
- attrib = g_attrib_new(iochannel);
+ bt_io_get(io, &err, BT_IO_OPT_IMTU, &mtu,
+ BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID);
+
+ if (err) {
+ g_printerr("Can't detect MTU, using default: %s", err->message);
+ g_error_free(err);
+ mtu = ATT_DEFAULT_LE_MTU;
+ }
+
+ if (cid == ATT_CID)
+ mtu = ATT_DEFAULT_LE_MTU;
+
+ attrib = g_attrib_new(iochannel, mtu);
g_attrib_register(attrib, ATT_OP_HANDLE_NOTIFY, GATTRIB_ALL_HANDLES,
events_handler, attrib, NULL);
g_attrib_register(attrib, ATT_OP_HANDLE_IND, GATTRIB_ALL_HANDLES,
diff --git a/src/device.c b/src/device.c
index 875a5c5..0925951 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3627,11 +3627,17 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
GError *gerr = NULL;
GAttrib *attrib;
BtIOSecLevel sec_level;
+ uint16_t mtu;
+ uint16_t cid;
bt_io_get(io, &gerr, BT_IO_OPT_SEC_LEVEL, &sec_level,
+ BT_IO_OPT_IMTU, &mtu,
+ BT_IO_OPT_CID, &cid,
BT_IO_OPT_INVALID);
+
if (gerr) {
error("bt_io_get: %s", gerr->message);
+ mtu = ATT_DEFAULT_LE_MTU;
g_error_free(gerr);
return false;
}
@@ -3649,7 +3655,10 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
}
}
- attrib = g_attrib_new(io);
+ if (cid == ATT_CID)
+ mtu = ATT_DEFAULT_LE_MTU;
+
+ attrib = g_attrib_new(io, mtu);
if (!attrib) {
error("Unable to create new GAttrib instance");
return false;
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ 2/5] attrib: Remove MTU-probing code
From: Michael Janssen @ 2014-10-23 16:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414082009-40903-1-git-send-email-jamuraa@chromium.org>
Probing for the MTU using bt_io is problematic for
testing because you cannot impersonate AF_BLUETOOTH sockets
with a socketpair.
---
attrib/gattrib.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 71d1cef..fa41ade 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -476,27 +476,17 @@ done:
GAttrib *g_attrib_new(GIOChannel *io)
{
struct _GAttrib *attrib;
- uint16_t imtu;
uint16_t att_mtu;
uint16_t cid;
- GError *gerr = NULL;
g_io_channel_set_encoding(io, NULL, NULL);
g_io_channel_set_buffered(io, FALSE);
- bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &imtu,
- BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID);
- if (gerr) {
- error("%s", gerr->message);
- g_error_free(gerr);
- return NULL;
- }
-
attrib = g_try_new0(struct _GAttrib, 1);
if (attrib == NULL)
return NULL;
- att_mtu = (cid == ATT_CID) ? ATT_DEFAULT_LE_MTU : imtu;
+ att_mtu = ATT_DEFAULT_LE_MTU;
attrib->buf = g_malloc0(att_mtu);
attrib->buflen = att_mtu;
@@ -651,7 +641,6 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
return ret;
}
-
uint8_t *g_attrib_get_buffer(GAttrib *attrib, size_t *len)
{
if (len == NULL)
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ 1/5] Remove unused g_attrib_set_debug function
From: Michael Janssen @ 2014-10-23 16:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414082009-40903-1-git-send-email-jamuraa@chromium.org>
This function is not used, and also not implemented.
---
attrib/gattrib.c | 5 -----
attrib/gattrib.h | 3 ---
2 files changed, 8 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 912dffb..71d1cef 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -651,11 +651,6 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
return ret;
}
-gboolean g_attrib_set_debug(GAttrib *attrib,
- GAttribDebugFunc func, gpointer user_data)
-{
- return TRUE;
-}
uint8_t *g_attrib_get_buffer(GAttrib *attrib, size_t *len)
{
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 3fe92c7..18df2ad 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -58,9 +58,6 @@ guint g_attrib_send(GAttrib *attrib, guint id, const guint8 *pdu, guint16 len,
gboolean g_attrib_cancel(GAttrib *attrib, guint id);
gboolean g_attrib_cancel_all(GAttrib *attrib);
-gboolean g_attrib_set_debug(GAttrib *attrib,
- GAttribDebugFunc func, gpointer user_data);
-
guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle,
GAttribNotifyFunc func, gpointer user_data,
GDestroyNotify notify);
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ 0/5] Unit tests for GAttrib
From: Michael Janssen @ 2014-10-23 16:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
Since we want to shim GAttrib for a smooth transition to bt_att
profiles, this patch cleans up the API and adds unit tests for
GAttrib functions.
This adds tests for all but the register / unregister functions, which
I will send in a separate patch since they are slightly more complicated due
to catchalls.
Michael Janssen (5):
Remove unused g_attrib_set_debug function
attrib: Remove MTU-probing code
attrib: Add mtu argument to g_attrib_new
attrib: remove g_attrib_is_encrypted
Add unit test for gattrib
.gitignore | 1 +
Makefile.am | 7 +
attrib/gattrib.c | 38 +---
attrib/gattrib.h | 7 +-
attrib/gatttool.c | 17 +-
attrib/interactive.c | 17 +-
src/attrib-server.c | 9 +-
src/device.c | 11 +-
unit/test-gattrib.c | 584 +++++++++++++++++++++++++++++++++++++++++++++++++++
9 files changed, 645 insertions(+), 46 deletions(-)
create mode 100644 unit/test-gattrib.c
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply
* Re: [PATCH 1/1 net-next] Bluetooth: fix shadow warning in hci_disconnect()
From: Marcel Holtmann @ 2014-10-23 16:29 UTC (permalink / raw)
To: Fabian Frederick
Cc: linux-kernel, Gustavo F. Padovan, Johan Hedberg, David S. Miller,
BlueZ development, Network Development
In-Reply-To: <1414081365-6461-1-git-send-email-fabf@skynet.be>
Hi Fabian,
> use cpr for hci_cp_read_clock_offset instead of cp
> (already defined above).
>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
> net/bluetooth/hci_conn.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..6151e09 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -141,10 +141,10 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason)
> */
> if (conn->type == ACL_LINK && conn->role == HCI_ROLE_MASTER) {
> struct hci_dev *hdev = conn->hdev;
> - struct hci_cp_read_clock_offset cp;
> + struct hci_cp_read_clock_offset cpr;
>
> - cp.handle = cpu_to_le16(conn->handle);
> - hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cp), &cp);
> + cpr.handle = cpu_to_le16(conn->handle);
> + hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cpr), &cpr);
valid change, but I do not like cpr as variable name. We need to come up with a better one. There are other places in the code where we had the same thing. Please have a look there.
Regards
Marcel
^ permalink raw reply
* [PATCH 1/1 net-next] Bluetooth: fix shadow warning in hci_disconnect()
From: Fabian Frederick @ 2014-10-23 16:22 UTC (permalink / raw)
To: linux-kernel
Cc: Fabian Frederick, Marcel Holtmann, Gustavo Padovan, Johan Hedberg,
David S. Miller, linux-bluetooth, netdev
use cpr for hci_cp_read_clock_offset instead of cp
(already defined above).
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
net/bluetooth/hci_conn.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..6151e09 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -141,10 +141,10 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason)
*/
if (conn->type == ACL_LINK && conn->role == HCI_ROLE_MASTER) {
struct hci_dev *hdev = conn->hdev;
- struct hci_cp_read_clock_offset cp;
+ struct hci_cp_read_clock_offset cpr;
- cp.handle = cpu_to_le16(conn->handle);
- hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cp), &cp);
+ cpr.handle = cpu_to_le16(conn->handle);
+ hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cpr), &cpr);
}
conn->state = BT_DISCONN;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH v4 08/11] unit/test-hfp: Add init test for HFP HF
From: Lukasz Rymanowski @ 2014-10-23 15:00 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <44190135.DuM4UiHnJI@uw000953>
Hi Szymon,
On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:08 Lukasz Rymanowski wrote:
>> This patch adds basic infrastruction for HFP HF test plus
>> init test.
>>
>> It also moves send_pdu function in the file so it can be used by
>> test_hf_handler
>> ---
>> unit/test-hfp.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 84 insertions(+), 18 deletions(-)
>>
>> diff --git a/unit/test-hfp.c b/unit/test-hfp.c
>> index 4b3473b..274ee55 100644
>> --- a/unit/test-hfp.c
>> +++ b/unit/test-hfp.c
>> @@ -36,6 +36,7 @@ struct context {
>> int fd_server;
>> int fd_client;
>> struct hfp_gw *hfp;
>> + struct hfp_hf *hfp_hf;
>> const struct test_data *data;
>> unsigned int pdu_offset;
>> };
>> @@ -52,6 +53,8 @@ struct test_data {
>> char *test_name;
>> struct test_pdu *pdu_list;
>> hfp_result_func_t result_func;
>> + hfp_response_func_t response_func;
>> + hfp_hf_result_func_t hf_result_func;
>> GIOFunc test_handler;
>> };
>>
>> @@ -99,6 +102,22 @@ struct test_data {
>> data.test_handler = test_handler; \
>> } while (0)
>>
>> +#define define_hf_test(name, function, result_func, response_function, \
>> + args...)\
>> + do { \
>> + const struct test_pdu pdus[] = { \
>> + args, { } \
>> + }; \
>> + static struct test_data data; \
>> + data.test_name = g_strdup(name); \
>> + data.pdu_list = g_malloc(sizeof(pdus)); \
>> + data.hf_result_func = result_func; \
>> + data.response_func = response_function; \
>> + memcpy(data.pdu_list, pdus, sizeof(pdus)); \
>> + g_test_add_data_func(name, &data, function); \
>> + data.test_handler = test_hf_handler; \
>> + } while (0)
>> +
>> static void context_quit(struct context *context)
>> {
>> g_main_loop_quit(context->main_loop);
>> @@ -128,6 +147,52 @@ static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
>> return FALSE;
>> }
>>
>> +static gboolean send_pdu(gpointer user_data)
>> +{
>> + struct context *context = user_data;
>> + const struct test_pdu *pdu;
>> + ssize_t len;
>> +
>> + pdu = &context->data->pdu_list[context->pdu_offset++];
>> +
>> + if (pdu && !pdu->valid)
>> + return FALSE;
>> +
>> + len = write(context->fd_server, pdu->data, pdu->size);
>> + g_assert_cmpint(len, ==, pdu->size);
>> +
>> + pdu = &context->data->pdu_list[context->pdu_offset];
>> + if (pdu->fragmented)
>> + g_idle_add(send_pdu, context);
>> +
>> + return FALSE;
>> +}
>> +
>> +static gboolean test_hf_handler(GIOChannel *channel, GIOCondition cond,
>> + gpointer user_data)
>> +{
>> + struct context *context = user_data;
>> + gchar buf[60];
>> + gsize bytes_read;
>> + GError *error = NULL;
>> +
>> + if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
>> + goto done;
>> +
>> + /* dummy read */
>> + g_io_channel_read_chars(channel, buf, 60, &bytes_read, &error);
>> +
>> + send_pdu(context);
>> +
>> + return TRUE;
>> +
>> +done:
>> + context_quit(context);
>> + context->watch_id = 0;
>> +
>> + return FALSE;
>> +}
>> +
>> static void cmd_handler(const char *command, void *user_data)
>> {
>> struct context *context = user_data;
>> @@ -203,6 +268,9 @@ static void execute_context(struct context *context)
>> if (context->hfp)
>> hfp_gw_unref(context->hfp);
>>
>> + if (context->hfp_hf)
>> + hfp_hf_unref(context->hfp_hf);
>> +
>> g_free(context);
>> }
>>
>> @@ -275,24 +343,6 @@ static void test_register(gconstpointer data)
>> execute_context(context);
>> }
>>
>> -static gboolean send_pdu(gpointer user_data)
>> -{
>> - struct context *context = user_data;
>> - const struct test_pdu *pdu;
>> - ssize_t len;
>> -
>> - pdu = &context->data->pdu_list[context->pdu_offset++];
>> -
>> - len = write(context->fd_server, pdu->data, pdu->size);
>> - g_assert_cmpint(len, ==, pdu->size);
>> -
>> - pdu = &context->data->pdu_list[context->pdu_offset];
>> - if (pdu->fragmented)
>> - g_idle_add(send_pdu, context);
>> -
>> - return FALSE;
>> -}
>> -
>> static void test_fragmented(gconstpointer data)
>> {
>> struct context *context = create_context(data);
>> @@ -404,6 +454,20 @@ static void check_string_2(struct hfp_gw_result *result,
>> hfp_gw_send_result(context->hfp, HFP_RESULT_ERROR);
>> }
>>
>> +static void test_hf_init(gconstpointer data)
>> +{
>> + struct context *context = create_context(data);
>> +
>> + context->hfp_hf = hfp_hf_new(context->fd_client);
>> + g_assert(context->hfp_hf);
>> + g_assert(hfp_hf_set_close_on_unref(context->hfp_hf, true));
>> +
>> + hfp_hf_unref(context->hfp_hf);
>> + context->hfp_hf = NULL;
>> +
>> + execute_context(context);
>> +}
>> +
>> int main(int argc, char *argv[])
>> {
>> g_test_init(&argc, &argv, NULL);
>> @@ -473,5 +537,7 @@ int main(int argc, char *argv[])
>> raw_pdu('\r'),
>> data_end());
>>
>> + define_hf_test("/hfp/test_init", test_hf_init, NULL, NULL, data_end());
>
> I'd prefer if all hfp_hf tests were prefixed like this:
> "/hfp_hf/test_foo"
>
> This will allow to avoid doubling tests name like this one (there is already
> /hfp/test_init test).
>
OK
>> +
>> return g_test_run();
>> }
>>
>
> --
> Best regards,
> Szymon Janc
\Łukasz
^ permalink raw reply
* Re: [PATCH v4 06/11] shared/hfp: Add send AT command API for HFP HF
From: Lukasz Rymanowski @ 2014-10-23 15:00 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <2209288.OLlArcbHcy@uw000953>
Hi Szymon,
On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:06 Lukasz Rymanowski wrote:
>> This patch adds handling send and response of AT command.
>> Note that we always wait for AT command response before sending next
>> command, however user can fill hfp_hf with more than one command.
>> All the commands are queued and send one by one.
>> ---
>> src/shared/hfp.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/shared/hfp.h | 8 +++
>> 2 files changed, 183 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index 5179092..8bebe97 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -70,6 +70,9 @@ struct hfp_hf {
>> struct ringbuf *read_buf;
>> struct ringbuf *write_buf;
>>
>> + bool writer_active;
>> + struct queue *cmd_queue;
>> +
>> struct queue *event_handlers;
>>
>> hfp_debug_func_t debug_callback;
>> @@ -101,6 +104,14 @@ struct hfp_hf_result {
>> unsigned int offset;
>> };
>>
>> +struct cmd_response {
>> + char *prefix;
>> + hfp_response_func_t resp_cb;
>> + struct hfp_hf_result *response;
>> + char *resp_data;
>> + void *user_data;
>> +};
>> +
>> struct event_handler {
>> char *prefix;
>> void *user_data;
>> @@ -868,17 +879,103 @@ static void destroy_event_handler(void *data)
>> free(handler);
>> }
>>
>> +static bool hf_can_write_data(struct io *io, void *user_data)
>> +{
>> + struct hfp_hf *hfp = user_data;
>> + ssize_t bytes_written;
>> +
>> + bytes_written = ringbuf_write(hfp->write_buf, hfp->fd);
>> + if (bytes_written < 0)
>> + return false;
>> +
>> + if (ringbuf_len(hfp->write_buf) > 0)
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +static void hf_write_watch_destroy(void *user_data)
>> +{
>> + struct hfp_hf *hfp = user_data;
>> +
>> + hfp->writer_active = false;
>> +}
>> +
>> static void hf_skip_whitespace(struct hfp_hf_result *result)
>> {
>> while (result->data[result->offset] == ' ')
>> result->offset++;
>> }
>>
>> +static bool is_response(const char *prefix, enum hfp_result *result)
>> +{
>> + if (strcmp(prefix, "OK") == 0) {
>> + *result = HFP_RESULT_OK;
>> + return true;
>> + }
>> +
>> + if (strcmp(prefix, "ERROR") == 0) {
>> + *result = HFP_RESULT_ERROR;
>> + return true;
>> + }
>> +
>> + if (strcmp(prefix, "NO CARRIER") == 0) {
>> + *result = HFP_RESULT_NO_CARRIER;
>> + return true;
>> + }
>> +
>> + if (strcmp(prefix, "CONNECT") == 0) {
>
> Shouldn't this be "BUSY"?
>
>> + *result = HFP_RESULT_CONNECT;
>
> And this enum value looks bogus to me.
> I couldn't find it in HFP nor HSP spec. Probably leftover from AT spec.
> I'd just handle what is defined in HFP spec here.
This comes from AT spec. I based on hfp_result enum but indeed this is
not needed. Will fix that and hfp_result enum. Agree that we need only
HFP/HSP related result here.
>
>> + return true;
>> + }
>> +
>> + if (strcmp(prefix, "NO ANSWER") == 0) {
>> + *result = HFP_RESULT_NO_ANSWER;
>> + return true;
>> + }
>> +
>> + if (strcmp(prefix, "DELAYED") == 0) {
>> + *result = HFP_RESULT_DELAYED;
>> + return true;
>> + }
>> +
>> + if (strcmp(prefix, "BLACKLISTED") == 0) {
>> + *result = HFP_RESULT_BLACKLISTED;
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static void hf_wakeup_writer(struct hfp_hf *hfp)
>> +{
>> + if (hfp->writer_active)
>> + return;
>> +
>> + if (!ringbuf_len(hfp->write_buf))
>> + return;
>> +
>> + if (!io_set_write_handler(hfp->io, hf_can_write_data,
>> + hfp, hf_write_watch_destroy))
>> + return;
>> +
>> + hfp->writer_active = true;
>> +}
>> +
>> +static void destroy_cmd_response(void *data)
>> +{
>> + struct cmd_response *cmd = data;
>> +
>> + free(cmd->prefix);
>> + free(cmd);
>> +}
>> +
>> static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>> {
>> struct event_handler *handler;
>> const char *separators = ";:\0";
>> struct hfp_hf_result result_data;
>> + enum hfp_result result;
>> char lookup_prefix[18];
>> uint8_t pref_len = 0;
>> const char *prefix;
>> @@ -904,6 +1001,22 @@ static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>> lookup_prefix[pref_len] = '\0';
>> result_data.offset += pref_len + 1;
>>
>> + if (is_response(lookup_prefix, &result)) {
>> + struct cmd_response *cmd;
>> +
>> + cmd = queue_peek_head(hfp->cmd_queue);
>> + if (!cmd)
>> + return;
>> +
>> + cmd->resp_cb(cmd->prefix, result, cmd->user_data);
>> +
>> + queue_remove(hfp->cmd_queue, cmd);
>> + destroy_cmd_response(cmd);
>> +
>> + hf_wakeup_writer(hfp);
>> + return;
>> + }
>> +
>> handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
>> lookup_prefix);
>> if (!handler)
>> @@ -1032,6 +1145,18 @@ struct hfp_hf *hfp_hf_new(int fd)
>> return NULL;
>> }
>>
>> + hfp->cmd_queue = queue_new();
>> + if (!hfp->cmd_queue) {
>> + io_destroy(hfp->io);
>> + ringbuf_free(hfp->write_buf);
>> + ringbuf_free(hfp->read_buf);
>> + queue_destroy(hfp->event_handlers, NULL);
>> + free(hfp);
>> + return NULL;
>> + }
>> +
>> + hfp->writer_active = false;
>> +
>> if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
>> read_watch_destroy)) {
>> queue_destroy(hfp->event_handlers,
>> @@ -1083,6 +1208,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
>> queue_destroy(hfp->event_handlers, destroy_event_handler);
>> hfp->event_handlers = NULL;
>>
>> + queue_destroy(hfp->cmd_queue, destroy_cmd_response);
>> + hfp->cmd_queue = NULL;
>> +
>> if (!hfp->in_disconnect) {
>> free(hfp);
>> return;
>> @@ -1142,6 +1270,53 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
>> return true;
>> }
>>
>> +bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
>> + void *user_data, const char *format, ...)
>> +{
>> + va_list ap;
>> + char *fmt;
>> + int len;
>> + const char *separators = ";?=\0";
>> + uint8_t prefix_len;
>> + struct cmd_response *cmd;
>> +
>> + if (!hfp || !format || !resp_cb)
>> + return false;
>> +
>> + if (asprintf(&fmt, "%s\r", format) < 0)
>> + return false;
>> +
>> + cmd = new0(struct cmd_response, 1);
>> + if (!cmd)
>> + return false;
>> +
>> + va_start(ap, format);
>> + len = ringbuf_vprintf(hfp->write_buf, fmt, ap);
>> + va_end(ap);
>> +
>> + free(fmt);
>> +
>> + if (len < 0) {
>> + free(cmd);
>> + return false;
>> + }
>> +
>> + prefix_len = strcspn(format, separators);
>
> I'd explore possibility of passing prefix as separate argument to the
> function to avoid need of this extra parsing. Also do we really need this
> prefix at all? We should not have more than one pending AT command anyway.
Well this is some leftover from my previous patches where I based on
it (had single function for command complete where I check that and
move with SLC connection setup) Now It is not needed in so will
basically remove it.
>
>> + cmd->prefix = strndup(format, prefix_len);
>> + cmd->resp_cb = resp_cb;
>> + cmd->user_data = user_data;
>> +
>> + if (!queue_push_tail(hfp->cmd_queue, cmd)) {
>> + ringbuf_drain(hfp->write_buf, len);
>> + free(cmd);
>> + return false;
>> + }
>> +
>> + hf_wakeup_writer(hfp);
>> +
>> + return true;
>> +}
>> +
>> bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>> const char *prefix,
>> void *user_data,
>> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
>> index 85037b1..5ee3017 100644
>> --- a/src/shared/hfp.h
>> +++ b/src/shared/hfp.h
>> @@ -32,6 +32,8 @@ enum hfp_result {
>> HFP_RESULT_NO_DIALTONE = 6,
>> HFP_RESULT_BUSY = 7,
>> HFP_RESULT_NO_ANSWER = 8,
>> + HFP_RESULT_DELAYED = 9,
>> + HFP_RESULT_BLACKLISTED = 10,
>> };
>>
>> enum hfp_error {
>> @@ -83,6 +85,10 @@ typedef void (*hfp_command_func_t)(const char *command, void *user_data);
>>
>> typedef void (*hfp_disconnect_func_t)(void *user_data);
>>
>> +typedef void (*hfp_response_func_t)(const char *prefix,
>> + enum hfp_result result,
>> + void *user_data);
>> +
>> struct hfp_gw;
>> struct hfp_hf;
>>
>> @@ -146,3 +152,5 @@ bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>> const char *prefix, void *user_data,
>> hfp_destroy_func_t destroy);
>> bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
>> +bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
>> + void *user_data, const char *format, ...);
>>
>
> --
> Best regards,
> Szymon Janc
BR
Lukasz
^ permalink raw reply
* Re: [PATCH v4 01/11] shared/hfp: Add support for HFP HF
From: Lukasz Rymanowski @ 2014-10-23 15:00 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <2009037.IXf9F6m0Mk@uw000953>
Hi Szymon,
On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:01 Lukasz Rymanowski wrote:
>> This patch add struct hfp_hf plus fuctions to create an instance ref and
>> unref. This code based on existing hfp_gw
>> ---
>> src/shared/hfp.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/shared/hfp.h | 6 ++++
>> 2 files changed, 98 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index efc981f..dbd049a 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -62,6 +62,18 @@ struct hfp_gw {
>> bool destroyed;
>> };
>>
>> +struct hfp_hf {
>> + int ref_count;
>> + int fd;
>> + bool close_on_unref;
>> + struct io *io;
>> + struct ringbuf *read_buf;
>> + struct ringbuf *write_buf;
>> +
>> + bool in_disconnect;
>> + bool destroyed;
>> +};
>> +
>> struct cmd_handler {
>> char *prefix;
>> void *user_data;
>> @@ -807,3 +819,83 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
>>
>> return io_shutdown(hfp->io);
>> }
>> +
>> +struct hfp_hf *hfp_hf_new(int fd)
>> +{
>> + struct hfp_hf *hfp;
>> +
>> + if (fd < 0)
>> + return NULL;
>> +
>> + hfp = new0(struct hfp_hf, 1);
>> + if (!hfp)
>> + return NULL;
>> +
>> + hfp->fd = fd;
>> + hfp->close_on_unref = false;
>> +
>> + hfp->read_buf = ringbuf_new(4096);
>> + if (!hfp->read_buf) {
>> + free(hfp);
>> + return NULL;
>> + }
>> +
>> + hfp->write_buf = ringbuf_new(4096);
>> + if (!hfp->write_buf) {
>> + ringbuf_free(hfp->read_buf);
>> + free(hfp);
>> + return NULL;
>> + }
>> +
>> + hfp->io = io_new(fd);
>> + if (!hfp->io) {
>> + ringbuf_free(hfp->write_buf);
>> + ringbuf_free(hfp->read_buf);
>> + free(hfp);
>> + return NULL;
>> + }
>> +
>> + return hfp_hf_ref(hfp);
>> +}
>> +
>> +struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp)
>> +{
>> + if (!hfp)
>> + return NULL;
>> +
>> + __sync_fetch_and_add(&hfp->ref_count, 1);
>> +
>> + return hfp;
>> +}
>> +
>> +void hfp_hf_unref(struct hfp_hf *hfp)
>> +{
>> + if (!hfp)
>> + return;
>> +
>> + if (__sync_sub_and_fetch(&hfp->ref_count, 1))
>> + return;
>> +
>> + io_set_write_handler(hfp->io, NULL, NULL, NULL);
>> + io_set_read_handler(hfp->io, NULL, NULL, NULL);
>> + io_set_disconnect_handler(hfp->io, NULL, NULL, NULL);
>> +
>> + io_destroy(hfp->io);
>> + hfp->io = NULL;
>> +
>> + if (hfp->close_on_unref)
>> + close(hfp->fd);
>> +
>> + ringbuf_free(hfp->read_buf);
>> + hfp->read_buf = NULL;
>> +
>> + ringbuf_free(hfp->write_buf);
>> + hfp->write_buf = NULL;
>> +
>> + if (!hfp->in_disconnect) {
>> + free(hfp);
>> + return;
>> + }
>> +
>> + hfp->destroyed = true;
>> +}
>> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
>> index 743db65..50d9c4b 100644
>> --- a/src/shared/hfp.h
>> +++ b/src/shared/hfp.h
>> @@ -76,9 +76,11 @@ typedef void (*hfp_destroy_func_t)(void *user_data);
>> typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
>>
>> typedef void (*hfp_command_func_t)(const char *command, void *user_data);
>> +
>
> Unrelated.
True.
>
>> typedef void (*hfp_disconnect_func_t)(void *user_data);
>>
>> struct hfp_gw;
>> +struct hfp_hf;
>
> I'd prefer if we have all hfp_hf stuff in same section.
OK
>
>>
>> struct hfp_gw *hfp_gw_new(int fd);
>>
>> @@ -124,3 +126,7 @@ bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
>> bool hfp_gw_result_get_unquoted_string(struct hfp_gw_result *result, char *buf,
>> uint8_t len);
>> bool hfp_gw_result_has_next(struct hfp_gw_result *result);
>> +
>> +struct hfp_hf *hfp_hf_new(int fd);
>> +struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp);
>> +void hfp_hf_unref(struct hfp_hf *hfp);
>>
>
> --
> Best regards,
> Szymon Janc
\Łukasz
^ permalink raw reply
* Re: [PATCH v4 05/11] shared/hfp: Add HFP HF parser
From: Lukasz Rymanowski @ 2014-10-23 15:00 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <2626729.yqIRdlFYHe@uw000953>
Hi Szymon,
On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:05 Lukasz Rymanowski wrote:
>> This patch adds parser for AT responses and unsolicited results.
>> ---
>> src/shared/hfp.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 129 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index b1cf08e..5179092 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -868,6 +868,126 @@ static void destroy_event_handler(void *data)
>> free(handler);
>> }
>>
>> +static void hf_skip_whitespace(struct hfp_hf_result *result)
>> +{
>> + while (result->data[result->offset] == ' ')
>> + result->offset++;
>> +}
>> +
>> +static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>> +{
>> + struct event_handler *handler;
>> + const char *separators = ";:\0";
>> + struct hfp_hf_result result_data;
>> + char lookup_prefix[18];
>> + uint8_t pref_len = 0;
>> + const char *prefix;
>> + int i;
>> +
>> + result_data.offset = 0;
>> + result_data.data = data;
>> +
>> + hf_skip_whitespace(&result_data);
>> +
>> + if (strlen(data + result_data.offset) < 2)
>> + return;
>> +
>> + prefix = data + result_data.offset;
>> +
>> + pref_len = strcspn(prefix, separators);
>> + if (pref_len > 17 || pref_len < 2)
>> + return;
>> +
>> + for (i = 0; i < pref_len; i++)
>> + lookup_prefix[i] = toupper(prefix[i]);
>> +
>> + lookup_prefix[pref_len] = '\0';
>> + result_data.offset += pref_len + 1;
>> +
>> + handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
>> + lookup_prefix);
>> + if (!handler)
>> + return;
>> +
>> + handler->callback(&result_data, handler->user_data);
>> +}
>> +
>> +static char *find_cr_lf(char *str, size_t len)
>> +{
>> + char *ptr;
>> + int count;
>> + int offset;
>> +
>> + offset = 0;
>> +
>> + ptr = memchr(str, '\r', len);
>> + while (ptr) {
>> + /*
>> + * Check if there is more data after '\r'. If so check for
>> + * '\n'
>> + */
>> + count = ptr-str;
>
> Style: spaces around -
>
>> + if ((count < (int) (len - 1)) && *(ptr + 1) == '\n')
>> + return ptr;
>
> If you make count size_t then this cast is not needed.
>
>> +
>> + /* There is only '\r'? Let's try to find next one */
>> + offset += count + 1;
>> +
>> + if (offset >= (int)len)
>
> If you make offset size_t then this cast is not needed.
> Also such casting should have space '(int) foo'.
>
>> + return NULL;
>> +
>> + ptr = memchr(str + offset, '\r', len - offset);
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static void hf_process_input(struct hfp_hf *hfp)
>> +{
>> + char *str, *ptr;
>> + size_t len, count, offset;
>> +
>> + str = ringbuf_peek(hfp->read_buf, 0, &len);
>> + if (!str)
>> + return;
>> +
>> + offset = 0;
>> +
>> + ptr = find_cr_lf(str, len);
>> + while (ptr) {
>> + count = ptr - (str + offset);
>
> If you would adjust str pointer instead of using str + offset everywhere
> then this code would be a bit simpler to follow.
>
see below
> Also this would not handle wrapped string correctly. Check how this is handled
> in process_input().
Missed that. Will fix, also will fix that code as asprintf should not
be use. str is not a string in case buffer is wrapped.
Also will need to keep offset so I can concatenate str and str2 which
will come from the begging of ringbuf
>
>> + if (count == 0) {
>> + /* 2 is for <cr><lf> */
>> + offset += 2;
>> + } else {
>> + *ptr = '\0';
>> + hf_call_prefix_handler(hfp, str + offset);
>> + offset += count + 2;
>> + }
>> +
>> + if (offset >= len)
>> + break;
>> +
>> + ptr = find_cr_lf(str + offset, len - offset);
>> + }
>> +
>> + ringbuf_drain(hfp->read_buf, offset < len ? offset : len);
>> +}
>> +
>> +static bool hf_can_read_data(struct io *io, void *user_data)
>> +{
>> + struct hfp_hf *hfp = user_data;
>> + ssize_t bytes_read;
>> +
>> + bytes_read = ringbuf_read(hfp->read_buf, hfp->fd);
>> + if (bytes_read < 0)
>> + return false;
>> +
>> + hf_process_input(hfp);
>> +
>> + return true;
>> +}
>> +
>> struct hfp_hf *hfp_hf_new(int fd)
>> {
>> struct hfp_hf *hfp;
>> @@ -912,6 +1032,15 @@ struct hfp_hf *hfp_hf_new(int fd)
>> return NULL;
>> }
>>
>> + if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
>> + read_watch_destroy)) {
>> + queue_destroy(hfp->event_handlers,
>> + destroy_event_handler);
>> + io_destroy(hfp->io);
>> + ringbuf_free(hfp->write_buf);
>> + ringbuf_free(hfp->read_buf);
>
> You are missing free(hfp); return NULL; here.
ah, some rebase issue. thanks
>
>> + }
>> +
>> return hfp_hf_ref(hfp);
>> }
>>
>>
>
> --
> Best regards,
> Szymon Janc
\Łukasz
^ permalink raw reply
* Re: [PATCH v4 04/11] shared/hfp: Add register/unregister event for HFP HF
From: Lukasz Rymanowski @ 2014-10-23 15:00 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <11593362.6Zs3oQ3sJP@uw000953>
Hi Szymon,
On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:04 Lukasz Rymanowski wrote:
>> This patch adds API which allows to register/unregister for unsolicited
>> responses.
>> ---
>> src/shared/hfp.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/shared/hfp.h | 8 +++++
>> 2 files changed, 116 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index b7855ed..b1cf08e 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -70,6 +70,8 @@ struct hfp_hf {
>> struct ringbuf *read_buf;
>> struct ringbuf *write_buf;
>>
>> + struct queue *event_handlers;
>> +
>> hfp_debug_func_t debug_callback;
>> hfp_destroy_func_t debug_destroy;
>> void *debug_data;
>> @@ -94,6 +96,18 @@ struct hfp_gw_result {
>> unsigned int offset;
>> };
>>
>> +struct hfp_hf_result {
>> + const char *data;
>> + unsigned int offset;
>> +};
>> +
>> +struct event_handler {
>> + char *prefix;
>> + void *user_data;
>> + hfp_destroy_func_t destroy;
>> + hfp_hf_result_func_t callback;
>> +};
>> +
>> static void destroy_cmd_handler(void *data)
>> {
>> struct cmd_handler *handler = data;
>> @@ -828,6 +842,32 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
>> return io_shutdown(hfp->io);
>> }
>>
>> +static bool match_handler_event_prefix(const void *a, const void *b)
>> +{
>> + const struct event_handler *handler = a;
>> + const char *prefix = b;
>> +
>> + if (strlen(handler->prefix) != strlen(prefix))
>> + return false;
>> +
>> + if (memcmp(handler->prefix, prefix, strlen(prefix)))
>> + return false;
>
> Why not just use strcmp() here?
> I'm aware that this was based on gw code so you may also fix it there :)
Copy paste issue. But that's correct. Will send also patch for base code.
>
>> +
>> + return true;
>> +}
>> +
>> +static void destroy_event_handler(void *data)
>> +{
>> + struct event_handler *handler = data;
>> +
>> + if (handler->destroy)
>> + handler->destroy(handler->user_data);
>> +
>> + free(handler->prefix);
>> +
>> + free(handler);
>> +}
>> +
>> struct hfp_hf *hfp_hf_new(int fd)
>> {
>> struct hfp_hf *hfp;
>> @@ -863,6 +903,15 @@ struct hfp_hf *hfp_hf_new(int fd)
>> return NULL;
>> }
>>
>> + hfp->event_handlers = queue_new();
>> + if (!hfp->event_handlers) {
>> + io_destroy(hfp->io);
>> + ringbuf_free(hfp->write_buf);
>> + ringbuf_free(hfp->read_buf);
>> + free(hfp);
>> + return NULL;
>> + }
>> +
>> return hfp_hf_ref(hfp);
>> }
>>
>> @@ -902,6 +951,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
>> ringbuf_free(hfp->write_buf);
>> hfp->write_buf = NULL;
>>
>> + queue_destroy(hfp->event_handlers, destroy_event_handler);
>> + hfp->event_handlers = NULL;
>> +
>> if (!hfp->in_disconnect) {
>> free(hfp);
>> return;
>> @@ -961,6 +1013,62 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
>> return true;
>> }
>>
>> +bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>> + const char *prefix,
>> + void *user_data,
>> + hfp_destroy_func_t destroy)
>> +{
>> + struct event_handler *handler;
>> +
>> + if (!callback)
>> + return false;
>> +
>> + handler = new0(struct event_handler, 1);
>> + if (!handler)
>> + return false;
>> +
>> + handler->callback = callback;
>> + handler->user_data = user_data;
>> +
>> + handler->prefix = strdup(prefix);
>> + if (!handler->prefix) {
>> + free(handler);
>> + return false;
>> + }
>> +
>> + if (queue_find(hfp->event_handlers, match_handler_event_prefix,
>> + handler->prefix)) {
>> + destroy_event_handler(handler);
>> + return false;
>> + }
>> +
>> + handler->destroy = destroy;
>> +
>> + return queue_push_tail(hfp->event_handlers, handler);
>> +}
>> +
>> +bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix)
>> +{
>> + struct cmd_handler *handler;
>> + char *lookup_prefix;
>> +
>> + lookup_prefix = strdup(prefix);
>> + if (!lookup_prefix)
>> + return false;
>
> This strdup seems superfluous. If this is only due to to queue api being
> non-const then I'd just cast it to (void *).
:) ditto.
>
>> +
>> + handler = queue_remove_if(hfp->event_handlers,
>> + match_handler_event_prefix,
>> + lookup_prefix);
>> + free(lookup_prefix);
>> +
>> + if (!handler)
>> + return false;
>> +
>> + destroy_event_handler(handler);
>> +
>> + return true;
>> +}
>> +
>> static void hf_disconnect_watch_destroy(void *user_data)
>> {
>> struct hfp_hf *hfp = user_data;
>> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
>> index a9a169b..85037b1 100644
>> --- a/src/shared/hfp.h
>> +++ b/src/shared/hfp.h
>> @@ -68,10 +68,14 @@ enum hfp_gw_cmd_type {
>> };
>>
>> struct hfp_gw_result;
>> +struct hfp_hf_result;
>
> As before, I'd keep hf stuff in single section.
This and the one below will finally goes out as it will be merget into
hfp_context. I think there is no sense to change that here now.
>
>>
>> typedef void (*hfp_result_func_t)(struct hfp_gw_result *result,
>> enum hfp_gw_cmd_type type, void *user_data);
>>
>> +typedef void (*hfp_hf_result_func_t)(struct hfp_hf_result *result,
>> + void *user_data);
>> +
>
> Same here.
ditto.
>
>> typedef void (*hfp_destroy_func_t)(void *user_data);
>> typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
>>
>> @@ -138,3 +142,7 @@ bool hfp_hf_set_disconnect_handler(struct hfp_hf *hfp,
>> void *user_data,
>> hfp_destroy_func_t destroy);
>> bool hfp_hf_disconnect(struct hfp_hf *hfp);
>> +bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>> + const char *prefix, void *user_data,
>> + hfp_destroy_func_t destroy);
>> +bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
>>
>
> --
> Best regards,
> Szymon Janc
^ permalink raw reply
* [PATCH v5 bluetooth-next 4/4] ieee802154: 6lowpan: rename process_data and lowpan_process_data
From: Martin Townsend @ 2014-10-23 14:40 UTC (permalink / raw)
To: linux-bluetooth, linux-wpan
Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend
In-Reply-To: <1414075256-9448-1-git-send-email-martin.townsend@xsilon.com>
From: Martin Townsend <mtownsend1973@gmail.com>
As we have decouple decompression from data delivery we can now rename all
occurences of process_data in receive path.
Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
include/net/6lowpan.h | 10 ++++++----
net/6lowpan/iphc.c | 12 +++++++-----
net/bluetooth/6lowpan.c | 15 ++++++++-------
net/ieee802154/6lowpan_rtnl.c | 15 ++++++++-------
4 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index abfa359..dc03d77 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -372,10 +372,12 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
return skb->len + uncomp_header - ret;
}
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
- const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
- const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
- u8 iphc0, u8 iphc1);
+int
+lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
+ const u8 *saddr, const u8 saddr_type,
+ const u8 saddr_len, const u8 *daddr,
+ const u8 daddr_type, const u8 daddr_len,
+ u8 iphc0, u8 iphc1);
int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
unsigned short type, const void *_daddr,
const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 45714fe..73a7065 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -301,10 +301,12 @@ err:
/* TTL uncompression values */
static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
- const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
- const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
- u8 iphc0, u8 iphc1)
+int
+lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
+ const u8 *saddr, const u8 saddr_type,
+ const u8 saddr_len, const u8 *daddr,
+ const u8 daddr_type, const u8 daddr_len,
+ u8 iphc0, u8 iphc1)
{
struct ipv6hdr hdr = {};
u8 tmp, num_context = 0;
@@ -480,7 +482,7 @@ drop:
kfree_skb(skb);
return -EINVAL;
}
-EXPORT_SYMBOL_GPL(lowpan_process_data);
+EXPORT_SYMBOL_GPL(lowpan_header_decompress);
static u8 lowpan_compress_addr_64(u8 **hc_ptr, u8 shift,
const struct in6_addr *ipaddr,
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 40e2cec..aa6ebbf 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -257,8 +257,8 @@ static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
return netif_rx(skb_cp);
}
-static int process_data(struct sk_buff *skb, struct net_device *netdev,
- struct l2cap_chan *chan)
+static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
+ struct l2cap_chan *chan)
{
const u8 *saddr, *daddr;
u8 iphc0, iphc1;
@@ -287,10 +287,11 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
if (lowpan_fetch_skb_u8(skb, &iphc1))
goto drop;
- return lowpan_process_data(skb, netdev,
- saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
- daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
- iphc0, iphc1);
+ return lowpan_header_decompress(skb, netdev,
+ saddr, IEEE802154_ADDR_LONG,
+ EUI64_ADDR_LEN, daddr,
+ IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
+ iphc0, iphc1);
drop:
kfree_skb(skb);
@@ -346,7 +347,7 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
if (!local_skb)
goto drop;
- ret = process_data(local_skb, dev, chan);
+ ret = iphc_decompress(local_skb, dev, chan);
if (ret < 0)
goto drop;
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 577597d..d413a60 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -166,7 +166,8 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
return stat;
}
-static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
+static int
+iphc_decompress(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
{
u8 iphc0, iphc1;
struct ieee802154_addr_sa sa, da;
@@ -196,9 +197,9 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
else
dap = &da.hwaddr;
- return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
- IEEE802154_ADDR_LEN, dap, da.addr_type,
- IEEE802154_ADDR_LEN, iphc0, iphc1);
+ return lowpan_header_decompress(skb, skb->dev, sap, sa.addr_type,
+ IEEE802154_ADDR_LEN, dap, da.addr_type,
+ IEEE802154_ADDR_LEN, iphc0, iphc1);
drop:
kfree_skb(skb);
@@ -541,7 +542,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
} else {
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
- ret = process_data(skb, &hdr);
+ ret = iphc_decompress(skb, &hdr);
if (ret < 0)
goto drop;
@@ -549,7 +550,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
case LOWPAN_DISPATCH_FRAG1: /* first fragment header */
ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
if (ret == 1) {
- ret = process_data(skb, &hdr);
+ ret = iphc_decompress(skb, &hdr);
if (ret < 0)
goto drop;
@@ -562,7 +563,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
case LOWPAN_DISPATCH_FRAGN: /* next fragments headers */
ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
if (ret == 1) {
- ret = process_data(skb, &hdr);
+ ret = iphc_decompress(skb, &hdr);
if (ret < 0)
goto drop;
--
1.9.1
^ permalink raw reply related
* [PATCH v5 bluetooth-next 3/4] bluetooth:6lowpan: use consume_skb when packet processed successfully
From: Martin Townsend @ 2014-10-23 14:40 UTC (permalink / raw)
To: linux-bluetooth, linux-wpan
Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend
In-Reply-To: <1414075256-9448-1-git-send-email-martin.townsend@xsilon.com>
From: Martin Townsend <mtownsend1973@gmail.com>
Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
net/bluetooth/6lowpan.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 94bbb66..40e2cec 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -337,8 +337,8 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
dev->stats.rx_bytes += skb->len;
dev->stats.rx_packets++;
- kfree_skb(local_skb);
- kfree_skb(skb);
+ consume_skb(local_skb);
+ consume_skb(skb);
} else {
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
@@ -363,7 +363,8 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
dev->stats.rx_bytes += skb->len;
dev->stats.rx_packets++;
- kfree_skb(skb);
+ consume_skb(local_skb);
+ consume_skb(skb);
break;
default:
break;
--
1.9.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox