From mboxrd@z Thu Jan 1 00:00:00 1970 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 16:43:07 -0800 Message-ID: Subject: Re: [PATCH BlueZ v1 03/14] core: Introduce btd_gatt_database From: Arman Uguray To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, > On Tue, Feb 17, 2015 at 4:03 AM, Luiz Augusto von Dentz wrote: > 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. > I'm taking a second look at storing the bt_gatt_server in btd_gatt_database, and I wonder if btd_device makes more sense the way the code currently is. Basically, we currently have the bt_att transport in btd_device and this likely won't change until we have cleaned up all the GAttrib dependencies. Currently there are two ways an ATT transport can be created: 1. Outgoing connection: e.g. via a D-Bus call to Device1.Connect, or outgoing auto-connect. 2. Incoming connection: btd_gatt_server's listening socket receives the connection event. In both cases we want to create both a bt_gatt_client and a bt_gatt_server. The way things are right now, it's much cleaner to create and store the bt_gatt_server alongside the bt_gatt_client within btd_device, since the client code already lives in btd_device. Otherwise, we'll have to create some kind of callback mechanism (or btd_gatt_database function) for attaching a server, which will lead to more spaghetti code. I already don't like the whole device_attach_att code path, but this is necessary for backwards compatibility reasons given all the profiles that still use GAttrib directly through btd_device. I suggest keeping bt_gatt_server inside btd_device for now. I'm already adding a TODO item for moving GATT related code to its own module as you suggested, so we can do a simple clean-up once the dependencies on the older code have been removed. > > -- > Luiz Augusto von Dentz Thanks, Arman