Hi Jakub,
> On Mon, Mar 23, 2015 at 12:31 PM, Jakub Pawlowski <jpawlowski@xxxxxxxxxx> wrote:
> This patch adds logic for properly setting, updating and deleting
> discovery filter for each client.
Saying "properly set" makes me think that your code used to
"improperly" set the filters, so I'd just drop that entirely.
>
> Note that the filter is not used yet, making proper use from filter
> will be added in following patches.
> ---
> src/adapter.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 116 insertions(+), 8 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index f57b58c..672f550 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -158,6 +158,7 @@ struct watch_client {
> struct btd_adapter *adapter;
> char *owner;
> guint watch;
> + struct discovery_filter *discovery_filter;
> };
>
> struct service_auth {
> @@ -207,6 +208,9 @@ struct btd_adapter {
> uint8_t discovery_enable; /* discovery enabled/disabled */
> bool discovery_suspended; /* discovery has been suspended */
> GSList *discovery_list; /* list of discovery clients */
> + GSList *set_filter_list; /* list of clients that specified
> + * filter, but don't scan yet
> + */
> GSList *discovery_found; /* list of found devices */
> guint discovery_idle_timeout; /* timeout between discovery runs */
> guint passive_scan_timeout; /* timeout between passive scans */
> @@ -1355,6 +1359,17 @@ static uint8_t get_current_type(struct btd_adapter *adapter)
> return type;
> }
>
> +static void free_discovery_filter(struct discovery_filter *discovery_filter)
> +{
> + if (!discovery_filter)
> + return;
Since this is an internal function, I wouldn't have this above check
without a particular use for it. Basically, the sites that call this
code should make sure that discovery_filter isn't NULL when this gets
called.
> +
> + if (discovery_filter->uuids)
I think this check is unnecessary; afaik g_slist_free_full should
treat a NULL value as an empty GSList. I'd still test for this of
course.
> + g_slist_free_full(discovery_filter->uuids, g_free);
> +
> + g_free(discovery_filter);
> +}
> +
> static void trigger_start_discovery(struct btd_adapter *adapter, guint delay);
>
> static void start_discovery_complete(uint8_t status, uint16_t length,
> @@ -1654,9 +1669,17 @@ static void discovery_destroy(void *user_data)
>
> DBG("owner %s", client->owner);
>
> + adapter->set_filter_list = g_slist_remove(adapter->set_filter_list,
> + client);
> +
> adapter->discovery_list = g_slist_remove(adapter->discovery_list,
> client);
>
> + if (client->discovery_filter != NULL) {
if (!client->discovery_filter) {
> + free_discovery_filter(client->discovery_filter);
> + client->discovery_filter = NULL;
> + }
> +
> g_free(client->owner);
> g_free(client);
>
> @@ -1693,6 +1716,9 @@ static void discovery_disconnect(DBusConnection *conn, void *user_data)
>
> DBG("owner %s", client->owner);
>
> + adapter->set_filter_list = g_slist_remove(adapter->set_filter_list,
> + client);
> +
> adapter->discovery_list = g_slist_remove(adapter->discovery_list,
> client);
>
> @@ -1748,16 +1774,28 @@ static DBusMessage *start_discovery(DBusConnection *conn,
> if (list)
> return btd_error_busy(msg);
>
> - client = g_new0(struct watch_client, 1);
> + /* If client called SetDiscoveryFilter before, use pre-set filter. */
> + list = g_slist_find_custom(adapter->set_filter_list, sender,
> + compare_sender);
>
> - client->adapter = adapter;
> - client->owner = g_strdup(sender);
> - client->watch = g_dbus_add_disconnect_watch(dbus_conn, sender,
> - discovery_disconnect, client,
> - discovery_destroy);
> + if (list) {
> + client = list->data;
> + adapter->set_filter_list = g_slist_remove(
> + adapter->set_filter_list, list);
> + } else {
> + client = g_new0(struct watch_client, 1);
> +
> + client->adapter = adapter;
> + client->owner = g_strdup(sender);
> + client->discovery_filter = NULL;
> + client->watch = g_dbus_add_disconnect_watch(dbus_conn, sender,
> + discovery_disconnect,
> + client,
> + discovery_destroy);
> + }
>
> adapter->discovery_list = g_slist_prepend(adapter->discovery_list,
> - client);
> + client);
>
> /*
> * Just trigger the discovery here. In case an already running
> @@ -1922,8 +1960,9 @@ static DBusMessage *set_discovery_filter(DBusConnection *conn,
> DBusMessage *msg, void *user_data)
> {
> struct btd_adapter *adapter = user_data;
> + struct watch_client *client;
> struct discovery_filter *discovery_filter;
> -
> + GSList *list;
> const char *sender = dbus_message_get_sender(msg);
>
> DBG("sender %s", sender);
> @@ -1935,6 +1974,63 @@ static DBusMessage *set_discovery_filter(DBusConnection *conn,
> if (!parse_discovery_filter_dict(&discovery_filter, msg))
> return btd_error_invalid_args(msg);
>
> + list = g_slist_find_custom(adapter->discovery_list, sender,
> + compare_sender);
> +
> + if (!list) {
> + /* Client is not running any form of discovery. */
> + GSList *filter_list;
> +
> + /* Check wether client already pre-set his filter. */
s/wether/whether/
> + filter_list = g_slist_find_custom(adapter->set_filter_list,
> + sender, compare_sender);
> +
> + if (filter_list) {
> + client = filter_list->data;
> + free_discovery_filter(client->discovery_filter);
> +
> + if (discovery_filter != NULL) {
if (!discovery_filter) {
> + /* just update existing pre-set filter */
> + client->discovery_filter = discovery_filter;
> + DBG("successfully modified pre-set filter");
> + return dbus_message_new_method_return(msg);
> + }
> +
> + /* Removing pre-set filter */
> + adapter->set_filter_list = g_slist_remove(
> + adapter->set_filter_list,
> + client);
> + g_free(client->owner);
> + g_free(client);
> + DBG("successfully cleared pre-set filter");
> + return dbus_message_new_method_return(msg);
> + }
> + /* Client haven't pre-set his filter yet. */
> +
> + /* If there's no filter, no need to do anything. */
> + if (discovery_filter == NULL)
if (!discovery_filter) {
> + return dbus_message_new_method_return(msg);
> +
> + /* Client pre-setting his filter for first time */
> + client = g_new0(struct watch_client, 1);
> + client->adapter = adapter;
> + client->owner = g_strdup(sender);
> + client->discovery_filter = discovery_filter;
> + client->watch = g_dbus_add_disconnect_watch(dbus_conn, sender,
> + discovery_disconnect, client,
> + discovery_destroy);
This code is being repeated here and above for start_discovery_filter
and you're ending up with these huge nested if-else statements which
makes this code too hard to follow. I'd simplify this a little. Maybe
create a helper function that simply returns you a watch_client by
looking it up in both filter list and discovery list (perhaps we don't
even need to have both lists, e.g. you could add a filed to
watch_client that indicates whether or not discovery is in progress).
Something like:
client = get_discovery_client(adapter, owner);
This automatically allocates one and inserts it in the appropriate
list automatically. Then you won't need these repeated look ups.
> + adapter->set_filter_list = g_slist_prepend(
> + adapter->set_filter_list, client);
> +
> + DBG("successfully pre-set filter");
> + return dbus_message_new_method_return(msg);
> + }
> +
> + /* Client have already started discovery. */
> + client = list->data;
> + free_discovery_filter(client->discovery_filter);
> + client->discovery_filter = discovery_filter;
> +
> return btd_error_failed(msg, "Not implemented yet");
> }
>
> @@ -5160,6 +5256,18 @@ static void adapter_stop(struct btd_adapter *adapter)
>
> cancel_passive_scanning(adapter);
>
> + while (adapter->set_filter_list) {
> + struct watch_client *client;
> +
> + client = adapter->set_filter_list->data;
> +
> + /* g_dbus_remove_watch will remove the client from the
> + * adapter's list and free it using the discovery_destroy
> + * function.
> + */
> + g_dbus_remove_watch(dbus_conn, client->watch);
> + }
> +
> while (adapter->discovery_list) {
> struct watch_client *client;
>
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> 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
Thanks,
Arman
--
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
