Re: [PATCH] android/gatt: Refactor client connection handling

[Tyszkowski Jakub <jakub.tyszkowski@xxxxxxxxx>]

Hi, Andrzej

On 04/23/2014 07:35 PM, Andrzej Kaczmarek wrote:
> Hi Jakub,
>
> On 23 April 2014 14:26, Jakub Tyszkowski <jakub.tyszkowski@xxxxxxxxx> wrote:
> > Multiple, connection specific device lists were replaced with one,
> > while devices store their connection state instead. Client list in each
> > device were replaced with single, global list, storing (connection_id,
> > client*, device*) tuples. This seams to be more natural and easier to
> > perform actions which are mostly triggered by specific clients on
> > specific device or connection.
> >
> > Connection id previously assigned to device now properly identifies
> > client<->device and not the physical adapter<->device connection.
> > ---
> >   android/gatt.c | 957 +++++++++++++++++++++++++++++----------------------------
> >   1 file changed, 495 insertions(+), 462 deletions(-)
> >
> > diff --git a/android/gatt.c b/android/gatt.c
> > index 8a6bc12..55f4f97 100644
> > --- a/android/gatt.c
> > +++ b/android/gatt.c
> > @@ -54,6 +54,20 @@
> >   #define GATT_SUCCESS   0x00000000
> >   #define GATT_FAILURE   0x00000101
> >
> > +typedef enum {
> > +       DEVICE_DISCONNECTED = 0,
> > +       DEVICE_CONNECTION_PENDING,
> > +       DEVICE_CONNECTABLE,
> > +       DEVICE_CONNECTED,
> > +} gatt_device_t;
>
> I'd call it device_state.

Ups, I've renamed it by accident. Will fix that.

> > +static char *device_str[] = {
> > +       "DISCONNECTED",
> > +       "CONNECTION PENDING",
> > +       "CONNECTABLE",
> > +       "CONNECTED",
> > +};
>
> device_state_str here and make it const.

Renamed by accident as above. I'll make it const

