* [PATCH 04/10] android/ipc-tester: Add missing service opcode boundries test cases
From: Grzegorz Kolodziejczyk @ 2014-10-09 12:45 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1412858714-2845-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
This patch adds tests sending out of range opcode for each service.
---
android/ipc-tester.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/android/ipc-tester.c b/android/ipc-tester.c
index ea71c8d..161777d 100644
--- a/android/ipc-tester.c
+++ b/android/ipc-tester.c
@@ -919,9 +919,23 @@ int main(int argc, char *argv[])
test_opcode_valid("PAN", HAL_SERVICE_ID_PAN, 0x05, 0,
HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_PAN);
+ test_opcode_valid("HANDSFREE", HAL_SERVICE_ID_HANDSFREE, 0x0f, 0,
+ HAL_SERVICE_ID_BLUETOOTH,
+ HAL_SERVICE_ID_HANDSFREE);
+
test_opcode_valid("A2DP", HAL_SERVICE_ID_A2DP, 0x03, 0,
HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_A2DP);
+ test_opcode_valid("HEALTH", HAL_SERVICE_ID_HEALTH, 0x06, 0,
+ HAL_SERVICE_ID_BLUETOOTH,
+ HAL_SERVICE_ID_HEALTH);
+
+ test_opcode_valid("AVRCP", HAL_SERVICE_ID_AVRCP, 0x0b, 0,
+ HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_AVRCP);
+
+ test_opcode_valid("GATT", HAL_SERVICE_ID_GATT, 0x24, 0,
+ HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_GATT);
+
test_opcode_valid("HF_CLIENT", HAL_SERVICE_ID_HANDSFREE_CLIENT, 0x10, 0,
HAL_SERVICE_ID_BLUETOOTH,
HAL_SERVICE_ID_HANDSFREE_CLIENT);
--
1.9.3
^ permalink raw reply related
* [PATCH 03/10] android/map-client: Add support for get remote MAS instances
From: Grzegorz Kolodziejczyk @ 2014-10-09 12:45 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1412858714-2845-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
This allows to get remote mas instances. In case of wrong sdp record
fail status will be returned in notification.
---
android/map-client.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 123 insertions(+), 1 deletion(-)
diff --git a/android/map-client.c b/android/map-client.c
index 1001b36..adeae4b 100644
--- a/android/map-client.c
+++ b/android/map-client.c
@@ -30,21 +30,143 @@
#include <stdint.h>
#include <glib.h>
+#include "lib/sdp.h"
+#include "lib/sdp_lib.h"
+#include "src/sdp-client.h"
+
#include "ipc.h"
#include "lib/bluetooth.h"
#include "map-client.h"
#include "src/log.h"
#include "hal-msg.h"
+#include "ipc-common.h"
+#include "utils.h"
+#include "src/shared/util.h"
static struct ipc *hal_ipc = NULL;
static bdaddr_t adapter_addr;
+static int fill_mce_inst(void *buf, int32_t id, int32_t scn, int32_t msg_type,
+ const void *name, uint8_t name_len)
+{
+ struct hal_map_client_mas_instance *inst = buf;
+
+ inst->id = id;
+ inst->scn = scn;
+ inst->msg_types = msg_type;
+ inst->name_len = name_len;
+
+ if (name_len)
+ memcpy(inst->name, name, name_len);
+
+ return sizeof(*inst) + name_len;
+}
+
+static void map_client_sdp_search_cb(sdp_list_t *recs, int err, gpointer data)
+{
+ uint8_t buf[IPC_MTU];
+ struct hal_ev_map_client_remote_mas_instances *ev = (void *) buf;
+ bdaddr_t *dst = data;
+ sdp_list_t *list, *protos;
+ uint8_t status;
+ int32_t id, scn, msg_type, name_len, num_instances = 0;
+ char *name;
+ size_t size;
+
+ size = sizeof(*ev);
+ bdaddr2android(dst, &ev->bdaddr);
+
+ if (err < 0) {
+ error("mce: Unable to get SDP record: %s", strerror(-err));
+ status = HAL_STATUS_FAILED;
+ goto fail;
+ }
+
+ for (list = recs; list != NULL; list = list->next) {
+ sdp_record_t *rec = list->data;
+ sdp_data_t *data;
+
+ data = sdp_data_get(rec, SDP_ATTR_MAS_INSTANCE_ID);
+ if (data) {
+ id = data->val.uint8;
+ } else {
+ error("mce: cannot get mas instance id");
+ status = HAL_STATUS_FAILED;
+ goto fail;
+ }
+
+ data = sdp_data_get(rec, SDP_ATTR_SVCNAME_PRIMARY);
+ if (data) {
+ name = data->val.str;
+ name_len = data->unitSize;
+ } else {
+ error("mce: cannot get mas instance name");
+ status = HAL_STATUS_FAILED;
+ goto fail;
+ }
+
+ data = sdp_data_get(rec, SDP_ATTR_SUPPORTED_MESSAGE_TYPES);
+ if (data) {
+ msg_type = data->val.uint8;
+ } else {
+ error("mce: cannot get mas instance msg type");
+ status = HAL_STATUS_FAILED;
+ goto fail;
+ }
+
+ if (!sdp_get_access_protos(rec, &protos)) {
+ scn = sdp_get_proto_port(protos, RFCOMM_UUID);
+
+ sdp_list_foreach(protos,
+ (sdp_list_func_t) sdp_list_free, NULL);
+ sdp_list_free(protos, NULL);
+ } else {
+ error("mce: cannot get mas instance rfcomm channel");
+ status = HAL_STATUS_FAILED;
+ goto fail;
+ }
+
+ size += fill_mce_inst(buf + size, id, scn, msg_type, name,
+ name_len);
+ num_instances++;
+ }
+
+ status = HAL_STATUS_SUCCESS;
+
+fail:
+ ev->num_instances = num_instances;
+ ev->status = status;
+
+ ipc_send_notif(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT,
+ HAL_EV_MAP_CLIENT_REMOTE_MAS_INSTANCES, size, buf);
+
+ free(dst);
+}
+
static void handle_get_instances(const void *buf, uint16_t len)
{
+ const struct hal_cmd_map_client_get_instances *cmd = buf;
+ uint8_t status;
+ bdaddr_t *dst;
+ uuid_t uuid;
+
DBG("");
+ dst = new0(bdaddr_t, 1);
+ android2bdaddr(&cmd->bdaddr, dst);
+
+ sdp_uuid16_create(&uuid, MAP_MSE_SVCLASS_ID);
+
+ if (bt_search_service(&adapter_addr, dst, &uuid,
+ map_client_sdp_search_cb, dst, NULL, 0)) {
+ error("mce: Failed to search SDP details");
+ status = HAL_STATUS_FAILED;
+ free(dst);
+ }
+
+ status = HAL_STATUS_SUCCESS;
ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT,
- HAL_OP_MAP_CLIENT_GET_INSTANCES, HAL_STATUS_FAILED);
+ HAL_OP_MAP_CLIENT_GET_INSTANCES, status);
}
static const struct ipc_handler cmd_handlers[] = {
--
1.9.3
^ permalink raw reply related
* [PATCH 02/10] android/map-client: Add stubs for MAP client commands handlers
From: Grzegorz Kolodziejczyk @ 2014-10-09 12:45 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1412858714-2845-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Add empty handlers for MAP client IPC commands.
---
android/map-client.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/android/map-client.c b/android/map-client.c
index 4556461..1001b36 100644
--- a/android/map-client.c
+++ b/android/map-client.c
@@ -28,17 +28,48 @@
#include <stdbool.h>
#include <stdlib.h>
#include <stdint.h>
+#include <glib.h>
#include "ipc.h"
#include "lib/bluetooth.h"
#include "map-client.h"
+#include "src/log.h"
+#include "hal-msg.h"
+
+static struct ipc *hal_ipc = NULL;
+static bdaddr_t adapter_addr;
+
+static void handle_get_instances(const void *buf, uint16_t len)
+{
+ DBG("");
+
+ ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT,
+ HAL_OP_MAP_CLIENT_GET_INSTANCES, HAL_STATUS_FAILED);
+}
+
+static const struct ipc_handler cmd_handlers[] = {
+ {handle_get_instances, false,
+ sizeof(struct hal_cmd_map_client_get_instances)},
+};
bool bt_map_client_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
{
- return false;
+ DBG("");
+
+ bacpy(&adapter_addr, addr);
+
+ hal_ipc = ipc;
+
+ ipc_register(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT, cmd_handlers,
+ G_N_ELEMENTS(cmd_handlers));
+
+ return true;
}
void bt_map_client_unregister(void)
{
+ DBG("");
+ ipc_unregister(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT);
+ hal_ipc = NULL;
}
--
1.9.3
^ permalink raw reply related
* [PATCH 01/10] android/map-client: Add initial files
From: Grzegorz Kolodziejczyk @ 2014-10-09 12:45 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1412858714-2845-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
This adds initial daemon code for MAP client profile.
---
android/Android.mk | 1 +
android/Makefile.am | 1 +
android/main.c | 12 ++++++++++++
android/map-client.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
android/map-client.h | 26 ++++++++++++++++++++++++++
5 files changed, 84 insertions(+)
create mode 100644 android/map-client.c
create mode 100644 android/map-client.h
diff --git a/android/Android.mk b/android/Android.mk
index f25d60e..a9e5d4b 100644
--- a/android/Android.mk
+++ b/android/Android.mk
@@ -57,6 +57,7 @@ LOCAL_SRC_FILES := \
bluez/android/gatt.c \
bluez/android/health.c \
bluez/profiles/health/mcap.c \
+ bluez/android/map-client.c \
bluez/src/log.c \
bluez/src/shared/mgmt.c \
bluez/src/shared/util.c \
diff --git a/android/Makefile.am b/android/Makefile.am
index f11cff6..9279bbd 100644
--- a/android/Makefile.am
+++ b/android/Makefile.am
@@ -46,6 +46,7 @@ android_bluetoothd_SOURCES = android/main.c \
android/gatt.h android/gatt.c \
android/health.h android/health.c \
profiles/health/mcap.h profiles/health/mcap.c \
+ android/map-client.h android/map-client.c \
attrib/att.c attrib/att.h \
attrib/gatt.c attrib/gatt.h \
attrib/gattrib.c attrib/gattrib.h \
diff --git a/android/main.c b/android/main.c
index 703b3b6..b5f6937 100644
--- a/android/main.c
+++ b/android/main.c
@@ -62,6 +62,7 @@
#include "gatt.h"
#include "health.h"
#include "handsfree-client.h"
+#include "map-client.h"
#include "utils.h"
#define DEFAULT_VENDOR "BlueZ"
@@ -235,6 +236,14 @@ static void service_register(const void *buf, uint16_t len)
}
break;
+ case HAL_SERVICE_ID_MAP_CLIENT:
+ if (!bt_map_client_register(hal_ipc, &adapter_bdaddr,
+ m->mode)) {
+ status = HAL_STATUS_FAILED;
+ goto failed;
+ }
+
+ break;
default:
DBG("service %u not supported", m->service_id);
status = HAL_STATUS_FAILED;
@@ -288,6 +297,9 @@ static bool unregister_service(uint8_t id)
case HAL_SERVICE_ID_HANDSFREE_CLIENT:
bt_hf_client_unregister();
break;
+ case HAL_SERVICE_ID_MAP_CLIENT:
+ bt_map_client_unregister();
+ break;
default:
DBG("service %u not supported", id);
return false;
diff --git a/android/map-client.c b/android/map-client.c
new file mode 100644
index 0000000..4556461
--- /dev/null
+++ b/android/map-client.c
@@ -0,0 +1,44 @@
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; 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 <stdbool.h>
+#include <stdlib.h>
+#include <stdint.h>
+
+#include "ipc.h"
+#include "lib/bluetooth.h"
+#include "map-client.h"
+
+bool bt_map_client_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
+{
+ return false;
+}
+
+void bt_map_client_unregister(void)
+{
+
+}
diff --git a/android/map-client.h b/android/map-client.h
new file mode 100644
index 0000000..0e63072
--- /dev/null
+++ b/android/map-client.h
@@ -0,0 +1,26 @@
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+bool bt_map_client_register(struct ipc *ipc, const bdaddr_t *addr,
+ uint8_t mode);
+void bt_map_client_unregister(void);
--
1.9.3
^ permalink raw reply related
* [PATCH 00/10] android/map-client: Add MAP client daemon implementation
From: Grzegorz Kolodziejczyk @ 2014-10-09 12:45 UTC (permalink / raw)
To: linux-bluetooth
v1.
*Add MAP client daemon implementation (skeleton, body)
*Add haltest MAP client support
Aleksander Drewnicki (4):
android/client: Add skeleton for mce calls
android/client: Add code for mce callback
android/client: Add code for mce method
android/client: Add completion for mce method
Grzegorz Kolodziejczyk (6):
android/map-client: Add initial files
android/map-client: Add stubs for MAP client commands handlers
android/map-client: Add support for get remote MAS instances
android/ipc-tester: Add missing service opcode boundries test cases
android/ipc-tester: Add case for MAP client service opcode boundries
android/ipc-tester: Add cases for MAP client msg size
android/Android.mk | 5 +-
android/Makefile.am | 2 +
android/client/haltest.c | 2 +
android/client/if-bt.c | 3 +
android/client/if-main.h | 3 +
android/client/if-mce.c | 87 +++++++++++++++++++++
android/ipc-tester.c | 32 ++++++++
android/main.c | 12 +++
android/map-client.c | 197 +++++++++++++++++++++++++++++++++++++++++++++++
android/map-client.h | 26 +++++++
10 files changed, 368 insertions(+), 1 deletion(-)
create mode 100644 android/client/if-mce.c
create mode 100644 android/map-client.c
create mode 100644 android/map-client.h
--
1.9.3
^ permalink raw reply
* [PATCH] monitor: Fix indentation for AVRCP PASS THROUGH commands
From: Vikrampal Yadav @ 2014-10-09 12:13 UTC (permalink / raw)
To: linux-bluetooth; +Cc: luiz.dentz, d.kasatkin, vikram.pal, cpgs
Intendation for AVRCP PASS THROUGH commands' decoding fixed.
---
monitor/avctp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/monitor/avctp.c b/monitor/avctp.c
index a4e34c5..4abd18f 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -686,13 +686,13 @@ static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame,
if (!l2cap_frame_get_u8(frame, &op))
return false;
- print_field("%*cOperation: 0x%02x (%s %s)", (indent - 2), ' ', op,
+ print_field("%*cOperation: 0x%02x (%s %s)", (indent - 8), ' ', op,
op2str(op), op & 0x80 ? "Released" : "Pressed");
if (!l2cap_frame_get_u8(frame, &len))
return false;
- print_field("%*cLength: 0x%02x", (indent - 2), ' ', len);
+ print_field("%*cLength: 0x%02x", (indent - 8), ' ', len);
packet_hexdump(frame->data, frame->size);
return true;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] Monitor: Fix indentation for AVRCP PASS THROUGH commands
From: Luiz Augusto von Dentz @ 2014-10-09 11:58 UTC (permalink / raw)
To: Vikrampal Yadav; +Cc: linux-bluetooth@vger.kernel.org, Dmitry Kasatkin, cpgs
In-Reply-To: <1412855826-3665-1-git-send-email-vikram.pal@samsung.com>
Hi Vikram,
On Thu, Oct 9, 2014 at 2:57 PM, Vikrampal Yadav <vikram.pal@samsung.com> wrote:
> Intendation for AVRCP PASS THROUGH commands' decoding fixed.
Please use lower case at the beginning e.g. monitor:
> ---
> monitor/avctp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/avctp.c b/monitor/avctp.c
> index a4e34c5..4abd18f 100644
> --- a/monitor/avctp.c
> +++ b/monitor/avctp.c
> @@ -686,13 +686,13 @@ static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame,
> if (!l2cap_frame_get_u8(frame, &op))
> return false;
>
> - print_field("%*cOperation: 0x%02x (%s %s)", (indent - 2), ' ', op,
> + print_field("%*cOperation: 0x%02x (%s %s)", (indent - 8), ' ', op,
> op2str(op), op & 0x80 ? "Released" : "Pressed");
>
> if (!l2cap_frame_get_u8(frame, &len))
> return false;
>
> - print_field("%*cLength: 0x%02x", (indent - 2), ' ', len);
> + print_field("%*cLength: 0x%02x", (indent - 8), ' ', len);
>
> packet_hexdump(frame->data, frame->size);
> return true;
> --
> 1.9.1
Could you please start adding the output of the btmon to the
description once you add new parsers like this that way we can spot
more easily formatting bugs such as this.
--
Luiz Augusto von Dentz
^ permalink raw reply
* [PATCH] Monitor: Fix indentation for AVRCP PASS THROUGH commands
From: Vikrampal Yadav @ 2014-10-09 11:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: luiz.dentz, d.kasatkin, vikram.pal, cpgs
Intendation for AVRCP PASS THROUGH commands' decoding fixed.
---
monitor/avctp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/monitor/avctp.c b/monitor/avctp.c
index a4e34c5..4abd18f 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -686,13 +686,13 @@ static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame,
if (!l2cap_frame_get_u8(frame, &op))
return false;
- print_field("%*cOperation: 0x%02x (%s %s)", (indent - 2), ' ', op,
+ print_field("%*cOperation: 0x%02x (%s %s)", (indent - 8), ' ', op,
op2str(op), op & 0x80 ? "Released" : "Pressed");
if (!l2cap_frame_get_u8(frame, &len))
return false;
- print_field("%*cLength: 0x%02x", (indent - 2), ' ', len);
+ print_field("%*cLength: 0x%02x", (indent - 8), ' ', len);
packet_hexdump(frame->data, frame->size);
return true;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] bnep: Add extra debug information for bnep_up
From: Szymon Janc @ 2014-10-09 11:30 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1412849863-6691-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Thursday 09 of October 2014 13:17:43 Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Adding extra debug information helps to investigate failing cases
> ---
> profiles/network/bnep.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
> index 136709d..a6b8728 100644
> --- a/profiles/network/bnep.c
> +++ b/profiles/network/bnep.c
> @@ -210,7 +210,8 @@ static int bnep_if_up(const char *devname)
> close(sk);
>
> if (err < 0) {
> - error("Could not bring up %s", devname);
> + error("Could not bring up %s: %s(%d)", devname, strerror(errno),
> + errno);
> return err;
> }
You should use -err no errno here. Errno might be already overwritten by call
to close().
--
Best regards,
Szymon Janc
^ permalink raw reply
* Re: [PATCH] android/tester: Fix possible NULL pointer passing to function
From: Grzegorz Kolodziejczyk @ 2014-10-09 11:26 UTC (permalink / raw)
To: Andrei Emeltchenko, Grzegorz Kolodziejczyk, linux-bluetooth
In-Reply-To: <20141009112024.GB27705@aemeltch-MOBL1>
Hi Andrei,
On 9 October 2014 13:20, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> Hi Grzegorz,
>
> On Thu, Oct 09, 2014 at 12:56:19PM +0200, Grzegorz Kolodziejczyk wrote:
>> Hi Andrei,
>>
>> On 9 October 2014 12:16, Andrei Emeltchenko
>> <Andrei.Emeltchenko.news@gmail.com> wrote:
>> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> >
>> > Silence code analyzers and follow strict C standard where passing NULL
>> > pointer results in undefined behaviour.
>> > ---
>> > android/tester-main.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/android/tester-main.c b/android/tester-main.c
>> > index 30e1c59..ee3444f 100644
>> > --- a/android/tester-main.c
>> > +++ b/android/tester-main.c
>> > @@ -801,12 +801,12 @@ static bool match_data(struct step *step)
>> > return false;
>> > }
>> >
>> > - if (exp->store_srvc_handle)
>> > + if (exp->store_srvc_handle && step->callback_result.srvc_handle)
>> > memcpy(exp->store_srvc_handle,
>> > step->callback_result.srvc_handle,
>> > sizeof(*exp->store_srvc_handle));
>> >
>> > - if (exp->store_char_handle)
>> > + if (exp->store_char_handle && step->callback_result.char_handle)
>> > memcpy(exp->store_char_handle,
>> > step->callback_result.char_handle,
>> > sizeof(*exp->store_char_handle));
>> > --
>> > 1.9.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>> I think it can cause test cause bad behavior by not setting variable
>> if test step is obliged to do it.
>
> Do you mean that memcpy(src, 0, size) stores something useful?
>
I mean that memcpy(src, 0, size) shows us that something with test
step is wrong, during test case development it should be analyzed and
fixed.
It's only possible to expect storing srvc_handle while we get
service_added callback - this callback always returns service handle -
similar for characteristics.
I prefer to handle it in this way as I mentioned in previous comment.
> Best regards
> Andrei Emeltchenko
>
>> Test cases are defined in this way that one step e.g. 'A' stores
>> service handle and step 'B' uses this handle.
>> If value is not set it cause wrong test case behavior in next steps.
>> This check of exp (if store of srvc handle is mandatory for this step)
>> and step srvc handle (received in callback) only mask problem
>> with storing attribute handle value - in result it shows only that
>> memory was not duplicated well while copying data from incoming
>> callback handler.
>> Summarizing - we consciously force writing this srvc, char handle
>> value, if something is wrong - that means something with test
>> case is wrong and it should be fixed.
>>
>> Best way to handle it - static analyzer and this logic safe is to do
>> it in this way:
>> "if (exp->store_srvc_handle) {
>> if (!step->callback_result.srvc_handle) {
>> tester_debug("wrong srvc handle received in callback");
>> return false;
>> } else {
>> memcpy(exp->store_char_handle,
>> step->callback_result.char_handle,
>>
>> sizeof(*exp->store_char_handle));
>> }
>> }
>>
>> Best Regards,
>> Grzegorz
Best regards,
Grzegorz
^ permalink raw reply
* Re: [PATCH] android/tester: Fix possible NULL pointer passing to function
From: Andrei Emeltchenko @ 2014-10-09 11:20 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <CAMv5Gbd8kj0BbgSKL7BkComzpB4qLEXrNX1dCSb5oviHGoH1dA@mail.gmail.com>
Hi Grzegorz,
On Thu, Oct 09, 2014 at 12:56:19PM +0200, Grzegorz Kolodziejczyk wrote:
> Hi Andrei,
>
> On 9 October 2014 12:16, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Silence code analyzers and follow strict C standard where passing NULL
> > pointer results in undefined behaviour.
> > ---
> > android/tester-main.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/android/tester-main.c b/android/tester-main.c
> > index 30e1c59..ee3444f 100644
> > --- a/android/tester-main.c
> > +++ b/android/tester-main.c
> > @@ -801,12 +801,12 @@ static bool match_data(struct step *step)
> > return false;
> > }
> >
> > - if (exp->store_srvc_handle)
> > + if (exp->store_srvc_handle && step->callback_result.srvc_handle)
> > memcpy(exp->store_srvc_handle,
> > step->callback_result.srvc_handle,
> > sizeof(*exp->store_srvc_handle));
> >
> > - if (exp->store_char_handle)
> > + if (exp->store_char_handle && step->callback_result.char_handle)
> > memcpy(exp->store_char_handle,
> > step->callback_result.char_handle,
> > sizeof(*exp->store_char_handle));
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> I think it can cause test cause bad behavior by not setting variable
> if test step is obliged to do it.
Do you mean that memcpy(src, 0, size) stores something useful?
Best regards
Andrei Emeltchenko
> Test cases are defined in this way that one step e.g. 'A' stores
> service handle and step 'B' uses this handle.
> If value is not set it cause wrong test case behavior in next steps.
> This check of exp (if store of srvc handle is mandatory for this step)
> and step srvc handle (received in callback) only mask problem
> with storing attribute handle value - in result it shows only that
> memory was not duplicated well while copying data from incoming
> callback handler.
> Summarizing - we consciously force writing this srvc, char handle
> value, if something is wrong - that means something with test
> case is wrong and it should be fixed.
>
> Best way to handle it - static analyzer and this logic safe is to do
> it in this way:
> "if (exp->store_srvc_handle) {
> if (!step->callback_result.srvc_handle) {
> tester_debug("wrong srvc handle received in callback");
> return false;
> } else {
> memcpy(exp->store_char_handle,
> step->callback_result.char_handle,
>
> sizeof(*exp->store_char_handle));
> }
> }
>
> Best Regards,
> Grzegorz
^ permalink raw reply
* Re: [PATCH] android/tester: Fix possible NULL pointer passing to function
From: Grzegorz Kolodziejczyk @ 2014-10-09 10:56 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1412849812-6498-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On 9 October 2014 12:16, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Silence code analyzers and follow strict C standard where passing NULL
> pointer results in undefined behaviour.
> ---
> android/tester-main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/android/tester-main.c b/android/tester-main.c
> index 30e1c59..ee3444f 100644
> --- a/android/tester-main.c
> +++ b/android/tester-main.c
> @@ -801,12 +801,12 @@ static bool match_data(struct step *step)
> return false;
> }
>
> - if (exp->store_srvc_handle)
> + if (exp->store_srvc_handle && step->callback_result.srvc_handle)
> memcpy(exp->store_srvc_handle,
> step->callback_result.srvc_handle,
> sizeof(*exp->store_srvc_handle));
>
> - if (exp->store_char_handle)
> + if (exp->store_char_handle && step->callback_result.char_handle)
> memcpy(exp->store_char_handle,
> step->callback_result.char_handle,
> sizeof(*exp->store_char_handle));
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
I think it can cause test cause bad behavior by not setting variable
if test step is obliged to do it.
Test cases are defined in this way that one step e.g. 'A' stores
service handle and step 'B' uses this handle.
If value is not set it cause wrong test case behavior in next steps.
This check of exp (if store of srvc handle is mandatory for this step)
and step srvc handle (received in callback) only mask problem
with storing attribute handle value - in result it shows only that
memory was not duplicated well while copying data from incoming
callback handler.
Summarizing - we consciously force writing this srvc, char handle
value, if something is wrong - that means something with test
case is wrong and it should be fixed.
Best way to handle it - static analyzer and this logic safe is to do
it in this way:
"if (exp->store_srvc_handle) {
if (!step->callback_result.srvc_handle) {
tester_debug("wrong srvc handle received in callback");
return false;
} else {
memcpy(exp->store_char_handle,
step->callback_result.char_handle,
sizeof(*exp->store_char_handle));
}
}
Best Regards,
Grzegorz
^ permalink raw reply
* [PATCH] bnep: Add extra debug information for bnep_up
From: Andrei Emeltchenko @ 2014-10-09 10:17 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Adding extra debug information helps to investigate failing cases
---
profiles/network/bnep.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
index 136709d..a6b8728 100644
--- a/profiles/network/bnep.c
+++ b/profiles/network/bnep.c
@@ -210,7 +210,8 @@ static int bnep_if_up(const char *devname)
close(sk);
if (err < 0) {
- error("Could not bring up %s", devname);
+ error("Could not bring up %s: %s(%d)", devname, strerror(errno),
+ errno);
return err;
}
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function
From: Luiz Augusto von Dentz @ 2014-10-09 10:17 UTC (permalink / raw)
To: Gu, Chao Jie; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <3D02B219753AD44CBDDDE484323B17741130DDF2@SHSMSX104.ccr.corp.intel.com>
Hi,
On Thu, Oct 9, 2014 at 12:38 PM, Gu, Chao Jie <chao.jie.gu@intel.com> wrote:
> Hi Luiz,
>> >
>> >> > @@ -77,6 +78,16 @@ struct bt_att {
>> >> > bt_att_debug_func_t debug_callback;
>> >> > bt_att_destroy_func_t debug_destroy;
>> >> > void *debug_data;
>> >> > +
>> >> > + struct bt_crypto *crypto;
>> >> > +
>> >> > + bool valid_local_csrk;
>> >> > + uint8_t local_csrk[16];
>> >> > + uint32_t local_sign_cnt;
>> >> > +
>> >> > + bool valid_remote_csrk;
>> >> > + uint8_t remote_csrk[16];
>> >> > + uint32_t remote_sign_cnt;
>> >>
>> >> Maybe it is better to have pointers to a structs so you can free the
>> >> data once it becomes invalid and it also removes the necessity of
>> >> extra flags just to tell if the data is still valid since you can check if the pointer is
>> not NULL then it must be valid.
>> >
>> > This is good idea to remove the extra flags. However, we define the
>> > struct to do this which would result in some problem. Now we
>> > initialize the local_sign_cnt in the bt_att_new function and only add
>> > local_sign_cnt value when signed write command outgoing. Meanwile, we
>> > set local CSRK in the bt_gatt_client_set_local_csrk function when there is valid
>> CSRK provided. This two Parameter set at different time now.
>>
>> Well I guess this was your design, it doesn't have to be this way, in fact I believe
>> there is no point of initializing the counter if there is no valid CSRK because one
>> depend on the other, also perhaps you have a bug in bt_gatt_client_set_local_csrk
>> since I believe every time you set CSRK you should reset the counter otherwise you
>> may be carrying the counters from the past CSRK which probably won't work.
>
> You said condition resuilt in bug which I want to avoid, because we use this command line
> By btgatt-client tool
> Write-value -w -c <CSRK> <value_handle> <value>
> If user write signed command twice, it would call bt_gatt_client_set_local_csrk twice.
> So it will restore sign_cnt, that's why I initialize the sign_cnt in bt_att_new to avoid this bug.
Considering what the spec says about SignCounter and that we do in
fact store the counter persistently I believe the counter is valid as
long as CSRK is valid, which means it is not per connection and
initializing it on bt_att_new is probably wrong since the counter will
probably be out of sync. We should either create a dedicate command to
set CSRK and the SignCounter or make write-value to take the counter
as well otherwise it wont work except with a fresh CSRK and
SignCounter which perhaps is the case of btgatt-client but not if we
want to reuse in the daemons.
> In fact, the only way is that we should tell user set CSRK once if CSRK did not change to
> resolve this problem.
>
>> Also don't to have to pass the counter when setting the key, the counter probably
>> need to be restored on every connection so bt_gatt_client_set_local_csrk and
>> bt_att_set_remote_csrk probably needs to take the counter and instead of having a
>> valid flag you can probably reset by setting NULL as key or an invalid counter if one
>> exists.
>
> If we define the new struct naming signed_info to store csrk and sign_cnt, what's your point?
>
>>
>> > If we have pointers to a struct including local_csrk and
>> > local_signd_cnt, we initialize the local_sign_cnt meanwhile the struct should not
>> be NULL because of incuding it. In fact, local_csrk is not valid at this time.
>> > If we initialize the local_signed_cnt when CSRK is valid , it would be
>> > risk to change local_signed_cnt by user calling bt_gatt_client_set_local_csrk.
>> >> > };
>> >> >
>> >> > enum att_op_type {
>> >> > @@ -176,6 +187,8 @@ struct att_send_op {
>> >> > bt_att_response_func_t callback;
>> >> > bt_att_destroy_func_t destroy;
>> >> > void *user_data;
>> >> > +
>> >> > + struct bt_att *att;
>> >> > };
>> >> >
>> >> > static void destroy_att_send_op(void *data) @@ -277,6 +290,10 @@
>> >> > static bool encode_pdu(struct att_send_op *op, const void *pdu,
>> >> > uint16_t length,
>> >> > uint16_t mtu) {
>> >> > uint16_t pdu_len = 1;
>> >> > + struct bt_att *att = op->att;
>> >> > +
>> >> > + if (op->opcode & ATT_OP_SIGNED_MASK)
>> >> > + pdu_len += BT_ATT_SIGNATURE_LEN;
>> >> >
>> >> > if (length && pdu)
>> >> > pdu_len += length;
>> >> > @@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op
>> >> > *op, const
>> >> void *pdu,
>> >> > if (pdu_len > 1)
>> >> > memcpy(op->pdu + 1, pdu, length);
>> >> >
>> >> > + if (op->opcode & ATT_OP_SIGNED_MASK) {
>> >> > + if (bt_crypto_sign_att(att->crypto,
>> >> > + att->local_csrk,
>> >> > + op->pdu,
>> >> > + 1 + length,
>> >> > + att->local_sign_cnt,
>> >> > + &((uint8_t *) op->pdu)[1 +
>> >> length]))
>> >> > + att->local_sign_cnt++;
>> >> > + else
>> >> > + return false;
>> >> > + }
>> >> > +
>> >>
>> >> You can probably bail out early if you check !(op->opcode &
>> >> ATT_OP_SIGNED_MASK), also perhaps it is a good idea to start defining
>> >> structs for the PDUs to simplify the access.
>> >>
>> > If we bail out this checking early , maybe we have to need two
>> > different encode_pdu function to implement it such as naming new
>> encode_signed_pdu function to do this.
>>
>> But you return true anyway after this code, no idea why you need a new function for
>> that, all I was suggesting is the following:
>>
>> if (!(op->opcode & ATT_OP_SIGNED_MASK))
>> return true;
>>
>> if (!bt_crypto_sign_att...)
>> return false;
>>
>> att->local_sign_cnt++;
>>
>> return true;
>>
>
> I got your meaning now. I have returned false when bt_crypto_sign_att. Failed
>
> + else
> + return false;
>
> Your proposal is good so as to make code more clearer. I would accept that.
>
> Thanks
> Chaojie Gu
--
Luiz Augusto von Dentz
^ permalink raw reply
* [PATCH] android/tester: Fix possible NULL pointer passing to function
From: Andrei Emeltchenko @ 2014-10-09 10:16 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Silence code analyzers and follow strict C standard where passing NULL
pointer results in undefined behaviour.
---
android/tester-main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/android/tester-main.c b/android/tester-main.c
index 30e1c59..ee3444f 100644
--- a/android/tester-main.c
+++ b/android/tester-main.c
@@ -801,12 +801,12 @@ static bool match_data(struct step *step)
return false;
}
- if (exp->store_srvc_handle)
+ if (exp->store_srvc_handle && step->callback_result.srvc_handle)
memcpy(exp->store_srvc_handle,
step->callback_result.srvc_handle,
sizeof(*exp->store_srvc_handle));
- if (exp->store_char_handle)
+ if (exp->store_char_handle && step->callback_result.char_handle)
memcpy(exp->store_char_handle,
step->callback_result.char_handle,
sizeof(*exp->store_char_handle));
--
1.9.1
^ permalink raw reply related
* RE: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function
From: Gu, Chao Jie @ 2014-10-09 9:38 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZLtwXrN1tYH+5vvPrcYq8Xaa5mdwba6vyQMvC0_s4Z0Cg@mail.gmail.com>
SGkgTHVpeiwNCj4gPg0KPiA+PiA+IEBAIC03Nyw2ICs3OCwxNiBAQCBzdHJ1Y3QgYnRfYXR0IHsN
Cj4gPj4gPiAgICAgICAgIGJ0X2F0dF9kZWJ1Z19mdW5jX3QgZGVidWdfY2FsbGJhY2s7DQo+ID4+
ID4gICAgICAgICBidF9hdHRfZGVzdHJveV9mdW5jX3QgZGVidWdfZGVzdHJveTsNCj4gPj4gPiAg
ICAgICAgIHZvaWQgKmRlYnVnX2RhdGE7DQo+ID4+ID4gKw0KPiA+PiA+ICsgICAgICAgc3RydWN0
IGJ0X2NyeXB0byAqY3J5cHRvOw0KPiA+PiA+ICsNCj4gPj4gPiArICAgICAgIGJvb2wgdmFsaWRf
bG9jYWxfY3NyazsNCj4gPj4gPiArICAgICAgIHVpbnQ4X3QgbG9jYWxfY3Nya1sxNl07DQo+ID4+
ID4gKyAgICAgICB1aW50MzJfdCBsb2NhbF9zaWduX2NudDsNCj4gPj4gPiArDQo+ID4+ID4gKyAg
ICAgICBib29sIHZhbGlkX3JlbW90ZV9jc3JrOw0KPiA+PiA+ICsgICAgICAgdWludDhfdCByZW1v
dGVfY3Nya1sxNl07DQo+ID4+ID4gKyAgICAgICB1aW50MzJfdCByZW1vdGVfc2lnbl9jbnQ7DQo+
ID4+DQo+ID4+IE1heWJlIGl0IGlzIGJldHRlciB0byBoYXZlIHBvaW50ZXJzIHRvIGEgc3RydWN0
cyBzbyB5b3UgY2FuIGZyZWUgdGhlDQo+ID4+IGRhdGEgb25jZSBpdCBiZWNvbWVzIGludmFsaWQg
YW5kIGl0IGFsc28gcmVtb3ZlcyB0aGUgbmVjZXNzaXR5IG9mDQo+ID4+IGV4dHJhIGZsYWdzIGp1
c3QgdG8gdGVsbCBpZiB0aGUgZGF0YSBpcyBzdGlsbCB2YWxpZCBzaW5jZSB5b3UgY2FuIGNoZWNr
IGlmIHRoZSBwb2ludGVyIGlzDQo+IG5vdCBOVUxMIHRoZW4gaXQgbXVzdCBiZSB2YWxpZC4NCj4g
Pg0KPiA+IFRoaXMgaXMgZ29vZCBpZGVhIHRvIHJlbW92ZSB0aGUgZXh0cmEgZmxhZ3MuIEhvd2V2
ZXIsIHdlIGRlZmluZSB0aGUNCj4gPiBzdHJ1Y3QgdG8gZG8gdGhpcyB3aGljaCB3b3VsZCByZXN1
bHQgaW4gc29tZSBwcm9ibGVtLiBOb3cgd2UNCj4gPiBpbml0aWFsaXplIHRoZSBsb2NhbF9zaWdu
X2NudCBpbiB0aGUgYnRfYXR0X25ldyBmdW5jdGlvbiBhbmQgb25seSBhZGQNCj4gPiBsb2NhbF9z
aWduX2NudCB2YWx1ZSB3aGVuIHNpZ25lZCB3cml0ZSBjb21tYW5kIG91dGdvaW5nLiBNZWFud2ls
ZSwgd2UNCj4gPiBzZXQgbG9jYWwgQ1NSSyBpbiB0aGUgYnRfZ2F0dF9jbGllbnRfc2V0X2xvY2Fs
X2NzcmsgZnVuY3Rpb24gd2hlbiB0aGVyZSBpcyB2YWxpZA0KPiBDU1JLIHByb3ZpZGVkLiBUaGlz
IHR3byBQYXJhbWV0ZXIgc2V0IGF0IGRpZmZlcmVudCB0aW1lIG5vdy4NCj4gDQo+IFdlbGwgSSBn
dWVzcyB0aGlzIHdhcyB5b3VyIGRlc2lnbiwgaXQgZG9lc24ndCBoYXZlIHRvIGJlIHRoaXMgd2F5
LCBpbiBmYWN0IEkgYmVsaWV2ZQ0KPiB0aGVyZSBpcyBubyBwb2ludCBvZiBpbml0aWFsaXppbmcg
dGhlIGNvdW50ZXIgaWYgdGhlcmUgaXMgbm8gdmFsaWQgQ1NSSyBiZWNhdXNlIG9uZQ0KPiBkZXBl
bmQgb24gdGhlIG90aGVyLCBhbHNvIHBlcmhhcHMgeW91IGhhdmUgYSBidWcgaW4gYnRfZ2F0dF9j
bGllbnRfc2V0X2xvY2FsX2NzcmsNCj4gc2luY2UgSSBiZWxpZXZlIGV2ZXJ5IHRpbWUgeW91IHNl
dCBDU1JLIHlvdSBzaG91bGQgcmVzZXQgdGhlIGNvdW50ZXIgb3RoZXJ3aXNlIHlvdQ0KPiBtYXkg
YmUgY2FycnlpbmcgdGhlIGNvdW50ZXJzIGZyb20gdGhlIHBhc3QgQ1NSSyB3aGljaCBwcm9iYWJs
eSB3b24ndCB3b3JrLg0KDQpZb3Ugc2FpZCBjb25kaXRpb24gcmVzdWlsdCBpbiBidWcgd2hpY2gg
SSB3YW50IHRvIGF2b2lkLCBiZWNhdXNlIHdlIHVzZSB0aGlzIGNvbW1hbmQgbGluZQ0KQnkgYnRn
YXR0LWNsaWVudCB0b29sDQpXcml0ZS12YWx1ZSAtdyAtYyA8Q1NSSz4gPHZhbHVlX2hhbmRsZT4g
PHZhbHVlPg0KSWYgdXNlciB3cml0ZSBzaWduZWQgY29tbWFuZCB0d2ljZSwgaXQgd291bGQgY2Fs
bCBidF9nYXR0X2NsaWVudF9zZXRfbG9jYWxfY3NyayB0d2ljZS4gDQpTbyBpdCB3aWxsIHJlc3Rv
cmUgc2lnbl9jbnQsIHRoYXQncyB3aHkgSSBpbml0aWFsaXplIHRoZSBzaWduX2NudCBpbiBidF9h
dHRfbmV3IHRvIGF2b2lkIHRoaXMgYnVnLg0KDQpJbiBmYWN0LCB0aGUgb25seSB3YXkgaXMgdGhh
dCB3ZSBzaG91bGQgdGVsbCB1c2VyIHNldCBDU1JLIG9uY2UgaWYgQ1NSSyBkaWQgbm90IGNoYW5n
ZSB0byANCnJlc29sdmUgdGhpcyBwcm9ibGVtLg0KIA0KPiBBbHNvIGRvbid0IHRvIGhhdmUgdG8g
cGFzcyB0aGUgY291bnRlciB3aGVuIHNldHRpbmcgdGhlIGtleSwgdGhlIGNvdW50ZXIgcHJvYmFi
bHkNCj4gbmVlZCB0byBiZSByZXN0b3JlZCBvbiBldmVyeSBjb25uZWN0aW9uIHNvIGJ0X2dhdHRf
Y2xpZW50X3NldF9sb2NhbF9jc3JrIGFuZA0KPiBidF9hdHRfc2V0X3JlbW90ZV9jc3JrIHByb2Jh
Ymx5IG5lZWRzIHRvIHRha2UgdGhlIGNvdW50ZXIgYW5kIGluc3RlYWQgb2YgaGF2aW5nIGENCj4g
dmFsaWQgZmxhZyB5b3UgY2FuIHByb2JhYmx5IHJlc2V0IGJ5IHNldHRpbmcgTlVMTCBhcyBrZXkg
b3IgYW4gaW52YWxpZCBjb3VudGVyIGlmIG9uZQ0KPiBleGlzdHMuDQoNCklmIHdlIGRlZmluZSB0
aGUgbmV3IHN0cnVjdCBuYW1pbmcgc2lnbmVkX2luZm8gdG8gc3RvcmUgY3NyayBhbmQgc2lnbl9j
bnQsIHdoYXQncyB5b3VyIHBvaW50Pw0KDQo+IA0KPiA+IElmIHdlIGhhdmUgcG9pbnRlcnMgdG8g
YSBzdHJ1Y3QgaW5jbHVkaW5nIGxvY2FsX2NzcmsgYW5kDQo+ID4gbG9jYWxfc2lnbmRfY250LCB3
ZSBpbml0aWFsaXplIHRoZSBsb2NhbF9zaWduX2NudCBtZWFud2hpbGUgdGhlIHN0cnVjdCBzaG91
bGQgbm90DQo+IGJlIE5VTEwgYmVjYXVzZSBvZiBpbmN1ZGluZyBpdC4gSW4gZmFjdCwgbG9jYWxf
Y3NyayBpcyBub3QgdmFsaWQgYXQgdGhpcyB0aW1lLg0KPiA+IElmIHdlIGluaXRpYWxpemUgdGhl
IGxvY2FsX3NpZ25lZF9jbnQgd2hlbiBDU1JLIGlzIHZhbGlkICwgaXQgd291bGQgYmUNCj4gPiBy
aXNrIHRvIGNoYW5nZSBsb2NhbF9zaWduZWRfY250IGJ5IHVzZXIgY2FsbGluZyBidF9nYXR0X2Ns
aWVudF9zZXRfbG9jYWxfY3Nyay4NCj4gPj4gPiAgfTsNCj4gPj4gPg0KPiA+PiA+ICBlbnVtIGF0
dF9vcF90eXBlIHsNCj4gPj4gPiBAQCAtMTc2LDYgKzE4Nyw4IEBAIHN0cnVjdCBhdHRfc2VuZF9v
cCB7DQo+ID4+ID4gICAgICAgICBidF9hdHRfcmVzcG9uc2VfZnVuY190IGNhbGxiYWNrOw0KPiA+
PiA+ICAgICAgICAgYnRfYXR0X2Rlc3Ryb3lfZnVuY190IGRlc3Ryb3k7DQo+ID4+ID4gICAgICAg
ICB2b2lkICp1c2VyX2RhdGE7DQo+ID4+ID4gKw0KPiA+PiA+ICsgICAgICAgc3RydWN0IGJ0X2F0
dCAqYXR0Ow0KPiA+PiA+ICB9Ow0KPiA+PiA+DQo+ID4+ID4gIHN0YXRpYyB2b2lkIGRlc3Ryb3lf
YXR0X3NlbmRfb3Aodm9pZCAqZGF0YSkgQEAgLTI3Nyw2ICsyOTAsMTAgQEANCj4gPj4gPiBzdGF0
aWMgYm9vbCBlbmNvZGVfcGR1KHN0cnVjdCBhdHRfc2VuZF9vcCAqb3AsIGNvbnN0IHZvaWQgKnBk
dSwNCj4gPj4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICB1aW50MTZfdCBsZW5ndGgsDQo+ID4+ID4gdWludDE2X3QgbXR1KSAgew0KPiA+PiA+ICAgICAg
ICAgdWludDE2X3QgcGR1X2xlbiA9IDE7DQo+ID4+ID4gKyAgICAgICBzdHJ1Y3QgYnRfYXR0ICph
dHQgPSBvcC0+YXR0Ow0KPiA+PiA+ICsNCj4gPj4gPiArICAgICAgIGlmIChvcC0+b3Bjb2RlICYg
QVRUX09QX1NJR05FRF9NQVNLKQ0KPiA+PiA+ICsgICAgICAgICAgICAgICBwZHVfbGVuICs9IEJU
X0FUVF9TSUdOQVRVUkVfTEVOOw0KPiA+PiA+DQo+ID4+ID4gICAgICAgICBpZiAobGVuZ3RoICYm
IHBkdSkNCj4gPj4gPiAgICAgICAgICAgICAgICAgcGR1X2xlbiArPSBsZW5ndGg7DQo+ID4+ID4g
QEAgLTI5MywxNyArMzEwLDMyIEBAIHN0YXRpYyBib29sIGVuY29kZV9wZHUoc3RydWN0IGF0dF9z
ZW5kX29wDQo+ID4+ID4gKm9wLCBjb25zdA0KPiA+PiB2b2lkICpwZHUsDQo+ID4+ID4gICAgICAg
ICBpZiAocGR1X2xlbiA+IDEpDQo+ID4+ID4gICAgICAgICAgICAgICAgIG1lbWNweShvcC0+cGR1
ICsgMSwgcGR1LCBsZW5ndGgpOw0KPiA+PiA+DQo+ID4+ID4gKyAgICAgICBpZiAob3AtPm9wY29k
ZSAmIEFUVF9PUF9TSUdORURfTUFTSykgew0KPiA+PiA+ICsgICAgICAgICAgICAgICBpZiAoYnRf
Y3J5cHRvX3NpZ25fYXR0KGF0dC0+Y3J5cHRvLA0KPiA+PiA+ICsgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICBhdHQtPmxvY2FsX2NzcmssDQo+ID4+ID4gKyAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIG9wLT5wZHUsDQo+ID4+ID4gKyAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIDEgKyBsZW5ndGgsDQo+ID4+ID4gKyAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGF0dC0+bG9jYWxfc2lnbl9jbnQsDQo+ID4+
ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICYoKHVpbnQ4X3QgKikg
b3AtPnBkdSlbMSArDQo+ID4+IGxlbmd0aF0pKQ0KPiA+PiA+ICsgICAgICAgICAgICAgICAgICAg
ICAgIGF0dC0+bG9jYWxfc2lnbl9jbnQrKzsNCj4gPj4gPiArICAgICAgICAgICAgICAgZWxzZQ0K
PiA+PiA+ICsgICAgICAgICAgICAgICAgICAgICAgIHJldHVybiBmYWxzZTsNCj4gPj4gPiArICAg
ICAgIH0NCj4gPj4gPiArDQo+ID4+DQo+ID4+IFlvdSBjYW4gcHJvYmFibHkgYmFpbCBvdXQgZWFy
bHkgaWYgeW91IGNoZWNrICEob3AtPm9wY29kZSAmDQo+ID4+IEFUVF9PUF9TSUdORURfTUFTSyks
IGFsc28gcGVyaGFwcyBpdCBpcyBhIGdvb2QgaWRlYSB0byBzdGFydCBkZWZpbmluZw0KPiA+PiBz
dHJ1Y3RzIGZvciB0aGUgUERVcyB0byBzaW1wbGlmeSB0aGUgYWNjZXNzLg0KPiA+Pg0KPiA+IElm
IHdlIGJhaWwgb3V0IHRoaXMgY2hlY2tpbmcgZWFybHkgLCBtYXliZSB3ZSBoYXZlIHRvIG5lZWQg
dHdvDQo+ID4gZGlmZmVyZW50IGVuY29kZV9wZHUgZnVuY3Rpb24gdG8gaW1wbGVtZW50IGl0IHN1
Y2ggYXMgbmFtaW5nIG5ldw0KPiBlbmNvZGVfc2lnbmVkX3BkdSBmdW5jdGlvbiB0byBkbyB0aGlz
Lg0KPiANCj4gQnV0IHlvdSByZXR1cm4gdHJ1ZSBhbnl3YXkgYWZ0ZXIgdGhpcyBjb2RlLCBubyBp
ZGVhIHdoeSB5b3UgbmVlZCBhIG5ldyBmdW5jdGlvbiBmb3INCj4gdGhhdCwgYWxsIEkgd2FzIHN1
Z2dlc3RpbmcgaXMgdGhlIGZvbGxvd2luZzoNCj4gDQo+IGlmICghKG9wLT5vcGNvZGUgJiBBVFRf
T1BfU0lHTkVEX01BU0spKQ0KPiAgICAgcmV0dXJuIHRydWU7DQo+IA0KPiBpZiAoIWJ0X2NyeXB0
b19zaWduX2F0dC4uLikNCj4gICAgIHJldHVybiBmYWxzZTsNCj4gDQo+IGF0dC0+bG9jYWxfc2ln
bl9jbnQrKzsNCj4gDQo+IHJldHVybiB0cnVlOw0KPiANCg0KSSBnb3QgeW91ciBtZWFuaW5nIG5v
dy4gSSBoYXZlIHJldHVybmVkIGZhbHNlIHdoZW4gYnRfY3J5cHRvX3NpZ25fYXR0LiBGYWlsZWQN
Cg0KICsgICAgICAgICAgICAgICBlbHNlDQogKyAgICAgICAgICAgICAgICAgICAgICAgcmV0dXJu
IGZhbHNlOw0KDQpZb3VyIHByb3Bvc2FsIGlzIGdvb2Qgc28gYXMgdG8gbWFrZSBjb2RlIG1vcmUg
Y2xlYXJlci4gSSB3b3VsZCBhY2NlcHQgdGhhdC4NCg0KVGhhbmtzDQpDaGFvamllIEd1DQo=
^ permalink raw reply
* Re: [PATCH BlueZ v1 0/5] Introduce shared/gatt-server.
From: Luiz Augusto von Dentz @ 2014-10-09 8:29 UTC (permalink / raw)
To: Arman Uguray; +Cc: BlueZ development
In-Reply-To: <CAHrH25TbsEmz3qdQTLZdcqFx8=dRRqpcivT2mPM+xWEOi0HFyA@mail.gmail.com>
Hi Arman,
On Wed, Oct 8, 2014 at 6:14 PM, Arman Uguray <armansito@chromium.org> wrote:
> On Mon, Oct 6, 2014 at 5:01 PM, Arman Uguray <armansito@chromium.org> wrote:
>> *v1:
>> - Make gatt-db external to gatt-server, as initially discussed.
>> - Also implement Read By Group Type request.
>>
>> Arman Uguray (5):
>> Revert "plugins/sixaxis: Add a set_leds_sysfs() function"
>> shared/att: Drop the connection is a request is received while one is
>> pending.
>> shared/gatt-server: Introduce shared/gatt-server.
>> shared/gatt-server: Support Exchange MTU request.
>> shared/gatt-server: Support Read By Group Type request.
>>
>> Makefile.am | 1 +
>> configure.ac | 4 +-
>> plugins/sixaxis.c | 89 +-----------
>> src/shared/att.c | 101 ++++++++------
>> src/shared/gatt-server.c | 347 +++++++++++++++++++++++++++++++++++++++++++++++
>> src/shared/gatt-server.h | 40 ++++++
>> 6 files changed, 455 insertions(+), 127 deletions(-)
>> create mode 100644 src/shared/gatt-server.c
>> create mode 100644 src/shared/gatt-server.h
>>
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>
> ping.
>
> Also ignore the bit about 'Revert "plugins/sixaxis: Add a
> set_leds_sysfs() function"'. I revert it locally since it gives me
> some dependency issues when compiling for Chromium OS; it's not part
> of the patch set I uploaded.
Apart from the comments of the last patch Im fine with this set.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ v1 4/4] shared/gatt-server: Support Read By Group Type request.
From: Luiz Augusto von Dentz @ 2014-10-09 8:24 UTC (permalink / raw)
To: Arman Uguray; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1412629304-3391-5-git-send-email-armansito@chromium.org>
Hi Arman,
On Tue, Oct 7, 2014 at 12:01 AM, Arman Uguray <armansito@chromium.org> wrote:
> This patch adds handling for the Read By Group Type request.
> ---
> src/shared/gatt-server.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 169 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index c7b0873..3ea8c59 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -45,6 +45,7 @@ struct bt_gatt_server {
> uint16_t mtu;
>
> unsigned int mtu_id;
> + unsigned int read_by_grp_type_id;
>
> bt_gatt_server_debug_func_t debug_callback;
> bt_gatt_server_destroy_func_t debug_destroy;
> @@ -54,6 +55,7 @@ struct bt_gatt_server {
> static void gatt_server_unregister_handlers(struct bt_gatt_server *server)
> {
> bt_att_unregister(server->att, server->mtu_id);
> + bt_att_unregister(server->att, server->read_by_grp_type_id);
> }
>
> static void gatt_server_cleanup(struct bt_gatt_server *server)
> @@ -71,6 +73,157 @@ static void encode_error_rsp(uint8_t opcode, uint16_t handle, uint8_t ecode,
> put_le16(handle, pdu + 1);
> }
>
> +static bool get_uuid_le(const uint8_t *uuid, size_t len, bt_uuid_t *out_uuid)
> +{
> + uint128_t u128;
> +
> + switch (len) {
> + case 2:
> + bt_uuid16_create(out_uuid, get_le16(uuid));
> + return true;
> + case 16:
> + bswap_128(uuid, &u128.data);
> + bt_uuid128_create(out_uuid, u128);
> + return true;
> + default:
> + return false;
> + }
> +
> + return false;
> +}
> +
> +static void read_by_grp_type_cb(uint8_t opcode, const void *pdu,
> + uint16_t length, void *user_data)
> +{
> + struct bt_gatt_server *server = user_data;
> + uint16_t start, end;
> + bt_uuid_t type;
> + bt_uuid_t prim, snd;
> + uint16_t mtu = bt_att_get_mtu(server->att);
> + uint8_t rsp_pdu[mtu];
> + uint16_t rsp_len;
> + uint8_t rsp_opcode;
> + uint8_t ecode = 0;
> + uint16_t ehandle = 0;
> + struct queue *q = NULL;
> + int iter = 0;
> + uint8_t data_val_len;
> +
> + if (length != 6 && length != 20) {
> + ecode = BT_ATT_ERROR_INVALID_PDU;
> + goto error;
> + }
> +
> + q = queue_new();
> + if (!q) {
> + ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES;
> + goto error;
> + }
> +
> + start = get_le16(pdu);
> + end = get_le16(pdu + 2);
> + get_uuid_le(pdu + 4, length - 4, &type);
> +
> + util_debug(server->debug_callback, server->debug_data,
> + "Read By Grp Type - start: 0x%04x end: 0x%04x",
> + start, end);
> +
> + if (!start || !end) {
> + ecode = BT_ATT_ERROR_INVALID_HANDLE;
> + goto error;
> + }
> +
> + ehandle = start;
> +
> + if (start > end) {
> + ecode = BT_ATT_ERROR_INVALID_HANDLE;
> + goto error;
> + }
> +
> + /* GATT defines that only the <<Primary Service>> and
> + * <<Secondary Service>> group types can be used for the
> + * "Read By Group Type" request (Core v4.1, Vol 3, sec 2.5.3). Return an
> + * error if any other group type is given.
> + */
> + bt_uuid16_create(&prim, GATT_PRIM_SVC_UUID);
> + bt_uuid16_create(&snd, GATT_SND_SVC_UUID);
> + if (bt_uuid_cmp(&type, &prim) && bt_uuid_cmp(&type, &snd)) {
> + ecode = BT_ATT_ERROR_UNSUPPORTED_GROUP_TYPE;
> + goto error;
> + }
> +
> + gatt_db_read_by_group_type(server->db, start, end, type, q);
> +
> + if (queue_isempty(q)) {
> + ecode = BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND;
> + goto error;
> + }
This function is quite long, perhaps using queue_foreach be better
than iterating inline since you destroy the queue at end but it would
be a problem if one of items fails to be encoded, the funny thing is
that it is possible to do the same with queue_find and return true to
interrupt in case of failure but it would be a obvious abuse of the
API so perhaps queue_foreach_func_t could have a return as well and in
case it return false we stop.
> + rsp_len = 0;
> + while (queue_peek_head(q)) {
> + uint16_t start_handle = PTR_TO_UINT(queue_pop_head(q));
> + uint16_t end_handle;
> + uint8_t *value = NULL;
> + int value_len = 0;
> +
> + /* TODO: Figure out how to check permissions based on security
> + * level on a bt_att, which sits on a generic io.
> + */
> +
> + if (!gatt_db_read(server->db, start_handle, 0, opcode, NULL,
> + &value, &value_len)) {
> + ecode = BT_ATT_ERROR_UNLIKELY;
> + goto error;
> + }
> +
> + if (value_len < 0) {
> + /* This should never happen for primary/secondary
> + * service declarations.
> + */
> + ecode = BT_ATT_ERROR_UNLIKELY;
> + goto error;
> + }
> +
> + /* Use the first attribute to determine the length of each
> + * attribute data unit. Stop the list when a different attribute
> + * value is seen.
> + */
> + if (iter == 0) {
> + data_val_len = value_len;
> + rsp_pdu[0] = data_val_len + 4;
> + iter++;
> + } else if (value_len != data_val_len)
> + break;
> +
> + /* Stop if this unit would surpass the MTU */
> + if (iter + data_val_len + 4 > mtu)
> + break;
> +
> + end_handle = gatt_db_get_end_handle(server->db, start_handle);
> +
> + put_le16(start_handle, rsp_pdu + iter);
> + put_le16(end_handle, rsp_pdu + iter + 2);
> + memcpy(rsp_pdu + iter + 4, value, value_len);
> +
> + iter += data_val_len + 4;
> + }
> +
> + rsp_opcode = BT_ATT_OP_READ_BY_GRP_TYPE_RSP;
> + rsp_len = iter;
> +
> + goto done;
> +
> +error:
> + rsp_opcode = BT_ATT_OP_ERROR_RSP;
> + rsp_len = 4;
> + encode_error_rsp(opcode, ehandle, ecode, rsp_pdu);
> +
> +done:
> + queue_destroy(q, NULL);
> + bt_att_send(server->att, rsp_opcode, rsp_pdu, rsp_len,
> + NULL, NULL, NULL);
> +}
> +
> static void exchange_mtu_cb(uint8_t opcode, const void *pdu,
> uint16_t length, void *user_data)
> {
> @@ -97,18 +250,33 @@ static void exchange_mtu_cb(uint8_t opcode, const void *pdu,
> /* Set MTU to be the minimum */
> server->mtu = final_mtu;
> bt_att_set_mtu(server->att, final_mtu);
> +
> + util_debug(server->debug_callback, server->debug_data,
> + "MTU exchange complete, with MTU: %u", final_mtu);
> }
>
> static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
> {
> - /* EXCHANGE MTU request */
> + /* Exchange MTU */
> server->mtu_id = bt_att_register(server->att, BT_ATT_OP_MTU_REQ,
> exchange_mtu_cb,
> server, NULL);
> if (!server->mtu_id)
> return false;
>
> + /* Read By Group Type */
> + server->read_by_grp_type_id = bt_att_register(server->att,
> + BT_ATT_OP_READ_BY_GRP_TYPE_REQ,
> + read_by_grp_type_cb,
> + server, NULL);
> + if (!server->read_by_grp_type_id)
> + goto fail;
> +
> return true;
> +
> +fail:
> + gatt_server_unregister_handlers(server);
> + return false;
> }
>
> struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
> --
> 2.1.0.rc2.206.gedb03e5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ v1 2/4] shared/gatt-server: Introduce shared/gatt-server.
From: Marcin Kraglak @ 2014-10-09 8:18 UTC (permalink / raw)
To: Arman Uguray; +Cc: linux-bluetooth@vger.kernel.org development
In-Reply-To: <1412629304-3391-3-git-send-email-armansito@chromium.org>
Hi Arman,
On 6 October 2014 23:01, Arman Uguray <armansito@chromium.org> wrote:
> This patch introduces bt_gatt_server which will implement the server-side of the
> ATT protocol over a bt_att structure and construct responses based on a gatt_db
> structure.
> ---
> Makefile.am | 1 +
> src/shared/gatt-server.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++
> src/shared/gatt-server.h | 40 +++++++++++++++
> 3 files changed, 166 insertions(+)
> create mode 100644 src/shared/gatt-server.c
> create mode 100644 src/shared/gatt-server.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 1a1a43f..2dfea28 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -114,6 +114,7 @@ shared_sources = src/shared/io.h src/shared/timeout.h \
> src/shared/att.h src/shared/att.c \
> src/shared/gatt-helpers.h src/shared/gatt-helpers.c \
> src/shared/gatt-client.h src/shared/gatt-client.c \
> + src/shared/gatt-server.h src/shared/gatt-server.c \
> src/shared/gatt-db.h src/shared/gatt-db.c \
> src/shared/gap.h src/shared/gap.c
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> new file mode 100644
> index 0000000..544e60e
> --- /dev/null
> +++ b/src/shared/gatt-server.c
> @@ -0,0 +1,125 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2014 Google Inc.
> + *
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#include "src/shared/att.h"
> +#include "lib/uuid.h"
> +#include "src/shared/queue.h"
> +#include "src/shared/gatt-db.h"
> +#include "src/shared/gatt-server.h"
> +#include "src/shared/gatt-helpers.h"
> +#include "src/shared/att-types.h"
> +#include "src/shared/util.h"
> +
> +#ifndef MAX
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> +#endif
> +
> +struct bt_gatt_server {
> + struct gatt_db *db;
> + struct bt_att *att;
> + int ref_count;
> + uint16_t mtu;
> +
> + bt_gatt_server_debug_func_t debug_callback;
> + bt_gatt_server_destroy_func_t debug_destroy;
> + void *debug_data;
> +};
> +
> +static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
> +{
> + /* TODO */
> + return true;
> +}
> +
> +static void gatt_server_cleanup(struct bt_gatt_server *server)
> +{
> + bt_att_unref(server->att);
> + server->att = NULL;
> +}
> +
> +struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
> + struct bt_att *att, uint16_t mtu)
> +{
> + struct bt_gatt_server *server;
> +
> + if (!att)
> + return NULL;
> +
> + server = new0(struct bt_gatt_server, 1);
> + if (!server)
> + return NULL;
> +
> + server->db = db;
> + server->att = bt_att_ref(att);
> + server->mtu = MAX(mtu, BT_ATT_DEFAULT_LE_MTU);
> +
> + if (!gatt_server_register_att_handlers(server)) {
> + gatt_db_destroy(server->db);
> + bt_att_unref(att);
> + free(server);
> + return NULL;
> + }
> +
> + return bt_gatt_server_ref(server);
> +}
> +
> +struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server)
> +{
> + if (!server)
> + return NULL;
> +
> + __sync_fetch_and_add(&server->ref_count, 1);
> +
> + return server;
> +}
> +
> +void bt_gatt_server_unref(struct bt_gatt_server *server)
> +{
> + if (__sync_sub_and_fetch(&server->ref_count, 1))
> + return;
> +
> + if (server->debug_destroy)
> + server->debug_destroy(server->debug_data);
> +
> + gatt_server_cleanup(server);
> +
> + free(server);
> +}
> +
> +bool bt_gatt_server_set_debug(struct bt_gatt_server *server,
> + bt_gatt_server_debug_func_t callback,
> + void *user_data,
> + bt_gatt_server_destroy_func_t destroy)
> +{
> + if (!server)
> + return false;
> +
> + if (server->debug_destroy)
> + server->debug_destroy(server->debug_data);
> +
> + server->debug_callback = callback;
> + server->debug_destroy = destroy;
> + server->debug_data = user_data;
> +
> + return true;
> +}
> diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
> new file mode 100644
> index 0000000..e3c4def
> --- /dev/null
> +++ b/src/shared/gatt-server.h
> @@ -0,0 +1,40 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2014 Google Inc.
> + *
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#include <stdint.h>
> +
> +struct bt_gatt_server;
> +
> +struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
> + struct bt_att *att, uint16_t mtu);
> +
> +struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server);
> +void bt_gatt_server_unref(struct bt_gatt_server *server);
> +
> +typedef void (*bt_gatt_server_destroy_func_t)(void *user_data);
> +typedef void (*bt_gatt_server_debug_func_t)(const char *str, void *user_data);
> +
> +bool bt_gatt_server_set_debug(struct bt_gatt_server *server,
> + bt_gatt_server_debug_func_t callback,
> + void *user_data,
> + bt_gatt_server_destroy_func_t destroy);
> --
> 2.1.0.rc2.206.gedb03e5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
It looks good in context of future usage in Android
BR
Marcin
^ permalink raw reply
* Re: [PATCH v3 06/11] shared/hfp: Add send AT command API for HFP HF
From: Lukasz Rymanowski @ 2014-10-09 8:05 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org
In-Reply-To: <1412763331-30989-7-git-send-email-lukasz.rymanowski@tieto.com>
Hi,
On 8 October 2014 12:15, Lukasz Rymanowski <lukasz.rymanowski@tieto.com> 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 | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/shared/hfp.h | 5 ++
> 2 files changed, 187 insertions(+)
>
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index d61d76d..9681aae 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -70,6 +70,10 @@ struct hfp_hf {
> struct ringbuf *read_buf;
> struct ringbuf *write_buf;
>
> + bool writer_active;
> + struct queue *cmd_queue;
> + bool command_in_progress;
> +
> struct queue *event_handlers;
>
> hfp_debug_func_t debug_callback;
> @@ -101,6 +105,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,12 +880,74 @@ 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 *msg, enum hfp_result *result)
> +{
> + if (strcmp(msg, "OK") == 0) {
> + *result = HFP_RESULT_OK;
> + return true;
> + }
> +
> + if (strcmp(msg, "ERROR") == 0) {
> + *result = HFP_RESULT_ERROR;
> + 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->resp_data);
> + free(cmd->response);
> + free(cmd);
> +}
> +
> static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
> {
> struct event_handler *handler;
> @@ -904,6 +978,46 @@ 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 (hfp->command_in_progress) {
> + struct cmd_response *cmd;
> + enum hfp_result result;
> +
> + cmd = queue_peek_head(hfp->cmd_queue);
> + if (!cmd)
> + return;
> +
> + if (is_response(lookup_prefix, &result)) {
> + cmd->resp_cb(result, cmd->response, cmd->user_data);
> +
> + queue_remove(hfp->cmd_queue, cmd);
> + destroy_cmd_response(cmd);
> +
> + if (!queue_isempty(hfp->cmd_queue)) {
> + hf_wakeup_writer(hfp);
> + return;
> + }
> +
> + hfp->command_in_progress = false;
> +
> + return;
> + }
> + /*
> + * Check if unsolicited result is the response for ongoing
> + * command. If not we try to find registered handler for it
> + * later.
> + */
> + if (strcmp(lookup_prefix, &cmd->prefix[2]) == 0 &&
> + !cmd->resp_data) {
Looks like I missed the case where we can get more then one response
before OK e.g. on AT+CLCC we can get +CLCC, ..., +CLCC, OK.
Will fix that.
> + /* Store response and wait for OK */
> + cmd->resp_data = strdup(result_data.data);
> +
> + cmd->response = new0(struct hfp_hf_result, 1);
> + cmd->response->offset = result_data.offset;
> + cmd->response->data = cmd->resp_data;
> + return;
> + }
> + }
> +
> handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
> lookup_prefix);
>
> @@ -1033,6 +1147,19 @@ 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;
> + hfp->command_in_progress = false;
> +
> if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
> read_watch_destroy)) {
> queue_destroy(hfp->event_handlers,
> @@ -1084,6 +1211,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;
> @@ -1143,6 +1273,58 @@ 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);
> + 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;
> + }
> +
> + if (hfp->command_in_progress)
> + return true;
> +
> + hfp->command_in_progress = true;
> +
> + 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..773d827 100644
> --- a/src/shared/hfp.h
> +++ b/src/shared/hfp.h
> @@ -83,6 +83,9 @@ 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)(enum hfp_result result,
> + struct hfp_hf_result *resp,
> + void *user_data);
> struct hfp_gw;
> struct hfp_hf;
>
> @@ -146,3 +149,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, ...);
> --
> 1.8.4
>
\Łukasz
^ permalink raw reply
* Re: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function
From: Luiz Augusto von Dentz @ 2014-10-09 8:00 UTC (permalink / raw)
To: Gu, Chao Jie; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <3D02B219753AD44CBDDDE484323B17741130DD22@SHSMSX104.ccr.corp.intel.com>
Hi,
On Thu, Oct 9, 2014 at 7:02 AM, Gu, Chao Jie <chao.jie.gu@intel.com> wrote:
> Hi Luiz,
>
>> > @@ -77,6 +78,16 @@ struct bt_att {
>> > bt_att_debug_func_t debug_callback;
>> > bt_att_destroy_func_t debug_destroy;
>> > void *debug_data;
>> > +
>> > + struct bt_crypto *crypto;
>> > +
>> > + bool valid_local_csrk;
>> > + uint8_t local_csrk[16];
>> > + uint32_t local_sign_cnt;
>> > +
>> > + bool valid_remote_csrk;
>> > + uint8_t remote_csrk[16];
>> > + uint32_t remote_sign_cnt;
>>
>> Maybe it is better to have pointers to a structs so you can free the data once it
>> becomes invalid and it also removes the necessity of extra flags just to tell if the data
>> is still valid since you can check if the pointer is not NULL then it must be valid.
>
> This is good idea to remove the extra flags. However, we define the struct to do this which
> would result in some problem. Now we initialize the local_sign_cnt in the bt_att_new function
> and only add local_sign_cnt value when signed write command outgoing. Meanwile, we set local
> CSRK in the bt_gatt_client_set_local_csrk function when there is valid CSRK provided. This two
> Parameter set at different time now.
Well I guess this was your design, it doesn't have to be this way, in
fact I believe there is no point of initializing the counter if there
is no valid CSRK because one depend on the other, also perhaps you
have a bug in bt_gatt_client_set_local_csrk since I believe every time
you set CSRK you should reset the counter otherwise you may be
carrying the counters from the past CSRK which probably won't work.
Also don't to have to pass the counter when setting the key, the
counter probably need to be restored on every connection so
bt_gatt_client_set_local_csrk and bt_att_set_remote_csrk probably
needs to take the counter and instead of having a valid flag you can
probably reset by setting NULL as key or an invalid counter if one
exists.
> If we have pointers to a struct including local_csrk and local_signd_cnt, we initialize the local_sign_cnt
> meanwhile the struct should not be NULL because of incuding it. In fact, local_csrk is not valid at this time.
> If we initialize the local_signed_cnt when CSRK is valid , it would be risk to change local_signed_cnt by user
> calling bt_gatt_client_set_local_csrk.
>> > };
>> >
>> > enum att_op_type {
>> > @@ -176,6 +187,8 @@ struct att_send_op {
>> > bt_att_response_func_t callback;
>> > bt_att_destroy_func_t destroy;
>> > void *user_data;
>> > +
>> > + struct bt_att *att;
>> > };
>> >
>> > static void destroy_att_send_op(void *data) @@ -277,6 +290,10 @@
>> > static bool encode_pdu(struct att_send_op *op, const void *pdu,
>> > uint16_t length,
>> > uint16_t mtu) {
>> > uint16_t pdu_len = 1;
>> > + struct bt_att *att = op->att;
>> > +
>> > + if (op->opcode & ATT_OP_SIGNED_MASK)
>> > + pdu_len += BT_ATT_SIGNATURE_LEN;
>> >
>> > if (length && pdu)
>> > pdu_len += length;
>> > @@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op *op, const
>> void *pdu,
>> > if (pdu_len > 1)
>> > memcpy(op->pdu + 1, pdu, length);
>> >
>> > + if (op->opcode & ATT_OP_SIGNED_MASK) {
>> > + if (bt_crypto_sign_att(att->crypto,
>> > + att->local_csrk,
>> > + op->pdu,
>> > + 1 + length,
>> > + att->local_sign_cnt,
>> > + &((uint8_t *) op->pdu)[1 +
>> length]))
>> > + att->local_sign_cnt++;
>> > + else
>> > + return false;
>> > + }
>> > +
>>
>> You can probably bail out early if you check !(op->opcode & ATT_OP_SIGNED_MASK),
>> also perhaps it is a good idea to start defining structs for the PDUs to simplify the
>> access.
>>
> If we bail out this checking early , maybe we have to need two different encode_pdu
> function to implement it such as naming new encode_signed_pdu function to do this.
But you return true anyway after this code, no idea why you need a new
function for that, all I was suggesting is the following:
if (!(op->opcode & ATT_OP_SIGNED_MASK))
return true;
if (!bt_crypto_sign_att...)
return false;
att->local_sign_cnt++;
return true;
> However, I think encoding signed_write_command pdu most difference is adding
> signatgure in the end of pdu. As old stack, we can see every command has their
> own encode_pdu function. In fact, most procedure is common. So I think all in one
> encode_pdu function is new feature in new ATT Stack, so this implementation is
> at lowest price to compatible with new ATT Stack. That is why I didn’t bail out early
> this checking (op->opcode & ATT_OP_SIGNED_MASK). Following this implementation,
> there is no need to create another the encode function to implement signed_write,
> Just using one encode_pdu function to do this is OK.
>
> Do you think this make sense?
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH bluetooth-next] bluetooth: 6lowpan: fix skb_unshare behaviour
From: Johan Hedberg @ 2014-10-09 7:57 UTC (permalink / raw)
To: Alexander Aring; +Cc: linux-bluetooth, jukka.rissanen
In-Reply-To: <20141008084736.GA19532@omega>
Hi Alex,
On Wed, Oct 08, 2014, Alexander Aring wrote:
> On Wed, Oct 08, 2014 at 11:37:06AM +0300, Johan Hedberg wrote:
> > Hi Alex,
> >
> > On Wed, Oct 08, 2014, Alexander Aring wrote:
> > > This patch reverts commit:
> > >
> > > a7807d73 ("Bluetooth: 6lowpan: Avoid memory leak if memory allocation
> > > fails")
> > >
> > > which was wrong suggested by Alexander Aring. The function skb_unshare
> > > run also kfree_skb on failure.
> > >
> > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > ---
> > > compile tested only.
> > >
> > > net/bluetooth/6lowpan.c | 8 ++------
> > > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > This doesn't look like something that should only go to bluetooth-next
> > (which you've tagged it for). The bluetooth-next tree (since 3.17 has
> > been released) has moved to catering 3.19 patches whereas this patch
> > seems to be something that's fixing an issue heading to 3.18-rc1. So it
> > seems to me this patch should in fact be going to the bluetooth.git tree
> > instead of bluetooth-next.git. Am I right?
> >
>
> yes, but commit a7807d73 isn't inside of bluetooth.git right now. Should
> I rebase it to some other repository or cc stable here?
No, you shouldn't need to do anything. We'll take care of rebasing
bluetooth.git appropriately, applying the patch and eventually making a
pull request for it. My only concern was with the "PATCH bluetooth-next"
annotation in the subject.
Johan
^ permalink raw reply
* [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Martin Townsend @ 2014-10-09 7:46 UTC (permalink / raw)
To: linux-bluetooth, linux-wpan
Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend
In-Reply-To: <1412840794-17283-1-git-send-email-martin.townsend@xsilon.com>
Currently there are potentially 2 skb_copy_expand calls in IPHC
decompression. This patch replaces this with one call to
pskb_expand_head. It also checks to see if there is enough headroom
first to ensure it's only done if necessary.
As pskb_expand_head must only have one reference the calling code
now ensures this.
Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
net/bluetooth/6lowpan.c | 7 +++++++
2 files changed, 32 insertions(+), 26 deletions(-)
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..853b4b8 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
struct net_device *dev, skb_delivery_cb deliver_skb)
{
- struct sk_buff *new;
int stat;
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
+ skb_push(skb, sizeof(struct ipv6hdr));
+ skb_reset_network_header(skb);
+ skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
- new->protocol = htons(ETH_P_IPV6);
- new->pkt_type = PACKET_HOST;
- new->dev = dev;
+ skb->protocol = htons(ETH_P_IPV6);
+ skb->pkt_type = PACKET_HOST;
+ skb->dev = dev;
raw_dump_table(__func__, "raw skb data dump before receiving",
- new->data, new->len);
+ skb->data, skb->len);
- stat = deliver_skb(new, dev);
+ stat = deliver_skb(skb, dev);
- kfree_skb(new);
+ consume_skb(skb);
return stat;
}
@@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* UDP data uncompression */
if (iphc0 & LOWPAN_IPHC_NH_C) {
struct udphdr uh;
- struct sk_buff *new;
+ const int needed = sizeof(struct udphdr) + sizeof(hdr);
if (uncompress_udp_header(skb, &uh))
goto drop;
@@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* replace the compressed UDP head by the uncompressed UDP
* header
*/
- new = skb_copy_expand(skb, sizeof(struct udphdr),
- skb_tailroom(skb), GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb = new;
+ if (skb_headroom(skb) < needed) {
+ err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
skb_push(skb, sizeof(struct udphdr));
skb_reset_transport_header(skb);
@@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
(u8 *)&uh, sizeof(uh));
hdr.nexthdr = UIP_PROTO_UDP;
+ } else {
+ if (skb_headroom(skb) < sizeof(hdr)) {
+ err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
}
hdr.payload_len = htons(skb->len);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index c2e0d14..6643a7c 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
kfree_skb(local_skb);
kfree_skb(skb);
} else {
+ /* Decompression may use pskb_expand_head so no shared skb's */
+ skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!skb) {
+ dev->stats.rx_dropped++;
+ return NET_RX_DROP;
+ }
+
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
local_skb = skb_clone(skb, GFP_ATOMIC);
--
1.9.1
^ permalink raw reply related
* [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Martin Townsend @ 2014-10-09 7:46 UTC (permalink / raw)
To: linux-bluetooth, linux-wpan
Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend
This patch aims to reduce the amount of copying in the receive path when
decompressing by checking for headroom and calling pskb_expand_head if required.
Another benefit of this is that the skb pointer passed to the decompression
routine will be the same skb after decompression. This will be a step towards
freeing the skb within the receive function and not within all the error paths
of the decompression routine.
Bluetooth and 802.15.4 have been changed to ensure that the skb isn't shared
before calling the decompression routine as this is a requirement of
pskb_expand_head.
v1 -> v2
Use const int for new headroom size.
Remove blank line.
Use skb_unshare instead of skb_copy_expand.
Use consume_skb.
v2 -> v3
Remove cases where we are trying to free a NULL skb.
v3 -> v4
Now uses skb_share_check to ensure the skb isn't shared before decompressing
v4 -> v5
Remove skb_share_check from 802.15.4 lowpan_rcv as it's already done at the
top of the function.
Martin Townsend (1):
6lowpan: Use pskb_expand_head in IPHC decompression.
net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
net/bluetooth/6lowpan.c | 7 +++++++
2 files changed, 32 insertions(+), 26 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH v4 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Jukka Rissanen @ 2014-10-09 7:18 UTC (permalink / raw)
To: Martin Townsend
Cc: Alexander Aring, Martin Townsend, linux-bluetooth, linux-wpan,
marcel
In-Reply-To: <5435589A.7050605@xsilon.com>
Hi Martin,
code is ok by me.
Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
On ke, 2014-10-08 at 16:30 +0100, Martin Townsend wrote:
> Hi Alex,
>
> I'll wait for Jukka's ack for the bluetooth side and then send v5 with the removed skb_share_check.
>
> - Martin.
>
Cheers,
Jukka
^ permalink raw reply
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