Hi Luiz,
> On Thu, Jan 29, 2015 at 5:37 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
> Hi Arman,
>
> On Thu, Jan 29, 2015 at 4:10 AM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>> This patch implements the StartNotify method of
>> org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
>> session to the D-Bus sender which internally registers a unique
>> notify handler with the bt_gatt_client. Each received notification
>> or indication causes a PropertiesChanged signal to be emitted for the
>> "Value" property.
>>
>> The notify handler gets automatically unregistered when sender
>> disconnects from D-Bus.
>>
>> Change-Id: Ia805127613ee538eced4591ba0229789dc54fab8
>> ---
>> src/gatt-client.c | 256 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 247 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index cb8ddf6..2f187ad 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -88,6 +88,9 @@ struct characteristic {
>> unsigned int write_id;
>>
>> struct queue *descs;
>> +
>> + bool notifying;
>> + struct queue *notify_clients;
>> };
>>
>> struct descriptor {
>> @@ -231,7 +234,9 @@ static void async_dbus_op_free(void *data)
>> {
>> struct async_dbus_op *op = data;
>>
>> - dbus_message_unref(op->msg);
>> + if (op->msg)
>> + dbus_message_unref(op->msg);
>> +
>> free(op);
>> }
>>
>> @@ -689,12 +694,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
>> static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
>> DBusMessageIter *iter, void *data)
>> {
>> - dbus_bool_t notifying = FALSE;
>> -
>> - /*
>> - * TODO: Return the correct value here once StartNotify and StopNotify
>> - * methods are implemented.
>> - */
>> + struct characteristic *chrc = data;
>> + dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
>>
>> dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, ¬ifying);
>>
>> @@ -925,11 +926,240 @@ fail:
>> return btd_error_not_supported(msg);
>> }
>>
>> +struct notify_client {
>> + struct characteristic *chrc;
>> + int ref_count;
>> + char *owner;
>> + guint watch;
>> + unsigned int notify_id;
>> +};
>> +
>> +static void notify_client_free(struct notify_client *client)
>> +{
>> + DBG("owner %s", client->owner);
>> +
>> + g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
>> + free(client->owner);
>> + free(client);
>> +}
>> +
>> +static void notify_client_unref(void *data)
>> +{
>> + struct notify_client *client = data;
>> +
>> + DBG("owner %s", client->owner);
>> +
>> + if (__sync_sub_and_fetch(&client->ref_count, 1))
>> + return;
>> +
>> + notify_client_free(client);
>> +}
>> +
>> +static struct notify_client *notify_client_ref(struct notify_client *client)
>> +{
>> + DBG("owner %s", client->owner);
>> +
>> + __sync_fetch_and_add(&client->ref_count, 1);
>> +
>> + return client;
>> +}
>> +
>> +static bool match_notifying(const void *a, const void *b)
>> +{
>> + const struct notify_client *client = a;
>> +
>> + return !!client->notify_id;
>> +}
>> +
>> +static void update_notifying(struct characteristic *chrc)
>> +{
>> + if (!chrc->notifying)
>> + return;
>> +
>> + if (queue_find(chrc->notify_clients, match_notifying, NULL))
>> + return;
>> +
>> + chrc->notifying = false;
>> +
>> + g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
>> + GATT_CHARACTERISTIC_IFACE,
>> + "Notifying");
>> +}
>> +
>> +static void notify_client_disconnect(DBusConnection *conn, void *user_data)
>> +{
>> + struct notify_client *client = user_data;
>> + struct characteristic *chrc = client->chrc;
>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>> +
>> + DBG("owner %s", client->owner);
>> +
>> + queue_remove(chrc->notify_clients, client);
>> + bt_gatt_client_unregister_notify(gatt, client->notify_id);
>> +
>> + update_notifying(chrc);
>> +
>> + notify_client_unref(client);
>> +}
>> +
>> +static struct notify_client *notify_client_create(struct characteristic *chrc,
>> + const char *owner)
>> +{
>> + struct notify_client *client;
>> +
>> + client = new0(struct notify_client, 1);
>> + if (!client)
>> + return NULL;
>> +
>> + client->chrc = chrc;
>> + client->owner = strdup(owner);
>> + if (!client->owner) {
>> + free(client);
>> + return NULL;
>> + }
>> +
>> + client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
>> + owner, notify_client_disconnect,
>> + client, NULL);
>> + if (!client->watch) {
>> + free(client->owner);
>> + free(client);
>> + return NULL;
>> + }
>> +
>> + return notify_client_ref(client);
>> +}
>> +
>> +static bool match_notify_sender(const void *a, const void *b)
>> +{
>> + const struct notify_client *client = a;
>> + const char *sender = b;
>> +
>> + return strcmp(client->owner, sender) == 0;
>> +}
>> +
>> +static void notify_cb(uint16_t value_handle, const uint8_t *value,
>> + uint16_t length, void *user_data)
>> +{
>> + struct async_dbus_op *op = user_data;
>> + struct notify_client *client = op->data;
>> + struct characteristic *chrc = client->chrc;
>> +
>> + /*
>> + * Even if the value didn't change, we want to send a PropertiesChanged
>> + * signal so that we propagate the notification/indication to
>> + * applications.
>> + */
>> + gatt_db_attribute_reset(chrc->attr);
>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>> + write_characteristic_cb, chrc);
>> +}
>> +
>> +static void register_notify_cb(unsigned int id, uint16_t att_ecode,
>> + void *user_data)
>> +{
>> + struct async_dbus_op *op = user_data;
>> + struct notify_client *client = op->data;
>> + struct characteristic *chrc = client->chrc;
>> + DBusMessage *reply;
>> +
>> + /* Make sure that an interim disconnect did not remove the client */
>> + if (!queue_find(chrc->notify_clients, NULL, client)) {
>> + bt_gatt_client_unregister_notify(chrc->service->client->gatt,
>> + id);
>> + notify_client_unref(client);
>> +
>> + reply = btd_error_failed(op->msg,
>> + "Characteristic not available");
>> + goto done;
>> + }
>> +
>> + /*
>> + * Drop the reference count that we added when registering the callback.
>> + */
>> + notify_client_unref(client);
>> +
>> + if (!id) {
>> + queue_remove(chrc->notify_clients, client);
>> + notify_client_free(client);
>> +
>> + reply = create_gatt_dbus_error(op->msg, att_ecode);
>> +
>> + goto done;
>> + }
>> +
>> + client->notify_id = id;
>> +
>> + if (!chrc->notifying) {
>> + chrc->notifying = true;
>> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> + chrc->path, GATT_CHARACTERISTIC_IFACE,
>> + "Notifying");
>> + }
>> +
>> + reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
>> +
>> +done:
>> + if (reply)
>> + g_dbus_send_message(btd_get_dbus_connection(), reply);
>> + else
>> + error("Failed to construct D-Bus message reply");
>> +
>> + dbus_message_unref(op->msg);
>> + op->msg = NULL;
>
> The call to dbus_message_unref can probably be done in notify_client_free.
>
Actually it looks like async_dbus_op_free already calls
dbus_message_unref, so no need to do this clean-up here.
>> +}
>> +
>> static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>> DBusMessage *msg, void *user_data)
>> {
>> - /* TODO: Implement */
>> - return btd_error_failed(msg, "Not implemented");
>> + struct characteristic *chrc = user_data;
>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>> + const char *sender = dbus_message_get_sender(msg);
>> + struct async_dbus_op *op;
>> + struct notify_client *client;
>> +
>> + if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
>> + chrc->props & BT_GATT_CHRC_PROP_INDICATE))
>> + return btd_error_not_supported(msg);
>> +
>> + /* Each client can only have one active notify session. */
>> + client = queue_find(chrc->notify_clients, match_notify_sender, sender);
>> + if (client)
>> + return client->notify_id ?
>> + btd_error_failed(msg, "Already notifying") :
>> + btd_error_in_progress(msg);
>> +
>> + client = notify_client_create(chrc, sender);
>> + if (!client)
>> + return btd_error_failed(msg, "Failed allocate notify session");
>> +
>> + op = new0(struct async_dbus_op, 1);
>> + if (!op) {
>> + notify_client_unref(client);
>> + return btd_error_failed(msg, "Failed to initialize request");
>> + }
>> +
>> + /*
>> + * Add to the ref count so that a disconnect during register won't free
>> + * the client instance.
>> + */
>> + op->data = notify_client_ref(client);
>> + op->msg = dbus_message_ref(msg);
>> +
>> + queue_push_tail(chrc->notify_clients, client);
>> +
>> + if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
>> + register_notify_cb, notify_cb,
>> + op, async_dbus_op_free))
>> + return NULL;
>
> We better stop this pattern of doing non-cancelable operation, not
> only we cannot cancel any of the async_dbus_op it actually doesn't
> track the caller and we might have to store the op in the
> characteristic because in the event of the later being freed we will
> probably crash on the callback.
>
> Usually the pattern we had used for handling pending operation is to
> attach them to the resource being passed as user_data, not the other
> way around like you doing, take a look at
> android/hog.c:set_and_store_gatt_req it stores the id and then attach
> to the resource so in case anything happens to it the request can be
> cancelled.
>
async_dbus_op actually tracks the caller using op->data. The reason
why this has been safe for ReadValue/WriteValue is because I'm storing
the request ID directly in the caller (characteristic/descriptor).
When, for example, a characteristic gets freed, it cancels the request
by calling bt_gatt_client_cancel (which is really a wrapper for
bt_att_cancel). Since the async_dbus_op is passed as user_data at the
beginning, bt_att_cancel destroys the user_data and never executes the
callback for the request.
For bt_gatt_client_register_value we currently don't have a
cancellation mechanism, so you're right that this can cause a crash if
the characteristic is freed. So making the register operation
cancellable should address this, I'll go ahead and fix that.
>> + queue_remove(chrc->notify_clients, client);
>> + async_dbus_op_free(op);
>> +
>> + /* Directly free the client */
>> + notify_client_free(client);
>> +
>> + return btd_error_failed(msg, "Failed to register notify session");
>> }
>>
>> static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
>> @@ -1022,6 +1252,13 @@ static struct characteristic *characteristic_create(
>> return NULL;
>> }
>>
>> + chrc->notify_clients = queue_new();
>> + if (!chrc->notify_clients) {
>> + queue_destroy(chrc->descs, NULL);
>> + free(chrc);
>> + return NULL;
>> + }
>> +
>> chrc->service = service;
>>
>> gatt_db_attribute_get_char_data(attr, &chrc->handle,
>> @@ -1064,6 +1301,7 @@ static void unregister_characteristic(void *data)
>> if (chrc->write_id)
>> bt_gatt_client_cancel(gatt, chrc->write_id);
>>
>> + queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
>> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>>
>> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>
>
>
> --
> Luiz Augusto von Dentz
--
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
