Hi,
Comments below.
On Thu, 2013-05-23 at 11:37 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <[email protected]>
>
> The hash table is now the owner of the service entry instead of the
> service list.
> ---
> src/service.c | 311
> +++++++++++++++++++++-------------------------------------
> 1 file changed, 112 insertions(+), 199 deletions(-)
>
> diff --git a/src/service.c b/src/service.c
> index b8faa98..6a82b1e 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -41,7 +41,7 @@
...
> +static void list_move(GList *src, GList *dst)
> +{
> + GList *tmp;
> +
> + dst->prev->next = dst->next;
> + dst->next->prev = dst->prev;
> +
> + tmp = src->next;
> +
> + src->next = dst;
> + dst->prev = src;
> +
> + dst->next = tmp;
> + tmp->prev = dst;
> +}
> +
If either or both of the elements are at either end of the list, this
will crash.
> static void switch_default_service(struct connman_service *default_service,
> struct connman_service *downgrade_service)
> {
> - GSequenceIter *src, *dst;
> + GList *src, *dst;
>
> apply_relevant_default_downgrade(default_service);
> - src = g_hash_table_lookup(service_hash, downgrade_service->identifier);
> - dst = g_hash_table_lookup(service_hash, default_service->identifier);
> - g_sequence_move(src, dst);
> + src = g_list_find(service_list, downgrade_service);
> + dst = g_list_find(service_list, default_service);
> + list_move(src, dst);
> downgrade_state(downgrade_service);
> }
>
> @@ -4119,9 +4109,7 @@ static void service_append_added_foreach(gpointer data,
> gpointer user_data)
>
> static void service_append_ordered(DBusMessageIter *iter, void *user_data)
> {
> - if (service_list != NULL)
> - g_sequence_foreach(service_list,
> - service_append_added_foreach, iter);
> + g_list_foreach(service_list, service_append_added_foreach, iter);
> }
>
> static void append_removed(gpointer key, gpointer value, gpointer user_data)
> @@ -4252,8 +4240,6 @@ static void service_free(gpointer user_data)
>
> reply_pending(service, ENOENT);
>
> - g_hash_table_remove(service_hash, service->identifier);
> -
> __connman_notifier_service_remove(service);
> service_schedule_removed(service);
>
> @@ -4448,28 +4434,21 @@ connman_service_ref_debug(struct connman_service
> *service,
> void connman_service_unref_debug(struct connman_service *service,
> const char *file, int line, const char *caller)
> {
> - GSequenceIter *iter;
> -
> DBG("%p ref %d by %s:%d:%s()", service, service->refcount - 1,
> file, line, caller);
>
> if (__sync_fetch_and_sub(&service->refcount, 1) != 1)
> return;
>
> - iter = g_hash_table_lookup(service_hash, service->identifier);
> - if (iter != NULL) {
> - reply_pending(service, ECONNABORTED);
> + service_list = g_list_remove(service_list, service);
>
> - __connman_service_disconnect(service);
> + reply_pending(service, ECONNABORTED);
> + __connman_service_disconnect(service);
>
> - g_sequence_remove(iter);
> - } else {
> - service_free(service);
> - }
> + g_hash_table_remove(service_hash, service->identifier);
> }
>
> -static gint service_compare(gconstpointer a, gconstpointer b,
> - gpointer user_data)
> +static gint service_compare(gconstpointer a, gconstpointer b)
> {
> struct connman_service *service_a = (void *) a;
> struct connman_service *service_b = (void *) b;
> @@ -4685,13 +4664,8 @@ int __connman_service_set_favorite_delayed(struct
> connman_service *service,
> connman_bool_t favorite,
> gboolean delay_ordering)
> {
> - GSequenceIter *iter;
> -
> if (service->hidden == TRUE)
> return -EOPNOTSUPP;
> - iter = g_hash_table_lookup(service_hash, service->identifier);
> - if (iter == NULL)
> - return -ENOENT;
>
> if (service->favorite == favorite)
> return -EALREADY;
> @@ -4705,8 +4679,9 @@ int __connman_service_set_favorite_delayed(struct
> connman_service *service,
>
> if (delay_ordering == FALSE) {
>
> - if (g_sequence_get_length(service_list) > 1) {
> - g_sequence_sort_changed(iter, service_compare, NULL);
> + if (g_list_length(service_list) > 1) {
This will unnecessary iterate over the whole list. Use
service_list->next != NULL instead.
> + service_list = g_list_sort(service_list,
> + service_compare);
> service_schedule_changed();
> }
>
> @@ -5004,23 +4979,18 @@ static void request_input_cb (struct connman_service
> *service,
> static void downgrade_connected_services(void)
> {
> struct connman_service *up_service;
> - GSequenceIter *iter;
> + GList *list;
>
> - iter = g_sequence_get_begin_iter(service_list);
> - while (g_sequence_iter_is_end(iter) == FALSE) {
> - up_service = g_sequence_get(iter);
> + for (list = service_list; list != NULL; list = list->next) {
> + up_service = list->data;
>
> - if (is_connected(up_service) == FALSE) {
> - iter = g_sequence_iter_next(iter);
> + if (is_connected(up_service) == FALSE)
> continue;
> - }
>
> if (up_service->state == CONNMAN_SERVICE_STATE_ONLINE)
> return;
>
> downgrade_state(up_service);
> -
> - iter = g_sequence_iter_next(iter);
> }
> }
>
> @@ -5056,31 +5026,20 @@ static int service_update_preferred_order(struct
> connman_service *default_servic
>
> static void single_connected_tech(struct connman_service *allowed)
> {
> - GSList *services = NULL;
> - GSequenceIter *iter;
> - GSList *list;
> -
> - iter = g_sequence_get_begin_iter(service_list);
> -
> - while (g_sequence_iter_is_end(iter) == FALSE) {
> - struct connman_service *service = g_sequence_get(iter);
> -
> - if (service != allowed && is_connected(service))
> - services = g_slist_prepend(services, service);
> -
> - iter = g_sequence_iter_next(iter);
> - }
> + GList *list;
>
> DBG("keeping %p %s", allowed, allowed->path);
>
> - for (list = services; list != NULL; list = list->next) {
> + for (list = g_list_last(service_list); list != NULL;
> + list = list->prev) {
> struct connman_service *service = list->data;
>
> + if (allowed == service)
> + continue;
> +
> DBG("disconnecting %p %s", service, service->path);
> __connman_service_disconnect(service);
> }
> -
> - g_slist_free(services);
> }
Calling __connman_service_disconnect() will reorder the list (former
sequence) if/when the disconnection happens synchronously. Thus the
slist is first created and then the services are disconnected one by one
from that list to avoid the chaos of list (sequence) reordering when
iterating through it.
> static int service_indicate_state(struct connman_service *service)
> @@ -5088,7 +5047,6 @@ static int service_indicate_state(struct
> connman_service *service)
> enum connman_service_state old_state, new_state;
> struct connman_service *def_service;
> int result;
> - GSequenceIter *iter;
>
> if (service == NULL)
> return -EINVAL;
> @@ -5249,9 +5207,8 @@ static int service_indicate_state(struct
> connman_service *service)
> } else
> set_error(service, CONNMAN_SERVICE_ERROR_UNKNOWN);
>
> - iter = g_hash_table_lookup(service_hash, service->identifier);
> - if (iter != NULL && g_sequence_get_length(service_list) > 1) {
> - g_sequence_sort_changed(iter, service_compare, NULL);
> + if (g_list_length(service_list) > 1) {
As above.
> + service_list = g_list_sort(service_list, service_compare);
> service_schedule_changed();
> }
>
> @@ -5862,22 +5819,12 @@ int __connman_service_disconnect(struct
> connman_service *service)
>
> int __connman_service_disconnect_all(void)
> {
> - GSequenceIter *iter;
> - GSList *services = NULL, *list;
> + GList *list;
>
> DBG("");
>
> - iter = g_sequence_get_begin_iter(service_list);
> -
> - while (g_sequence_iter_is_end(iter) == FALSE) {
> - struct connman_service *service = g_sequence_get(iter);
> -
> - services = g_slist_prepend(services, service);
> -
> - iter = g_sequence_iter_next(iter);
> - }
> -
> - for (list = services; list != NULL; list = list->next) {
> + for (list = g_list_last(service_list); list != NULL;
> + list = list->prev) {
> struct connman_service *service = list->data;
>
> service->ignore = TRUE;
As above with the single connected tech.
> @@ -5887,10 +5834,7 @@ int __connman_service_disconnect_all(void)
> __connman_service_disconnect(service);
> }
>
> - g_slist_free(list);
> -
> return 0;
> -
> }
>
> /**
> @@ -5901,13 +5845,7 @@ int __connman_service_disconnect_all(void)
> */
> static struct connman_service *lookup_by_identifier(const char *identifier)
> {
> - GSequenceIter *iter;
> -
> - iter = g_hash_table_lookup(service_hash, identifier);
> - if (iter != NULL)
> - return g_sequence_get(iter);
> -
> - return NULL;
> + return g_hash_table_lookup(service_hash, identifier);
> }
>
> struct provision_user_data {
> @@ -5935,7 +5873,7 @@ int __connman_service_provision_changed(const char
> *ident)
> .ret = 0
> };
>
> - g_sequence_foreach(service_list, provision_changed, (void *)&data);
> + g_list_foreach(service_list, provision_changed, (void *)&data);
>
> /*
> * Because the provision_changed() might have set some services
> @@ -5944,8 +5882,9 @@ int __connman_service_provision_changed(const char
> *ident)
> if (services_dirty == TRUE) {
> services_dirty = FALSE;
>
> - if (g_sequence_get_length(service_list) > 1) {
> - g_sequence_sort(service_list, service_compare, NULL);
> + if (g_list_length(service_list) > 1) {
As above.
> + service_list = g_list_sort(service_list,
> + service_compare);
> service_schedule_changed();
> }
>
> @@ -5977,13 +5916,10 @@ void __connman_service_set_config(struct
> connman_service *service,
> static struct connman_service *service_get(const char *identifier)
> {
> struct connman_service *service;
> - GSequenceIter *iter;
>
> - iter = g_hash_table_lookup(service_hash, identifier);
> - if (iter != NULL) {
> - service = g_sequence_get(iter);
> - if (service != NULL)
> - connman_service_ref(service);
> + service = g_hash_table_lookup(service_hash, identifier);
> + if (service != NULL) {
> + connman_service_ref(service);
> return service;
> }
>
> @@ -5995,18 +5931,16 @@ static struct connman_service *service_get(const char
> *identifier)
>
> service->identifier = g_strdup(identifier);
>
> - iter = g_sequence_insert_sorted(service_list, service,
> - service_compare, NULL);
> + service_list = g_list_insert_sorted(service_list, service,
> + service_compare);
>
> - g_hash_table_insert(service_hash, service->identifier, iter);
> + g_hash_table_insert(service_hash, service->identifier, service);
>
> return service;
> }
>
> static int service_register(struct connman_service *service)
> {
> - GSequenceIter *iter;
> -
> DBG("service %p", service);
>
> if (service->path != NULL)
> @@ -6026,9 +5960,8 @@ static int service_register(struct connman_service
> *service)
> service_methods, service_signals,
> NULL, service, NULL);
>
> - iter = g_hash_table_lookup(service_hash, service->identifier);
> - if (iter != NULL && g_sequence_get_length(service_list) > 1) {
> - g_sequence_sort_changed(iter, service_compare, NULL);
> + if (g_list_length(service_list) > 1) {
As above.
> + service_list = g_list_sort(service_list, service_compare);
> service_schedule_changed();
> }
>
> @@ -6283,12 +6216,10 @@ struct connman_service
> *connman_service_lookup_from_network(struct connman_netwo
> struct connman_service *__connman_service_lookup_from_index(int index)
> {
> struct connman_service *service;
> - GSequenceIter *iter;
> -
> - iter = g_sequence_get_begin_iter(service_list);
> + GList *list;
>
> - while (g_sequence_iter_is_end(iter) == FALSE) {
> - service = g_sequence_get(iter);
> + for (list = service_list; list != NULL; list = list->next) {
> + service = list->data;
>
> if (__connman_ipconfig_get_index(service->ipconfig_ipv4)
> == index)
> @@ -6297,8 +6228,6 @@ struct connman_service
> *__connman_service_lookup_from_index(int index)
> if (__connman_ipconfig_get_index(service->ipconfig_ipv6)
> == index)
> return service;
> -
> - iter = g_sequence_iter_next(iter);
> }
>
> return NULL;
> @@ -6321,8 +6250,6 @@ const char *__connman_service_get_path(struct
> connman_service *service)
>
> unsigned int __connman_service_get_order(struct connman_service *service)
> {
> - GSequenceIter *iter;
> -
> if (service == NULL)
> return 0;
>
> @@ -6331,16 +6258,13 @@ unsigned int __connman_service_get_order(struct
> connman_service *service)
> goto done;
> }
>
> - iter = g_hash_table_lookup(service_hash, service->identifier);
> - if (iter != NULL) {
> - if (g_sequence_iter_get_position(iter) == 0)
> - service->order = 1;
> - else if (service->type == CONNMAN_SERVICE_TYPE_VPN &&
> - service->do_split_routing == FALSE)
> - service->order = 10;
> - else
> - service->order = 0;
> - }
> + if (service == service_list->data)
> + service->order = 1;
> + else if (service->type == CONNMAN_SERVICE_TYPE_VPN &&
> + service->do_split_routing == FALSE)
> + service->order = 10;
> + else
> + service->order = 0;
>
> DBG("service %p name %s order %d split %d", service, service->name,
> service->order, service->do_split_routing);
> @@ -6351,11 +6275,8 @@ done:
>
> void __connman_service_update_ordering(void)
> {
> - GSequenceIter *iter;
> -
> - iter = g_sequence_get_begin_iter(service_list);
> - if (iter != NULL && g_sequence_get_length(service_list) > 1)
> - g_sequence_sort_changed(iter, service_compare, NULL);
> + if (g_list_length(service_list) > 1)
As above.
> + service_list = g_list_sort(service_list, service_compare);
> }
>
> static enum connman_service_type convert_network_type(struct connman_network
> *network)
> @@ -6404,7 +6325,6 @@ static void update_from_network(struct connman_service
> *service,
> struct connman_network *network)
> {
> uint8_t strength = service->strength;
> - GSequenceIter *iter;
> const char *str;
>
> DBG("service %p network %p", service, network);
> @@ -6453,9 +6373,8 @@ static void update_from_network(struct connman_service
> *service,
> if (service->network == NULL)
> service->network = connman_network_ref(network);
>
> - iter = g_hash_table_lookup(service_hash, service->identifier);
> - if (iter != NULL && g_sequence_get_length(service_list) > 1) {
> - g_sequence_sort_changed(iter, service_compare, NULL);
> + if (g_list_length(service_list) > 1) {
As above.
> + service_list = g_list_sort(service_list, service_compare);
> service_schedule_changed();
> }
> }
> @@ -6566,7 +6485,6 @@ void __connman_service_update_from_network(struct
> connman_network *network)
> struct connman_service *service;
> uint8_t strength;
> connman_bool_t roaming;
> - GSequenceIter *iter;
> const char *name;
> connman_bool_t stats_enable;
>
> @@ -6621,9 +6539,9 @@ roaming:
>
> sorting:
> if (need_sort == TRUE) {
> - iter = g_hash_table_lookup(service_hash, service->identifier);
> - if (iter != NULL && g_sequence_get_length(service_list) > 1) {
> - g_sequence_sort_changed(iter, service_compare, NULL);
> + if (g_list_length(service_list) > 1) {
As above.
> + service_list = g_list_sort(service_list,
> + service_compare);
> service_schedule_changed();
> }
> }
> @@ -6820,9 +6738,7 @@ int __connman_service_init(void)
> connection = connman_dbus_get_connection();
>
> service_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
> - NULL, NULL);
> -
> - service_list = g_sequence_new(service_free);
> + NULL, service_free);
>
> services_notify = g_new0(struct _services_notify, 1);
> services_notify->remove = g_hash_table_new_full(g_str_hash,
> @@ -6836,8 +6752,6 @@ int __connman_service_init(void)
>
> void __connman_service_cleanup(void)
> {
> - GSequence *list;
> -
> DBG("");
>
> if (autoconnect_timeout != 0) {
> @@ -6845,9 +6759,8 @@ void __connman_service_cleanup(void)
> autoconnect_timeout = 0;
> }
>
> - list = service_list;
> + g_list_free(service_list);
> service_list = NULL;
> - g_sequence_free(list);
>
> g_hash_table_destroy(service_hash);
> service_hash = NULL;
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
