Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1423711064-7390-1-git-send-email-armansito@chromium.org> <1423711064-7390-4-git-send-email-armansito@chromium.org> Date: Tue, 17 Feb 2015 14:03:17 +0200 Message-ID: Subject: Re: [PATCH BlueZ v1 03/14] core: Introduce btd_gatt_database From: Luiz Augusto von Dentz To: Arman Uguray Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Arman, On Fri, Feb 13, 2015 at 6:21 PM, Arman Uguray wrote: > Hi Luiz, > >> On Fri, Feb 13, 2015 at 8:06 AM, Luiz Augusto von Dentz wrote: >> Hi Arman, >> >> On Thu, Feb 12, 2015 at 5:17 AM, Arman Uguray wrote: >>> This patch introduces src/gatt-database.* which handles incoming ATT >>> connections, manages per-adapter shared/gatt-db instances, and routes >>> connections to the corresponding device object. This is the layer that >>> will perform all the CCC management and Service Changed handling. >>> --- >>> Makefile.am | 1 + >>> src/adapter.c | 8 ++- >>> src/gatt-database.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> src/gatt-database.h | 26 +++++++ >>> src/main.c | 3 + >>> 5 files changed, 240 insertions(+), 2 deletions(-) >>> create mode 100644 src/gatt-database.c >>> create mode 100644 src/gatt-database.h >>> >>> diff --git a/Makefile.am b/Makefile.am >>> index 60811f1..4407355 100644 >>> --- a/Makefile.am >>> +++ b/Makefile.am >>> @@ -167,6 +167,7 @@ src_bluetoothd_SOURCES = $(builtin_sources) \ >>> src/sdpd-server.c src/sdpd-request.c \ >>> src/sdpd-service.c src/sdpd-database.c \ >>> src/attrib-server.h src/attrib-server.c \ >>> + src/gatt-database.h src/gatt-database.c \ >>> src/sdp-xml.h src/sdp-xml.c \ >>> src/sdp-client.h src/sdp-client.c \ >>> src/textfile.h src/textfile.c \ >>> diff --git a/src/adapter.c b/src/adapter.c >>> index 1839286..0253d08 100644 >>> --- a/src/adapter.c >>> +++ b/src/adapter.c >>> @@ -68,6 +68,7 @@ >>> #include "attrib/att.h" >>> #include "attrib/gatt.h" >>> #include "attrib-server.h" >>> +#include "gatt-database.h" >>> #include "eir.h" >>> >>> #define ADAPTER_INTERFACE "org.bluez.Adapter1" >>> @@ -302,6 +303,7 @@ static void dev_class_changed_callback(uint16_t index, uint16_t length, >>> appearance[1] = rp->val[1] & 0x1f; /* removes service class */ >>> appearance[2] = rp->val[2]; >>> >>> + /* TODO: Do this through btd_gatt_database instead */ >>> attrib_gap_set(adapter, GATT_CHARAC_APPEARANCE, appearance, 2); >>> } >>> >>> @@ -4014,6 +4016,7 @@ static void convert_sdp_entry(char *key, char *value, void *user_data) >>> if (record_has_uuid(rec, att_uuid)) >>> goto failed; >>> >>> + /* TODO: Do this through btd_gatt_database */ >>> if (!gatt_parse_record(rec, &uuid, &psm, &start, &end)) >>> goto failed; >>> >>> @@ -4548,7 +4551,7 @@ static void adapter_remove(struct btd_adapter *adapter) >>> adapter->devices = NULL; >>> >>> unload_drivers(adapter); >>> - btd_adapter_gatt_server_stop(adapter); >>> + btd_gatt_database_unregister_adapter(adapter); >>> >>> g_slist_free(adapter->pin_callbacks); >>> adapter->pin_callbacks = NULL; >>> @@ -6590,7 +6593,8 @@ static int adapter_register(struct btd_adapter *adapter) >>> agent_unref(agent); >>> } >>> >>> - btd_adapter_gatt_server_start(adapter); >>> + if (!btd_gatt_database_register_adapter(adapter)) >>> + error("Failed to register adapter with GATT server"); >>> >>> load_config(adapter); >>> fix_storage(adapter); >>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>> new file mode 100644 >>> index 0000000..57bdf1a >>> --- /dev/null >>> +++ b/src/gatt-database.c >>> @@ -0,0 +1,204 @@ >>> +/* >>> + * >>> + * BlueZ - Bluetooth protocol stack for Linux >>> + * >>> + * Copyright (C) 2015 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. >>> + * >>> + */ >>> + >>> +#ifdef HAVE_CONFIG_H >>> +#include >>> +#endif >>> + >>> +#include >>> +#include >>> + >>> +#include "lib/uuid.h" >>> +#include "btio/btio.h" >>> +#include "src/shared/util.h" >>> +#include "src/shared/queue.h" >>> +#include "src/shared/att.h" >>> +#include "src/shared/gatt-db.h" >>> +#include "log.h" >>> +#include "adapter.h" >>> +#include "device.h" >>> +#include "gatt-database.h" >>> + >>> +#ifndef ATT_CID >>> +#define ATT_CID 4 >>> +#endif >>> + >>> +static struct queue *servers = NULL; >>> + >>> +struct btd_gatt_database { >>> + struct btd_adapter *adapter; >>> + struct gatt_db *db; >>> + GIOChannel *le_io; >>> +}; >>> + >>> +static void gatt_server_free(void *data) >>> +{ >>> + struct btd_gatt_database *server = data; >>> + >>> + if (server->le_io) { >>> + g_io_channel_shutdown(server->le_io, FALSE, NULL); >>> + g_io_channel_unref(server->le_io); >>> + } >>> + >>> + gatt_db_unref(server->db); >>> + btd_adapter_unref(server->adapter); >>> + free(server); >>> +} >>> + >>> +bool btd_gatt_database_init(void) >>> +{ >>> + >>> + info("Initializing GATT server"); >>> + >>> + servers = queue_new(); >>> + if (!servers) { >>> + error("Failed to set up local GATT server"); >>> + return false; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +void btd_gatt_database_cleanup(void) >>> +{ >>> + info("Cleaning up GATT server"); >>> + >>> + queue_destroy(servers, gatt_server_free); >>> + servers = NULL; >>> +} >>> + >>> +static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>> +{ >>> + struct btd_adapter *adapter; >>> + struct btd_device *device; >>> + uint8_t dst_type; >>> + bdaddr_t src, dst; >>> + >>> + DBG("New incoming LE ATT connection"); >>> + >>> + if (gerr) { >>> + error("%s", gerr->message); >>> + return; >>> + } >>> + >>> + bt_io_get(io, &gerr, BT_IO_OPT_SOURCE_BDADDR, &src, >>> + BT_IO_OPT_DEST_BDADDR, &dst, >>> + BT_IO_OPT_DEST_TYPE, &dst_type, >>> + BT_IO_OPT_INVALID); >>> + if (gerr) { >>> + error("bt_io_get: %s", gerr->message); >>> + g_error_free(gerr); >>> + return; >>> + } >>> + >>> + adapter = adapter_find(&src); >>> + if (!adapter) >>> + return; >>> + >>> + device = btd_adapter_get_device(adapter, &dst, dst_type); >>> + if (!device) >>> + return; >>> + >>> + device_attach_att(device, io); >>> +} >>> + >>> +static bool match_adapter(const void *a, const void *b) >>> +{ >>> + const struct btd_gatt_database *server = a; >>> + const struct btd_adapter *adapter = b; >>> + >>> + return server->adapter == adapter; >>> +} >>> + >>> +bool btd_gatt_database_register_adapter(struct btd_adapter *adapter) >>> +{ >>> + struct btd_gatt_database *server; >>> + GError *gerr = NULL; >>> + const bdaddr_t *addr; >>> + >>> + if (!adapter) >>> + return false; >>> + >>> + if (!servers) { >>> + error("GATT server not initialized"); >>> + return false; >>> + } >>> + >>> + if (queue_find(servers, match_adapter, adapter)) { >>> + error("Adapter already registered with GATT server"); >>> + return false; >>> + } >>> + >>> + server = new0(struct btd_gatt_database, 1); >>> + if (!server) >>> + return false; >>> + >>> + server->adapter = btd_adapter_ref(adapter); >>> + server->db = gatt_db_new(); >>> + if (!server->db) >>> + goto fail; >>> + >>> + addr = btd_adapter_get_address(adapter); >>> + server->le_io = bt_io_listen(connect_cb, NULL, NULL, NULL, &gerr, >>> + BT_IO_OPT_SOURCE_BDADDR, addr, >>> + BT_IO_OPT_SOURCE_TYPE, BDADDR_LE_PUBLIC, >>> + BT_IO_OPT_CID, ATT_CID, >>> + BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW, >>> + BT_IO_OPT_INVALID); >>> + if (!server->le_io) { >>> + error("Failed to start listening: %s", gerr->message); >>> + g_error_free(gerr); >>> + goto fail; >>> + } >>> + >>> + queue_push_tail(servers, server); >>> + >>> + /* TODO: Set up GAP/GATT services */ >>> + >>> + return true; >>> + >>> +fail: >>> + gatt_server_free(server); >>> + >>> + return false; >>> +} >>> + >>> +void btd_gatt_database_unregister_adapter(struct btd_adapter *adapter) >>> +{ >>> + if (!adapter || !servers) >>> + return; >>> + >>> + queue_remove_all(servers, match_adapter, adapter, gatt_server_free); >>> +} >>> + >>> +struct gatt_db *btd_gatt_database_get_db(struct btd_adapter *adapter) >>> +{ >>> + struct btd_gatt_database *server; >>> + >>> + if (!servers) { >>> + error("GATT server not initialized"); >>> + return false; >>> + } >>> + >>> + server = queue_find(servers, match_adapter, adapter); >>> + if (!server) >>> + return false; >>> + >>> + return server->db; >>> +} >>> diff --git a/src/gatt-database.h b/src/gatt-database.h >>> new file mode 100644 >>> index 0000000..05e4ab9 >>> --- /dev/null >>> +++ b/src/gatt-database.h >>> @@ -0,0 +1,26 @@ >>> +/* >>> + * >>> + * BlueZ - Bluetooth protocol stack for Linux >>> + * >>> + * Copyright (C) 2015 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. >>> + * >>> + */ >>> + >>> +bool btd_gatt_database_init(void); >>> +void btd_gatt_database_cleanup(void); >>> + >>> +bool btd_gatt_database_register_adapter(struct btd_adapter *adapter); >>> +void btd_gatt_database_unregister_adapter(struct btd_adapter *adapter); >>> + >>> +struct gatt_db *btd_gatt_database_get_db(struct btd_adapter *adapter); >> >> Did not like this API, imo it is better to have >> btd_gatt_database_new() and declare struct btd_gatt_database, >> otherwise for every use we need a lookup which is not efficient. >> Perhaps if you don't have time look at this today I may find sometime >> during the weekend or Monday morning. >> > > I basically just followed src/attrib-server.c as an example, then > again that code didn't seem the cleanest to me either. I won't have > time to look at it today but I can revise it on Tuesday when I'm back, > though feel free to take a jab at it. > >>> diff --git a/src/main.c b/src/main.c >>> index 061060d..4ccc18f 100644 >>> --- a/src/main.c >>> +++ b/src/main.c >>> @@ -56,6 +56,7 @@ >>> #include "agent.h" >>> #include "profile.h" >>> #include "gatt.h" >>> +#include "gatt-database.h" >>> #include "systemd.h" >>> >>> #define BLUEZ_NAME "org.bluez" >>> @@ -579,6 +580,7 @@ int main(int argc, char *argv[]) >>> g_dbus_set_flags(gdbus_flags); >>> >>> gatt_init(); >>> + btd_gatt_database_init(); >>> >>> if (adapter_init() < 0) { >>> error("Adapter handling initialization failed"); >>> @@ -642,6 +644,7 @@ int main(int argc, char *argv[]) >>> >>> adapter_cleanup(); >>> >>> + btd_gatt_database_cleanup(); >>> gatt_cleanup(); >>> >>> rfkill_exit(); >>> -- >>> 2.2.0.rc0.207.ga3a616c >>> I applied this one, not that I did changed the API quite a bit so we don't need to do extra lookups, I also left comments how you should proceed, for instance it probably makes more sense to store the bt_gatt_server instance inside btd_gatt_database, at least it does not sound useful in device.c. Also it is probably necessary to have btd_adapter_get_database but the rest should be pretty straight forward. -- Luiz Augusto von Dentz