> >   struct gatt_client {
> >          int32_t id;
> >          uint8_t uuid[16];
> > @@ -98,8 +112,7 @@ struct service {
> >   struct notification_data {
> >          struct hal_gatt_srvc_id service;
> >          struct hal_gatt_gatt_id ch;
> > -       struct gatt_client *client;
> > -       struct gatt_device *dev;
> > +       struct connection_record *connection;
> >          guint notif_id;
> >          guint ind_id;
> >          int ref;
> > @@ -109,16 +122,21 @@ struct gatt_device {
> >          bdaddr_t bdaddr;
> >          uint8_t bdaddr_type;
> >
> > -       struct queue *clients;
> > -
> > -       bool connect_ready;
> > -       int32_t conn_id;
> > +       gatt_device_t state;
> >
> >          GAttrib *attrib;
> >          GIOChannel *att_io;
> >          struct queue *services;
> >
> >          guint watch_id;
> > +
> > +       uint32_t connection_ref_count;
>
> Why not just e.g. 'conn_ref'? You won't have loooooong lines later.

Yeap. Why not :)

> > +};
> > +
> > +struct connection_record {
> > +       struct gatt_device *device;
> > +       struct gatt_client *client;
> > +       int32_t id;
> >   };
>
> connection should be enough (you actually use it like this in function
> names anyway)

OK.

> >   static struct ipc *hal_ipc = NULL;
> > @@ -127,9 +145,8 @@ static bool scanning = false;
> >
> >   static struct queue *gatt_clients = NULL;
> >   static struct queue *gatt_servers = NULL;
> > -static struct queue *conn_list = NULL;         /* Connected devices */
> > -static struct queue *conn_wait_queue = NULL;   /* Devs waiting to connect */
> > -static struct queue *disc_dev_list = NULL;     /* Disconnected devices */
> > +static struct queue *gatt_devices = NULL;
>
> And this one I'd call just devices unless there's other list of
> non-gatt devices to be added.

Just wanted to keep the gatt_clients, gatt_server convention, but yes, 
this could be changed.

> > +static struct queue *client_connections = NULL;
> >
> >   static struct queue *listen_clients = NULL;
> >
> > @@ -276,26 +293,76 @@ static bool match_dev_by_bdaddr(const void *data, const void *user_data)
> >          return !bacmp(&dev->bdaddr, addr);
> >   }
> >
> > -static bool match_dev_connect_ready(const void *data, const void *user_data)
> > +static bool match_dev_by_state(const void *data, const void *user_data)
>
> You have other functions called e.g.
> 'match_connection_by_device_and_client' so would be good to have this
> consistent - either dev there or device here.

I'd go with "device", like "find_device..." functions.

> >   {
> >          const struct gatt_device *dev = data;
> >
> > -       return dev->connect_ready;
> > +       if (dev->state != PTR_TO_UINT(user_data))
> > +               return false;
> > +
> > +       return true;
> >   }
> >
> > -static bool match_dev_by_conn_id(const void *data, const void *user_data)
> > +static bool match_connection_by_id(const void *data, const void *user_data)
> >   {
> > -       const struct gatt_device *dev = data;
> > -       const int32_t conn_id = PTR_TO_INT(user_data);
> > +       const struct connection_record *record = data;
>
> Same here, later in code you use 'conn' or 'connection' as local
> variable for this so as long as it's possible, let's make it
> consistent. I'd make it 'connection' for function arguments and 'conn'
> for local variables to avoid conflicts.

Sure.

> > +       const int32_t id = PTR_TO_INT(user_data);
> >
> > -       return dev->conn_id == conn_id;
> > +       return record->id == id;
> >   }
> >
> > -static bool match_dev_without_client(const void *data, const void *user_data)
> > +static bool match_connection_by_device_and_client(const void *data,
> > +                                                       const void *user_data)
> >   {
> > -       const struct gatt_device *dev = data;
> > +       const struct connection_record *record = data;
> > +       const struct connection_record *match = user_data;
> >
> > -       return queue_isempty(dev->clients);
> > +       if (record->device == match->device)
> > +               return record->client == match->client;
> > +
> > +       return false;
>
> return (record->device == match->device && record->client == match->client);

OK.

> > +}
> > +
> > +static struct connection_record *find_connection_by_id(int32_t conn_id)
> > +{
> > +       return queue_find(client_connections, match_connection_by_id,
> > +                                                       INT_TO_PTR(conn_id));
> > +}
> > +
> > +static bool match_connection_by_device(const void *data, const void *user_data)
> > +{
> > +       const struct connection_record *record = data;
> > +       const struct gatt_device *dev = user_data;
> > +
> > +       return record->device == dev;
> > +}
> > +
> > +static bool match_connection_by_client(const void *data, const void *user_data)
> > +{
> > +       const struct connection_record *record = data;
> > +       const struct gatt_client *client = user_data;
> > +
> > +       return record->client == client;
> > +}
> > +
> > +static struct gatt_device *find_device(struct gatt_device *dev)
> > +{
> > +       return queue_find(gatt_devices, match_by_value, dev);
> > +}
> > +
> > +static struct gatt_device *find_device_by_addr(const bdaddr_t *addr)
> > +{
> > +       return queue_find(gatt_devices, match_dev_by_bdaddr, (void *)addr);
> > +}
> > +
> > +static struct gatt_device *find_device_by_state(uint32_t state)
> > +{
> > +       struct gatt_device *dev;
> > +
> > +       dev = queue_find(gatt_devices, match_dev_by_state,
> > +                                                       UINT_TO_PTR(state));
> > +
> > +       return dev;
>
> You have other functions above which return directly from queue_find,
> why this one can't?

Well... it should. I'll fix it.

> >   }
> >
> >   static bool match_srvc_by_element_id(const void *data, const void *user_data)
> > @@ -355,7 +422,7 @@ static bool match_notification(const void *a, const void *b)
> >          const struct notification_data *a1 = a;
> >          const struct notification_data *b1 = b;
> >
> > -       if (bacmp(&a1->dev->bdaddr, &b1->dev->bdaddr))
> > +       if (a1->connection != b1->connection)
> >                  return false;
> >
> >          if (memcmp(&a1->ch, &b1->ch, sizeof(a1->ch)))
> > @@ -381,11 +448,13 @@ static bool match_char_by_element_id(const void *data, const void *user_data)
> >   static void destroy_notification(void *data)
> >   {
> >          struct notification_data *notification = data;
> > +       struct gatt_client *client;
> >
> >          if (--notification->ref)
> >                  return;
> >
> > -       queue_remove_if(notification->client->notifications, match_notification,
> > +       client = notification->connection->client;
> > +       queue_remove_if(client->notifications, match_notification,
> >                                                                  notification);
> >          free(notification);
> >   }
> > @@ -393,14 +462,31 @@ static void destroy_notification(void *data)
> >   static void unregister_notification(void *data)
> >   {
> >          struct notification_data *notification = data;
> > +       struct gatt_device *dev = notification->connection->device;
> >
> > -       if (notification->notif_id)
> > -               g_attrib_unregister(notification->dev->attrib,
> > -                                                       notification->notif_id);
> > +       /* No device means it was already disconnected and client cleanup was
> > +        * triggered afterwards, but once client unregisters, device stays if
> > +        * used by others. Then just unregister single handle.
> > +        */
> > +       if (!queue_find(gatt_devices, match_by_value, dev))
> > +               return;
> > +
> > +       if (notification->notif_id && dev)
> > +               g_attrib_unregister(dev->attrib, notification->notif_id);
> > +
> > +       if (notification->ind_id && dev)
> > +               g_attrib_unregister(dev->attrib, notification->ind_id);
> > +}
> > +
> > +static void device_set_state(struct gatt_device *dev, uint32_t state)
> > +{
> > +       char bda[18];
> > +
> > +       ba2str(&dev->bdaddr, bda);
> > +       DBG("gatt: Device %s state changed %s -> %s", bda,
> > +                               device_str[dev->state], device_str[state]);
> >
> > -       if (notification->ind_id)
> > -               g_attrib_unregister(notification->dev->attrib,
> > -                                                       notification->ind_id);
> > +       dev->state = state;
> >   }
> >
> >   static void connection_cleanup(struct gatt_device *device)
> > @@ -422,21 +508,18 @@ static void connection_cleanup(struct gatt_device *device)
> >                  g_attrib_cancel_all(attrib);
> >                  g_attrib_unref(attrib);
> >          }
> > -}
> > -
> > -static void destroy_device(void *data)
> > -{
> > -       struct gatt_device *dev = data;
> >
> > -       if (!dev)
> > -               return;
> > -
> > -       if (dev->conn_id)
> > -               connection_cleanup(dev);
> > +       /* If device was in connection_pending or connectable state we
> > +        * search device list if we should stop the scan.
> > +        */
> > +       if (device->state == DEVICE_CONNECTION_PENDING ||
> > +                                       device->state == DEVICE_CONNECTABLE) {
> > +               if (!find_device_by_state(DEVICE_CONNECTION_PENDING) &&
> > +                       !find_device_by_state(DEVICE_CONNECTABLE) && !scanning)
>
> Check bool first - no need to iterate list twice in case scanning==false anyway.
> And perhaps there should be some helper to check if there's device in
> either state so we don't need to iterate list twice - this can be done
> in single pass.

I'll add helper function for that.

> > +                                       bt_le_discovery_stop(NULL);
> > +       }
> >
> > -       queue_destroy(dev->clients, NULL);
> > -       queue_destroy(dev->services, destroy_service);
> > -       free(dev);
> > +       device_set_state(device, DEVICE_DISCONNECTED);
> >   }
> >
> >   static void destroy_gatt_client(void *data)
> > @@ -531,123 +614,89 @@ failed:
> >                                                                          status);
> >   }
> >
> > -static void send_client_disconnect_notify(int32_t id, struct gatt_device *dev,
> > -                                                               int32_t status)
> > +static void send_client_connection_notify(struct connection_record *conn,
> > +                                                       int32_t gatt_event,
> > +                                                       int32_t status)
> >   {
> >          struct hal_ev_gatt_client_disconnect ev;
> >
> > -       ev.client_if = id;
> > -       ev.conn_id = dev->conn_id;
> > +       ev.client_if = conn->client->id;
> > +       ev.conn_id = conn->id;
> >          ev.status = status;
> > -       bdaddr2android(&dev->bdaddr, &ev.bda);
> >
> > -       ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> > -                       HAL_EV_GATT_CLIENT_DISCONNECT, sizeof(ev), &ev);
> > -}
> > +       bdaddr2android(&conn->device->bdaddr, &ev.bda);
> >
> > -static void send_client_connect_notify(int32_t id, struct gatt_device *dev,
> > -                                                               int32_t status)
> > -{
> > -       struct hal_ev_gatt_client_connect ev;
> > -
> > -       ev.client_if = id;
> > -       ev.status = status;
> > -       bdaddr2android(&dev->bdaddr, &ev.bda);
> > -       ev.conn_id = status == GATT_SUCCESS ? dev->conn_id : 0;
> > -
> > -       ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> > -                               HAL_EV_GATT_CLIENT_CONNECT, sizeof(ev), &ev);
> > +       ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT, gatt_event, sizeof(ev),
> > +                                                                       &ev);
> >   }
> >
> > -static void remove_client_from_device_list(void *data, void *user_data)
> > +static void send_client_disconnect_notify(struct connection_record *conn,
> > +                                                               int32_t status)
> >   {
> > -       struct gatt_device *dev = data;
> > -
> > -       queue_remove_if(dev->clients, match_by_value, user_data);
> > +       send_client_connection_notify(conn, HAL_EV_GATT_CLIENT_DISCONNECT,
> > +                                                                       status);
> >   }
> >
> > -static void put_device_on_disc_list(struct gatt_device *dev)
> > +static void send_client_connect_notify(struct connection_record *conn,
> > +                                                               int32_t status)
> >   {
> > -       dev->conn_id = 0;
> > -       queue_remove_all(dev->clients, NULL, NULL, NULL);
> > -       queue_push_tail(disc_dev_list, dev);
> > +       send_client_connection_notify(conn, HAL_EV_GATT_CLIENT_CONNECT, status);
> >   }
> >
> > -static void cleanup_conn_list(int32_t id)
> > +static void disconnect_notify_by_dev(void *data, void *user_data)
> >   {
> > -       struct gatt_device *dev;
> > -
> > -       /* Find device without client */
> > -       dev = queue_remove_if(conn_list, match_dev_without_client, NULL);
> > -       while (dev) {
> > -               /* Device is connected, lets disconnect and notify client */
> > -               connection_cleanup(dev);
> > -               send_client_disconnect_notify(id, dev, GATT_SUCCESS);
> > +       struct connection_record *record = data;
> > +       struct gatt_device *dev = user_data;
> >
> > -               /* Put device on disconnected device list */
> > -               put_device_on_disc_list(dev);
> > -               dev = queue_remove_if(conn_list, match_dev_without_client,
> > -                                                                       NULL);
> > -       };
> > +       if (dev == record->device) {
>
> if (dev != record->device)
>          return;

Got it.

> > +               if (dev->state == DEVICE_CONNECTED)
> > +                       send_client_disconnect_notify(record, GATT_SUCCESS);
> > +               else if (dev->state == DEVICE_CONNECTION_PENDING ||
> > +                                       dev->state == DEVICE_CONNECTABLE)
> > +                       send_client_connect_notify(record, GATT_FAILURE);
> > +       }
> >   }
> >
> > -static void cleanup_conn_wait_queue(int32_t id)
> > +static void destroy_connection(void *data)
> >   {
> > -       struct gatt_device *dev;
> > +       struct connection_record *conn = data;
> >
> > -       /* Find device without client */
> > -       dev = queue_remove_if(conn_wait_queue, match_dev_without_client, NULL);
> > -       while (dev) {
> > -               send_client_connect_notify(id, dev, GATT_FAILURE);
> > +       if (!queue_find(gatt_devices, match_by_value, conn->device))
> > +               goto cleanup;
> >
> > -               /* Put device on disconnected device list */
> > -               put_device_on_disc_list(dev);
> > -               dev = queue_remove_if(conn_wait_queue,
> > -                                               match_dev_without_client, NULL);
> > -       };
> > +       if (conn->device->connection_ref_count > 0)
> > +               conn->device->connection_ref_count--;
>
> As long as reference counting works fine you do not need to check >0
> since it should not be possible to have connection for device without
> +1 reference.
> This actually hides potential issue in reference counting.

That's true. Will change that.

> > -       /* Stop scan we had for connecting devices */
> > -       if (queue_isempty(conn_wait_queue) && !scanning)
> > -               bt_le_discovery_stop(NULL);
> > +       if (conn->device->connection_ref_count == 0)
> > +               connection_cleanup(conn->device);
> > +
> > +cleanup:
> > +       free(conn);
> >   }
> >
> > -static void remove_client_from_devices(int32_t id)
> > +static void device_disconnect_clients(struct gatt_device *dev)
> >   {
> > -       DBG("");
> > +       /* Notify disconnection to all clients */
> > +       queue_foreach(client_connections, disconnect_notify_by_dev, dev);
> >
> > -       queue_foreach(conn_list, remove_client_from_device_list,
> > -                                                       INT_TO_PTR(id));
> > -       cleanup_conn_list(id);
> > -
> > -       queue_foreach(conn_wait_queue, remove_client_from_device_list,
> > -                                                       INT_TO_PTR(id));
> > -       cleanup_conn_wait_queue(id);
> > +       /* Remove all clients by given device's */
> > +       queue_remove_all(client_connections, match_connection_by_device, dev,
> > +                                                       destroy_connection);
> >   }
> >
> > -static void handle_client_unregister(const void *buf, uint16_t len)
> > +static void destroy_device(void *data)
> >   {
> > -       const struct hal_cmd_gatt_client_unregister *cmd = buf;
> > -       uint8_t status;
> > -       struct gatt_client *cl;
> > +       struct gatt_device *dev = data;
> >
> > -       DBG("");
> > +       if (!dev)
> > +               return;
> >
> > -       cl = queue_remove_if(gatt_clients, match_client_by_id,
> > -                                               INT_TO_PTR(cmd->client_if));
> > -       if (!cl) {
> > -               error("gatt: client_if=%d not found", cmd->client_if);
> > -               status = HAL_STATUS_FAILED;
> > -       } else {
> > -               /* Check if there is any connect request or connected device for this
> > -                * client. If so, remove this client from those lists.
> > -                */
> > -               remove_client_from_devices(cl->id);
> > -               destroy_gatt_client(cl);
> > -               status = HAL_STATUS_SUCCESS;
> > -       }
> > +       if (dev->state != DEVICE_DISCONNECTED)
> > +               device_disconnect_clients(dev);
> >
> > -       ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
> > -                                       HAL_OP_GATT_CLIENT_UNREGISTER, status);
> > +       queue_destroy(dev->services, destroy_service);
> > +
> > +       free(dev);
> >   }
> >
> >   static void send_client_primary_notify(void *data, void *user_data)
> > @@ -670,17 +719,18 @@ static void send_client_primary_notify(void *data, void *user_data)
> >                                          sizeof(ev), &ev);
> >   }
> >
> > -static void send_client_all_primary(int32_t status, struct queue *services,
> > -                                                       int32_t conn_id)
> > +static void send_client_all_primary(struct connection_record *conn,
> > +                                                               int32_t status)
> >   {
> >          struct hal_ev_gatt_client_search_complete ev;
> >
> >          if (!status)
> > -               queue_foreach(services, send_client_primary_notify,
> > -                                                       INT_TO_PTR(conn_id));
> > +               queue_foreach(conn->device->services,
> > +                                               send_client_primary_notify,
> > +                                               INT_TO_PTR(conn->id));
> >
> >          ev.status = status;
> > -       ev.conn_id = conn_id;
> > +       ev.conn_id = conn->id;
> >          ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> >                          HAL_EV_GATT_CLIENT_SEARCH_COMPLETE, sizeof(ev), &ev);
> >
> > @@ -735,7 +785,7 @@ static struct service *create_service(uint8_t id, bool primary, char *uuid,
> >
> >   static void primary_cb(uint8_t status, GSList *services, void *user_data)
> >   {
> > -       struct gatt_device *dev = user_data;
> > +       struct connection_record *conn = user_data;
> >          GSList *l;
> >          int32_t gatt_status;
> >          uint8_t instance_id;
> > @@ -755,7 +805,7 @@ static void primary_cb(uint8_t status, GSList *services, void *user_data)
> >                  goto done;
> >          }
> >
> > -       if (!queue_isempty(dev->services)) {
> > +       if (!queue_isempty(conn->device->services)) {
> >                  info("gatt: Services already cached");
> >                  gatt_status = GATT_SUCCESS;
> >                  goto done;
> > @@ -774,7 +824,7 @@ static void primary_cb(uint8_t status, GSList *services, void *user_data)
> >                  if (!p)
> >                          continue;
> >
> > -               if (!queue_push_tail(dev->services, p)) {
> > +               if (!queue_push_tail(conn->device->services, p)) {
> >                          error("gatt: Cannot push primary service to the list");
> >                          free(p);
> >                          continue;
> > @@ -787,36 +837,7 @@ static void primary_cb(uint8_t status, GSList *services, void *user_data)
> >          gatt_status = GATT_SUCCESS;
> >
> >   done:
> > -       send_client_all_primary(gatt_status, dev->services, dev->conn_id);
> > -}
> > -
> > -static void client_disconnect_notify(void *data, void *user_data)
> > -{
> > -       struct gatt_device *dev = user_data;
> > -       int32_t id = PTR_TO_INT(data);
> > -
> > -       send_client_disconnect_notify(id, dev, GATT_SUCCESS);
> > -}
> > -
> > -static bool is_device_wating_for_connect(const bdaddr_t *addr,
> > -                                                       uint8_t addr_type)
> > -{
> > -       struct gatt_device *dev;
> > -
> > -       DBG("");
> > -
> > -       dev = queue_find(conn_wait_queue, match_dev_by_bdaddr, (void *)addr);
> > -       if (!dev)
> > -               return false;
> > -
> > -       dev->bdaddr_type = addr_type;
> > -
> > -       /* Mark that this device is ready for connect.
> > -        * Need it because will continue with connect after scan is stopped
> > -        */
> > -       dev->connect_ready = true;
> > -
> > -       return true;
> > +       send_client_all_primary(conn, gatt_status);
> >   }
> >
> >   static void le_device_found_handler(const bdaddr_t *addr, uint8_t addr_type,
> > @@ -826,6 +847,7 @@ static void le_device_found_handler(const bdaddr_t *addr, uint8_t addr_type,
> >   {
> >          uint8_t buf[IPC_MTU];
> >          struct hal_ev_gatt_client_scan_result *ev = (void *) buf;
> > +       struct gatt_device *dev;
> >          char bda[18];
> >
> >          if (!scanning || !discoverable)
> > @@ -845,9 +867,13 @@ static void le_device_found_handler(const bdaddr_t *addr, uint8_t addr_type,
> >                                                  sizeof(*ev) + ev->len, ev);
> >
> >   connect:
> > -       if (!is_device_wating_for_connect(addr, addr_type))
> > +       dev = find_device_by_addr(addr);
> > +       if (!dev || (dev->state != DEVICE_CONNECTION_PENDING))
> >                  return;
> >
> > +       device_set_state(dev, DEVICE_CONNECTABLE);
> > +       dev->bdaddr_type = addr_type;
> > +
> >          /* We are ok to perform connect now. Stop discovery
> >           * and once it is stopped continue with creating ACL
> >           */
> > @@ -861,45 +887,36 @@ static gboolean disconnected_cb(GIOChannel *io, GIOCondition cond,
> >          int sock, err = 0;
> >          socklen_t len;
> >
> > -       queue_remove(conn_list, dev);
> > -
> >          sock = g_io_channel_unix_get_fd(io);
> >          len = sizeof(err);
> >          if (!getsockopt(sock, SOL_SOCKET, SO_ERROR, &err, &len))
> >                  DBG("%s (%d)", strerror(err), err);
> >
> > -       connection_cleanup(dev);
> > -
> > -       queue_foreach(dev->clients, client_disconnect_notify, dev);
> > -
> > -       /* Reset conn_id and put on disconnected list. */
> > -       put_device_on_disc_list(dev);
> > +       device_disconnect_clients(dev);
> >
> >          return FALSE;
> >   }
> >
> > -struct connect_data {
> > -       struct gatt_device *dev;
> > -       int32_t status;
> > -};
> > -
> >   static void send_client_connect_notifications(void *data, void *user_data)
> >   {
> > -       int32_t id = PTR_TO_INT(data);
> > -       struct connect_data *c = user_data;
> > +       struct connection_record *rec = data;
> > +       uint32_t status = PTR_TO_UINT(user_data);
> >
> > -       send_client_connect_notify(id, c->dev, c->status);
> > +       send_client_connect_notify(rec, status);
> >   }
> >
> >   static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
> >   {
> >          struct gatt_device *dev = user_data;
> > -       struct connect_data data;
> > +       uint32_t status;
> >          GAttrib *attrib;
> > -       static uint32_t conn_id = 0;
> >
> > -       /* Take device from conn waiting queue */
> > -       if (!queue_remove(conn_wait_queue, dev)) {
> > +       /* Look for device if connection request is valid.
> > +        * TODO: Is this even possible when we have dev->att_io check
> > +        * in connect_le()?
> > +        */
> > +       dev = find_device(dev);
> > +       if (!dev || dev->state != DEVICE_CONNECTABLE) {
> >                  error("gatt: Device not on the connect wait queue!?");
> >                  g_io_channel_shutdown(io, TRUE, NULL);
> >                  return;
> > @@ -910,38 +927,31 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
> >
> >          if (gerr) {
> >                  error("gatt: connection failed %s", gerr->message);
> > -               data.status = GATT_FAILURE;
> > +               status = GATT_FAILURE;
> >                  goto reply;
> >          }
> >
> >          attrib = g_attrib_new(io);
> >          if (!attrib) {
> >                  error("gatt: unable to create new GAttrib instance");
> > -               data.status = GATT_FAILURE;
> > +               status = GATT_FAILURE;
> >                  goto reply;
> >          }
> >
> >          dev->attrib = attrib;
> >          dev->watch_id = g_io_add_watch(io, G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> >                                                          disconnected_cb, dev);
> > -       dev->conn_id = ++conn_id;
> >
> > -       /* Move gatt device from connect queue to conn_list */
> > -       if (!queue_push_tail(conn_list, dev)) {
> > -               error("gatt: Cannot push dev on conn_list");
> > -               connection_cleanup(dev);
> > -               data.status = GATT_FAILURE;
> > -               goto reply;
> > -       }
> > +       device_set_state(dev, DEVICE_CONNECTED);
> >
> > -       data.status = GATT_SUCCESS;
> > +       status = GATT_SUCCESS;
> >
> >   reply:
> > -       data.dev = dev;
> > -       queue_foreach(dev->clients, send_client_connect_notifications, &data);
> > +       queue_foreach(client_connections, send_client_connect_notifications,
> > +                                                       UINT_TO_PTR(status));
> >
> >          /* If connection did not succeed, destroy device */
> > -       if (data.status)
> > +       if (status)
> >                  destroy_device(dev);
> >
> >          /* Check if we should restart scan */
> > @@ -1050,16 +1060,9 @@ static int connect_next_dev(void)
> >
> >          DBG("");
> >
> > -       if (queue_isempty(conn_wait_queue))
> > -               return 0;
> > -
> > -       /* Discovery has been stopped because there is connection waiting */
> > -       dev = queue_find(conn_wait_queue, match_dev_connect_ready, NULL);
> > +       dev = find_device_by_state(DEVICE_CONNECTABLE);
> >          if (!dev)
> > -               /* Lets try again. */
> > -               return -1;
> > -
> > -       dev->connect_ready = false;
> > +               return -ENODEV;
> >
> >          return connect_le(dev);
>
> There should be separate CONNECTING state for device which is
> currently pending connection (actual one, not
> scanning-to-be-connected). Once there's connection request, we should
> not start scanning if there's device pending connection, just leave it
> in CONNECTION_PENDING state and then restart scanning in connect_cb in
> case there's any device in such state. This covers scenario where
> device was advertising and just stopped before we try to make actual
> connection - for consecutive connection request we'll try to start
> scanning but it won't work since there's connection request still
> pending.

Ok, It sounds reasonable to have such state.

> >   }
> > @@ -1073,22 +1076,6 @@ static void bt_le_discovery_stop_cb(void)
> >                  bt_le_discovery_start(le_device_found_handler);
> >   }
> >
> > -static struct gatt_device *find_device(bdaddr_t *addr)
> > -{
> > -       struct gatt_device *dev;
> > -
> > -       dev = queue_find(conn_list, match_dev_by_bdaddr, addr);
> > -       if (dev)
> > -               return dev;
> > -
> > -       return queue_find(conn_wait_queue, match_dev_by_bdaddr, addr);
> > -}
> > -
> > -static struct gatt_device *find_device_by_conn_id(int32_t conn_id)
> > -{
> > -       return queue_find(conn_list, match_dev_by_conn_id, INT_TO_PTR(conn_id));
> > -}
> > -
> >   static struct gatt_device *create_device(bdaddr_t *addr)
> >   {
> >          struct gatt_device *dev;
> > @@ -1099,96 +1086,202 @@ static struct gatt_device *create_device(bdaddr_t *addr)
> >
> >          bacpy(&dev->bdaddr, addr);
> >
> > -       dev->clients = queue_new();
> >          dev->services = queue_new();
> > -
> > -       if (!dev->clients || !dev->services) {
> > +       if (!dev->services) {
> >                  error("gatt: Failed to allocate memory for client");
> > +
>
> Avoid unrelated changes, i.e. whitespace.

OK.

> >                  destroy_device(dev);
> >                  return NULL;
> >          }
> >
> > +       if (!queue_push_head(gatt_devices, dev)) {
> > +               error("gatt: Cannot push device to queue");
> > +
> > +               return NULL;
>
> Need to destroy device here.

Thanks, missed that one.

> > +       }
> > +
> >          return dev;
> >   }
> >
> > -static void handle_client_connect(const void *buf, uint16_t len)
> > +static struct connection_record *create_connection(struct gatt_device *device,
> > +                                               struct gatt_client *client)
> >   {
> > -       const struct hal_cmd_gatt_client_connect *cmd = buf;
> > -       struct gatt_device *dev = NULL;
> > -       void *l;
> > -       bdaddr_t addr;
> > +       struct connection_record *new_conn;
> > +       struct connection_record *last;
> > +
> > +       /* Check if already connected */
> > +       new_conn = new0(struct connection_record, 1);
> > +       if (!new_conn)
> > +               return NULL;
> > +
> > +       new_conn->client = client;
> > +       new_conn->device = device;
> > +
> > +       /* Make connection id unique to connection record
> > +        * (client, device) pair.
> > +        */
> > +       last = queue_peek_head(client_connections);
> > +       if (last)
> > +               new_conn->id = last->id + 1;
> > +       else
> > +               new_conn->id = 1;
>
> You can just make static which will increment id for each connection
> and avoid this unnecessary logic here.

This was a "sort of" id reusage, but yes, we have plenty of those in 
int32_t range. I'll change that.

> > +
> > +       if (!queue_push_head(client_connections, new_conn)) {
> > +               error("gatt: Cannot push client on the client queue!?");
> > +
> > +               free(new_conn);
> > +               return NULL;
> > +       }
> > +
> > +       new_conn->device->connection_ref_count++;
> > +
> > +       return new_conn;
> > +}
> > +
> > +static void trigger_disconnection(struct connection_record *conn)
> > +{
> > +       /* Notify client */
> > +       if (queue_remove(client_connections, conn))
> > +               send_client_disconnect_notify(conn, GATT_SUCCESS);
> > +
> > +       destroy_connection(conn);
> > +}
> > +
> > +static void client_disconnect_devices(struct gatt_client *client)
> > +{
> > +       struct connection_record *conn;
> > +
> > +       /* find every connection for client record and trigger disconnect */
> > +       while ((conn = queue_remove_if(client_connections,
> > +                                       match_connection_by_client, client)))
>
> Make it without assignment in condition.

OK.

> > +               trigger_disconnection(conn);
> > +}
> > +
> > +static bool trigger_connection(struct connection_record *conn)
> > +{
> > +       switch (conn->device->state) {
> > +       case DEVICE_DISCONNECTED:
> > +               device_set_state(conn->device, DEVICE_CONNECTION_PENDING);
> > +               break;
> > +       case DEVICE_CONNECTED:
> > +               send_client_connect_notify(conn, GATT_SUCCESS);
> > +               break;
> > +       case DEVICE_CONNECTION_PENDING:
> > +       default:
> > +               break;
> > +       }
> > +
> > +       /* after state change trigger discovering */
> > +       if (!scanning && (conn->device->state == DEVICE_CONNECTION_PENDING))
> > +               if (!bt_le_discovery_start(le_device_found_handler)) {
> > +                       error("gatt: Could not start scan");
> > +
> > +                       return false;
> > +               }
> > +
> > +       return true;
> > +}
> > +
> > +static void handle_client_unregister(const void *buf, uint16_t len)
> > +{
> > +       const struct hal_cmd_gatt_client_unregister *cmd = buf;
> >          uint8_t status;
> > -       bool send_notify = false;
> > +       struct gatt_client *cl;
> >
> >          DBG("");
> >
> > -       /* Check if client is registered */
> > -       l = find_client_by_id(cmd->client_if);
> > -       if (!l) {
> > -               error("gatt: Client id %d not found", cmd->client_if);
> > +       cl = queue_remove_if(gatt_clients, match_client_by_id,
> > +                                               INT_TO_PTR(cmd->client_if));
> > +       if (!cl) {
> > +               error("gatt: client_if=%d not found", cmd->client_if);
> >                  status = HAL_STATUS_FAILED;
> > -               goto reply;
> > +       } else {
> > +               /* Check if there is any connect request or connected device
> > +                * for this client. If so, remove this client from those lists.
> > +                */
> > +               client_disconnect_devices(cl);
> > +               destroy_gatt_client(cl);
> > +               status = HAL_STATUS_SUCCESS;
> >          }
> >
> > -       android2bdaddr(&cmd->bdaddr, &addr);
> > +       ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
> > +                                       HAL_OP_GATT_CLIENT_UNREGISTER, status);
> > +}
> >
> > -       /* We do support many clients for one device connection so lets check
> > -        * If device is connected or in connecting state just update list of
> > -        * clients
> > -        */
> > -       dev = find_device(&addr);
> > -       if (dev) {
> > -               /* Remeber to send dummy notification event  if we area
> > -                * connected
> > -                */
> > -               if (dev->conn_id)
> > -                       send_notify = true;
> > +static struct connection_record *find_connection_by_dev_client_id(
>
> It's by addr and client_id.

Yeap, I'll fix that.

> BR,
> Andrzej

Thanks, I'll resend v2 with all those issues fixed.

BR,
Jakub

--
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