Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1422497463-32539-1-git-send-email-armansito@chromium.org> <1422497463-32539-2-git-send-email-armansito@chromium.org> Date: Thu, 29 Jan 2015 12:10:26 -0800 Message-ID: Subject: Re: [PATCH BlueZ 1/5] core: gatt: Implement GattCharacteristic1.StartNotify From: Arman Uguray To: Luiz Augusto von Dentz Cc: Arman Uguray , "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, > On Thu, Jan 29, 2015 at 5:37 AM, Luiz Augusto von Dentz wrote: > Hi Arman, > > On Thu, Jan 29, 2015 at 4:10 AM, Arman Uguray 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