Hi Sonny,
On Wed, Sep 13, 2017 at 12:54 AM, Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx> wrote:
> Hi Luiz,
>
> Thank you for the patch. However I see that the patch doesn't address
> the case where start discovery happens during stop discovery (after
> stop_discovery but before stop_discovery_complete).
I would considered this a different problem, but yes either multiple
starts or start/stop in a sequence may cause the states to be out of
sync so it needs fixing.
> On Sun, Sep 10, 2017 at 12:24 PM, Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
>> Hi,
>>
>> On Thu, Sep 7, 2017 at 2:59 PM, Luiz Augusto von Dentz
>> <luiz.dentz@xxxxxxxxx> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>>>
>>> We should not reply until the start discovery completes otherwise
>>> clients may attempt to stop the discovery before it even has started.
>>>
>>> On top of this it will now block clients so they so not be able to
>>> queue more requests.
>>> ---
>>> src/adapter.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
>>> 1 file changed, 47 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/adapter.c b/src/adapter.c
>>> index a571b1870..4b9f5a1cd 100644
>>> --- a/src/adapter.c
>>> +++ b/src/adapter.c
>>> @@ -160,6 +160,7 @@ struct discovery_filter {
>>>
>>> struct watch_client {
>>> struct btd_adapter *adapter;
>>> + DBusMessage *msg;
>>> char *owner;
>>> guint watch;
>>> struct discovery_filter *discovery_filter;
>>> @@ -1421,13 +1422,17 @@ static void start_discovery_complete(uint8_t status, uint16_t length,
>>> const void *param, void *user_data)
>>> {
>>> struct btd_adapter *adapter = user_data;
>>> + struct watch_client *client = adapter->discovery_list->data;
>>> const struct mgmt_cp_start_discovery *rp = param;
>>> + DBusMessage *reply;
>>>
>>> DBG("status 0x%02x", status);
>>>
>>> if (length < sizeof(*rp)) {
>>> btd_error(adapter->dev_id,
>>> "Wrong size of start discovery return parameters");
>>> + if (client->msg)
>>> + goto fail;
>>> return;
>>> }
>>>
>>> @@ -1440,6 +1445,14 @@ static void start_discovery_complete(uint8_t status, uint16_t length,
>>> else
>>> adapter->filtered_discovery = false;
>>>
>>> + if (client->msg) {
>>> + reply = g_dbus_create_reply(client->msg,
>>> + DBUS_TYPE_INVALID);
>>> + g_dbus_send_message(dbus_conn, reply);
>>> + dbus_message_unref(client->msg);
>>> + client->msg = NULL;
>>> + }
>>> +
>>> if (adapter->discovering)
>>> return;
>>>
>>> @@ -1449,6 +1462,15 @@ static void start_discovery_complete(uint8_t status, uint16_t length,
>>> return;
>>> }
>>>
>>> +fail:
>>> + /* Reply with an error if the first discovery has failed */
>>> + if (client->msg) {
>>> + reply = btd_error_busy(client->msg);
>>> + g_dbus_send_message(dbus_conn, reply);
>>> + g_dbus_remove_watch(dbus_conn, client->watch);
>>> + return;
>>> + }
>>> +
>>> /*
>>> * In case the restart of the discovery failed, then just trigger
>>> * it for the next idle timeout again.
>>> @@ -1933,7 +1955,7 @@ static bool filters_equal(struct mgmt_cp_start_service_discovery *a,
>>> return true;
>>> }
>>>
>>> -static void update_discovery_filter(struct btd_adapter *adapter)
>>> +static int update_discovery_filter(struct btd_adapter *adapter)
>>> {
>>> struct mgmt_cp_start_service_discovery *sd_cp;
>>>
>>> @@ -1942,7 +1964,7 @@ static void update_discovery_filter(struct btd_adapter *adapter)
>>> if (discovery_filter_to_mgmt_cp(adapter, &sd_cp)) {
>>> btd_error(adapter->dev_id,
>>> "discovery_filter_to_mgmt_cp returned error");
>>> - return;
>>> + return -ENOMEM;
>>> }
>>>
>>> /*
>>> @@ -1953,13 +1975,15 @@ static void update_discovery_filter(struct btd_adapter *adapter)
>>> adapter->discovering != 0) {
>>> DBG("filters were equal, deciding to not restart the scan.");
>>> g_free(sd_cp);
>>> - return;
>>> + return 0;
>>> }
>>>
>>> g_free(adapter->current_discovery_filter);
>>> adapter->current_discovery_filter = sd_cp;
>>>
>>> trigger_start_discovery(adapter, 0);
>>> +
>>> + return -EINPROGRESS;
>>> }
>>>
>>> static void discovery_destroy(void *user_data)
>>> @@ -1980,6 +2004,9 @@ static void discovery_destroy(void *user_data)
>>> client->discovery_filter = NULL;
>>> }
>>>
>>> + if (client->msg)
>>> + dbus_message_unref(client->msg);
>>> +
>>> g_free(client->owner);
>>> g_free(client);
>>>
>>> @@ -2087,6 +2114,7 @@ static DBusMessage *start_discovery(DBusConnection *conn,
>>> const char *sender = dbus_message_get_sender(msg);
>>> struct watch_client *client;
>>> bool is_discovering;
>>> + int err;
>>>
>>> DBG("sender %s", sender);
>>>
>>> @@ -2107,12 +2135,14 @@ static DBusMessage *start_discovery(DBusConnection *conn,
>>> * and trigger scan.
>>> */
>>> if (client) {
>>> + if (client->msg)
>>> + return btd_error_busy(msg);
>>> +
>>> adapter->set_filter_list = g_slist_remove(
>>> adapter->set_filter_list, client);
>>> adapter->discovery_list = g_slist_prepend(
>>> adapter->discovery_list, client);
>>> - update_discovery_filter(adapter);
>>> - return dbus_message_new_method_return(msg);
>>> + goto done;
>>> }
>>>
>>> client = g_new0(struct watch_client, 1);
>>> @@ -2126,14 +2156,23 @@ static DBusMessage *start_discovery(DBusConnection *conn,
>>> adapter->discovery_list = g_slist_prepend(adapter->discovery_list,
>>> client);
>>>
>>> +done:
>>> /*
>>> * Just trigger the discovery here. In case an already running
>>> * discovery in idle phase exists, it will be restarted right
>>> * away.
>>> */
>>> - update_discovery_filter(adapter);
>>> + err = update_discovery_filter(adapter);
>>> + if (!err)
>>> + return dbus_message_new_method_return(msg);
>>>
>>> - return dbus_message_new_method_return(msg);
>>> + /* If the discovery has to be started wait it complete to reply */
>>> + if (err == -EINPROGRESS) {
>>> + client->msg = dbus_message_ref(msg);
>>> + return NULL;
>>> + }
>>> +
>>> + return btd_error_failed(msg, strerror(-err));
>>> }
>>>
>>> static bool parse_uuids(DBusMessageIter *value, struct discovery_filter *filter)
>>> @@ -2984,7 +3023,7 @@ static DBusMessage *get_discovery_filters(DBusConnection *conn,
>>> }
>>>
>>> static const GDBusMethodTable adapter_methods[] = {
>>> - { GDBUS_METHOD("StartDiscovery", NULL, NULL, start_discovery) },
>>> + { GDBUS_ASYNC_METHOD("StartDiscovery", NULL, NULL, start_discovery) },
>>> { GDBUS_METHOD("SetDiscoveryFilter",
>>> GDBUS_ARGS({ "properties", "a{sv}" }), NULL,
>>> set_discovery_filter) },
>>> --
>>> 2.13.5
>>
>> Could you guys please check if this does address the problem you are having?
>>
>>
>> --
>> Luiz Augusto von Dentz
--
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
