Hi Santiago,
On Fri, Dec 16, 2011 at 1:09 PM, Santiago Carot-Nemesio
<sancane@xxxxxxxxx> wrote:
> This patch prepares the structures in att-server to start and stop
> GATT server whenever an adapter is plugged or unplugged.
> ---
> src/attrib-server.c | 168 +++++++++++++++++++++++++++++++++++----------------
> 1 files changed, 116 insertions(+), 52 deletions(-)
>
> diff --git a/src/attrib-server.c b/src/attrib-server.c
> index b767b72..e8dade8 100644
> --- a/src/attrib-server.c
> +++ b/src/attrib-server.c
> @@ -71,12 +71,23 @@ struct group_elem {
> uint16_t len;
> };
>
> -static GIOChannel *l2cap_io = NULL;
> static GIOChannel *le_io = NULL;
le_io is not being used anymore.
> static GSList *clients = NULL;
> static uint32_t gatt_sdp_handle = 0;
> static uint32_t gap_sdp_handle = 0;
>
> +struct gatt_adapter {
> + struct btd_adapter *adapter;
> + GIOChannel *l2cap_io;
> + GIOChannel *le_io;
> + GSList *clients;
> + uint32_t gatt_sdp_handle;
> + uint32_t gap_sdp_handle;
> + GSList *database;
I recommend to introduce new variable when the it is really being
used. "database" and "clients" are used by free functions only, but
new elements are not being added in the list by this patch.
> +};
> +
> +static GSList *adapters = NULL;
> +
> /* GAP attribute handles */
> static uint16_t name_handle = 0x0000;
> static uint16_t appearance_handle = 0x0000;
> @@ -94,6 +105,60 @@ static bt_uuid_t ccc_uuid = {
> .value.u16 = GATT_CLIENT_CHARAC_CFG_UUID
> };
>
> +static void attrib_free(void *data)
> +{
> + struct attribute *a = data;
> +
> + g_free(a->data);
> + g_free(a);
> +}
> +
> +static void channel_free(struct gatt_channel *channel)
> +{
> + g_attrib_unref(channel->attrib);
> +
> + g_free(channel);
> +}
> +
> +static void free_gatt_adapter(struct gatt_adapter *gatt_adapter)
the coding style is "free" at the end of the function name
> +{
> + g_slist_free_full(gatt_adapter->database, attrib_free);
> +
> + if (gatt_adapter->l2cap_io != NULL) {
> + g_io_channel_unref(gatt_adapter->l2cap_io);
> + g_io_channel_shutdown(gatt_adapter->l2cap_io, FALSE, NULL);
> + }
> +
> + if (gatt_adapter->le_io != NULL) {
> + g_io_channel_unref(gatt_adapter->le_io);
> + g_io_channel_shutdown(gatt_adapter->le_io, FALSE, NULL);
> + }
> +
> + g_slist_free_full(gatt_adapter->clients, (GDestroyNotify) channel_free);
> +
> + if (gatt_adapter->gatt_sdp_handle > 0)
> + remove_record_from_server(gatt_adapter->gatt_sdp_handle);
> +
> + if (gatt_adapter->gap_sdp_handle > 0)
> + remove_record_from_server(gatt_adapter->gap_sdp_handle);
Removing gap_sdp_handle again
> +
> + if (gatt_adapter->adapter != NULL)
> + btd_adapter_unref(gatt_adapter->adapter);
> +
> + g_free(gatt_adapter);
> +}
> +
> +static gint adapter_cmp(gconstpointer a, gconstpointer b)
> +{
> + const struct gatt_adapter *gatt_adapter = a;
> + const struct btd_adapter *adapter = b;
> +
> + if (gatt_adapter->adapter == adapter)
> + return 0;
> +
> + return -1;
> +}
> +
> static sdp_record_t *server_record_new(uuid_t *uuid, uint16_t start, uint16_t end)
> {
> sdp_list_t *svclass_id, *apseq, *proto[2], *root, *aproto;
> @@ -689,21 +754,6 @@ static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
> return enc_mtu_resp(old_mtu, pdu, len);
> }
>
> -static void attrib_free(void *data)
> -{
> - struct attribute *a = data;
> -
> - g_free(a->data);
> - g_free(a);
> -}
> -
> -static void channel_free(struct gatt_channel *channel)
> -{
> - g_attrib_unref(channel->attrib);
> -
> - g_free(channel);
> -}
> -
> static void channel_disconnect(void *user_data)
> {
> struct gatt_channel *channel = user_data;
> @@ -1012,74 +1062,88 @@ failed:
> return FALSE;
> }
>
> -int attrib_server_init(void)
> +static int attrib_adapter_probe(struct btd_adapter *adapter)
> {
> + struct gatt_adapter *gatt_adapter;
> GError *gerr = NULL;
> + bdaddr_t addr;
> +
> + DBG("Start GATT server in %s", adapter_get_path(adapter));
> +
> + gatt_adapter = g_new0(struct gatt_adapter, 1);
> + gatt_adapter->adapter = btd_adapter_ref(adapter);
> +
> + adapter_get_address(gatt_adapter->adapter, &addr);
>
> /* BR/EDR socket */
> - l2cap_io = bt_io_listen(BT_IO_L2CAP, NULL, confirm_event,
> + gatt_adapter->l2cap_io = bt_io_listen(BT_IO_L2CAP, NULL, confirm_event,
> NULL, NULL, &gerr,
> - BT_IO_OPT_SOURCE_BDADDR, BDADDR_ANY,
> + BT_IO_OPT_SOURCE_BDADDR, &addr,
> BT_IO_OPT_PSM, ATT_PSM,
> BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
> BT_IO_OPT_INVALID);
> - if (l2cap_io == NULL) {
> +
> + if (gatt_adapter->l2cap_io == NULL) {
> error("%s", gerr->message);
> g_error_free(gerr);
> + free_gatt_adapter(gatt_adapter);
> return -1;
> }
>
> - if (!register_core_services())
> - goto failed;
> + if (!register_core_services()) {
> + g_io_channel_unref(gatt_adapter->l2cap_io);
> + free_gatt_adapter(gatt_adapter);
Two unref calls for l2cap_io here. free function also calls unref for l2cap_io
> + return -1;
> + }
>
> /* LE socket */
> - le_io = bt_io_listen(BT_IO_L2CAP, NULL, confirm_event,
> - &le_io, NULL, &gerr,
> - BT_IO_OPT_SOURCE_BDADDR, BDADDR_ANY,
> + gatt_adapter->le_io = bt_io_listen(BT_IO_L2CAP, NULL, confirm_event,
> + &gatt_adapter->le_io, NULL, &gerr,
> + BT_IO_OPT_SOURCE_BDADDR, &addr,
> BT_IO_OPT_CID, ATT_CID,
> BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
> BT_IO_OPT_INVALID);
> - if (le_io == NULL) {
> +
> + if (gatt_adapter->le_io == NULL) {
> error("%s", gerr->message);
> g_error_free(gerr);
> /* Doesn't have LE support, continue */
> }
>
> + adapters = g_slist_prepend(adapters, gatt_adapter);
> return 0;
> -
> -failed:
> - g_io_channel_unref(l2cap_io);
> - l2cap_io = NULL;
> -
> - if (le_io) {
> - g_io_channel_unref(le_io);
> - le_io = NULL;
> - }
> -
> - return -1;
> }
>
> -void attrib_server_exit(void)
> +static void attrib_adapter_remove(struct btd_adapter *adapter)
> {
> - g_slist_free_full(database, attrib_free);
> + struct gatt_adapter *gatt_adapter;
> + GSList *l;
>
> - if (l2cap_io) {
> - g_io_channel_unref(l2cap_io);
> - g_io_channel_shutdown(l2cap_io, FALSE, NULL);
> - }
> + l = g_slist_find_custom(adapters, adapter, adapter_cmp);
> + if (l == NULL)
> + return;
>
> - if (le_io) {
> - g_io_channel_unref(le_io);
> - g_io_channel_shutdown(le_io, FALSE, NULL);
> - }
> + DBG("Stop GATT server in %s", adapter_get_path(adapter));
>
> - g_slist_free_full(clients, (GDestroyNotify) channel_free);
> + gatt_adapter = l->data;
> + adapters = g_slist_remove(adapters, gatt_adapter);
> + free_gatt_adapter(gatt_adapter);
> +}
>
> - if (gatt_sdp_handle)
> - remove_record_from_server(gatt_sdp_handle);
> +static struct btd_adapter_driver attrib_adapter_driver = {
> + .name = "attrib-adapter-driver",
> + .probe = attrib_adapter_probe,
> + .remove = attrib_adapter_remove,
> +};
Is there race condition here? Remember that plugins can also register
an adapter driver.
If a GATT "plugin" wants to register attributes during the probing the
attribute server needs to be ready.
BR,
Claudio
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
