From df137fdf9a7077c20d8923f068568c22fb479e5a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 May 2017 12:09:45 +0200 Subject: [PATCH 01/14] proxy: fix refcount handing for DestroyProxyConfiguration operation Fixes: e895beb0da38fc87ce93fe7403a6b50e92f0dd82 --- src/nm-pacrunner-manager.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nm-pacrunner-manager.c b/src/nm-pacrunner-manager.c index cfc028c2cc..fd3d1f8a2d 100644 --- a/src/nm-pacrunner-manager.c +++ b/src/nm-pacrunner-manager.c @@ -250,6 +250,7 @@ pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) _LOG2D (config, "sent"); if (config->removed) { + config_ref (config); g_dbus_proxy_call (priv->pacrunner, "DestroyProxyConfiguration", g_variant_new ("(o)", config->path), From a08540d967812457af192ebd34497412da5d143e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 May 2017 12:32:22 +0200 Subject: [PATCH 02/14] proxy: fix passing cancellable to async D-Bus operations We must not cancel pacrunner_cancellable when the D-Bus proxy is created. Instead, keep it around and use it later for the asynchronous D-Bus operations. This doesn't really matter at the moment, because the pacrunner manager is only destroyed when NetworkManager is about to terminated. That is the only time when we actually cancel the asynchronous request. Also, at that time we no longer iterate the mainloop, so the pending requests are never completed anyway. --- src/nm-pacrunner-manager.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/nm-pacrunner-manager.c b/src/nm-pacrunner-manager.c index fd3d1f8a2d..87e0a36437 100644 --- a/src/nm-pacrunner-manager.c +++ b/src/nm-pacrunner-manager.c @@ -330,7 +330,6 @@ pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data) priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); priv->pacrunner = proxy; - nm_clear_g_cancellable (&priv->pacrunner_cancellable); g_signal_connect (priv->pacrunner, "notify::g-name-owner", G_CALLBACK (name_owner_changed_cb), self); From d079846330b094a99a9402fb4ddccb85af2913fe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 May 2017 18:26:07 +0200 Subject: [PATCH 03/14] shared: add "nm-utils/c-list.h" header Include the circular, doubly-linked list implementation from c-util/c-list [1], commit 864051de6e7e1c93c782064fbe3a86b4c17ac466. [1] https://github.com/c-util/c-list --- Makefile.am | 1 + shared/nm-utils/c-list.h | 436 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 437 insertions(+) create mode 100644 shared/nm-utils/c-list.h diff --git a/Makefile.am b/Makefile.am index 335239bdbd..96443196d4 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4403,6 +4403,7 @@ EXTRA_DIST += \ shared/nm-dispatcher-api.h \ shared/nm-test-libnm-utils.h \ shared/nm-test-utils-impl.c \ + shared/nm-utils/c-list.h \ shared/nm-utils/gsystem-local-alloc.h \ shared/nm-utils/nm-glib.h \ shared/nm-utils/nm-macros-internal.h \ diff --git a/shared/nm-utils/c-list.h b/shared/nm-utils/c-list.h new file mode 100644 index 0000000000..07e3f3cecd --- /dev/null +++ b/shared/nm-utils/c-list.h @@ -0,0 +1,436 @@ +#pragma once + +/* + * Circular Double Linked List Implementation in Standard ISO-C11 + * + * This implements a generic circular double linked list. List entries must + * embed the CList object, which provides pointers to the next and previous + * element. Insertion and removal can be done in O(1) due to the double links. + * Furthermore, the list is circular, thus allows access to front/tail in O(1) + * as well, even if you only have a single head pointer (which is not how the + * list is usually operated, though). + * + * Note that you are free to use the list implementation without a head + * pointer. However, usual operation uses a single CList object as head, which + * is itself linked in the list and as such must be identified as list head. + * This allows very simply list operations and avoids a lot of special cases. + * Most importantly, you can unlink entries without requiring a head pointer. + */ + +#ifdef __cplusplus +extern "C" { +#endif + +#include + +typedef struct CList CList; + +/** + * struct CList - Entry of a circular double linked list + * @next: next entry + * @prev: previous entry + * + * Each entry in a list must embed a CList object. This object contains + * pointers to its next and previous elements, which can be freely accessed by + * the API user at any time. Note that the list is circular, and the list head + * is linked in the list as well. + * + * The list head must be initialized via C_LIST_INIT before use. There is no + * reason to initialize entry objects before linking them. However, if you need + * a boolean state that tells you whether the entry is linked or not, you should + * initialize the entry via C_LIST_INIT as well. + */ +struct CList { + CList *next; + CList *prev; +}; + +#define C_LIST_INIT(_var) { .next = &(_var), .prev = &(_var) } + +/** + * c_list_init() - initialize list entry + * @what: list entry to initialize + */ +static inline void c_list_init(CList *what) { + *what = (CList)C_LIST_INIT(*what); +} + +/** + * c_list_entry() - get parent container of list entry + * @_what: list entry, or NULL + * @_t: type of parent container + * @_m: member name of list entry in @_t + * + * If the list entry @_what is embedded into a surrounding structure, this will + * turn the list entry pointer @_what into a pointer to the parent container + * (using offsetof(3), or sometimes called container_of(3)). + * + * If @_what is NULL, this will also return NULL. + * + * Return: Pointer to parent container, or NULL. + */ +#define c_list_entry(_what, _t, _m) \ + ((_t *)(void *)(((unsigned long)(void *)(_what) ?: \ + offsetof(_t, _m)) - offsetof(_t, _m))) + +/** + * c_list_is_linked() - check whether a entry is linked + * @what: entry to check, or NULL + * + * Return: True if @what is linked in a list, false if not. + */ +static inline _Bool c_list_is_linked(const CList *what) { + return what && what->next != what; +} + +/** + * c_list_is_empty() - check whether a list is empty + * @list: list to check, or NULL + * + * Return: True if @list is empty, false if not. + */ +static inline _Bool c_list_is_empty(const CList *list) { + return !list || !c_list_is_linked(list); +} + +/** + * c_list_link_before() - link entry into list + * @where: linked list entry used as anchor + * @what: entry to link + * + * This links @what directly in front of @where. @where can either be a list + * head or any entry in the list. + * + * If @where points to the list head, this effectively links @what as new tail + * element. Hence, the macro c_list_link_tail() is an alias to this. + * + * @what is not inspected prior to being linked. Hence, it better not be linked + * into another list, or the other list will be corrupted. + */ +static inline void c_list_link_before(CList *where, CList *what) { + CList *prev = where->prev, *next = where; + + next->prev = what; + what->next = next; + what->prev = prev; + prev->next = what; +} +#define c_list_link_tail(_list, _what) c_list_link_before((_list), (_what)) + +/** + * c_list_link_after() - link entry into list + * @where: linked list entry used as anchor + * @what: entry to link + * + * This links @what directly after @where. @where can either be a list head or + * any entry in the list. + * + * If @where points to the list head, this effectively links @what as new front + * element. Hence, the macro c_list_link_front() is an alias to this. + * + * @what is not inspected prior to being linked. Hence, it better not be linked + * into another list, or the other list will be corrupted. + */ +static inline void c_list_link_after(CList *where, CList *what) { + CList *prev = where, *next = where->next; + + next->prev = what; + what->next = next; + what->prev = prev; + prev->next = what; +} +#define c_list_link_front(_list, _what) c_list_link_after((_list), (_what)) + +/** + * c_list_unlink() - unlink element from list + * @what: element to unlink + * + * This unlinks @what. If @what was initialized via C_LIST_INIT(), it has no + * effect. If @what was never linked, nor initialized, behavior is undefined. + * + * Note that this does not modify @what. It just modifies the previous and next + * elements in the list to no longer reference @what. If you want to make sure + * @what is re-initialized after removal, use c_list_unlink_init(). + */ +static inline void c_list_unlink(CList *what) { + CList *prev = what->prev, *next = what->next; + + next->prev = prev; + prev->next = next; +} + +/** + * c_list_unlink_init() - unlink element from list and re-initialize + * @what: element to unlink + * + * This is like c_list_unlink() but re-initializes @what after removal. + */ +static inline void c_list_unlink_init(CList *what) { + /* condition is not needed, but avoids STOREs in fast-path */ + if (c_list_is_linked(what)) { + c_list_unlink(what); + *what = (CList)C_LIST_INIT(*what); + } +} + +/** + * c_list_swap() - exchange the contents of two lists + * @list1: the list to operate on + * @list2: the list to operate on + * + * This replaces the contents of the list @list1 with the contents + * of @list2, and vice versa. + */ +static inline void c_list_swap(CList *list1, CList *list2) { + CList t; + + /* make neighbors of list1 point to list2, and vice versa */ + t = *list1; + t.next->prev = list2; + t.prev->next = list2; + t = *list2; + t.next->prev = list1; + t.prev->next = list1; + + /* swap list1 and list2 now that their neighbors were fixed up */ + t = *list1; + *list1 = *list2; + *list2 = t; +} + +/** + * c_list_splice() - splice one list into another + * @target: the list to splice into + * @source: the list to splice + * + * This removes all the entries from @source and splice them into @target. + * The order of the two lists is preserved and the source is appended + * to the end of target. + */ +static inline void c_list_splice(CList *target, CList *source) { + if (c_list_is_empty(source)) + return; + + /* attach the front of @source to the tail of @target */ + source->next->prev = target->prev; + target->prev->next = source->next; + + /* attach the tail of @source to the front of @target */ + source->prev->next = target; + target->prev = source->prev; +} + +/** + * c_list_loop_first() - return first list element, or head if empty + * @list: list to operate on + * + * This is an O(1) accessor to the first list element. If the list is empty, + * this returns a pointer to the list head. Hence, this never returns NULL. + * + * Return: Pointer to first list element, or pointer to head if empty. + */ +static inline CList *c_list_loop_first(CList *list) { + return list->next; +} + +/** + * c_list_loop_last() - return last list element, or head if empty + * @list: list to operate on + * + * This is an O(1) accessor to the last list element. If the list is empty, + * this returns a pointer to the list head. Hence, this never returns NULL. + * + * Return: Pointer to last list element, or pointer to head if empty. + */ +static inline CList *c_list_loop_last(CList *list) { + return list->prev; +} + +/** + * c_list_loop_next() - return next list element, or head if none + * @what: list entry to operate on + * + * This is an O(1) accessor to the next list element. If @what is the list tail + * this will return a pointer to the list head. Hence, this never returns NULL. + * + * Return: Pointer to next list element, or pointer to head if none. + */ +static inline CList *c_list_loop_next(CList *what) { + return what->next; +} + +/** + * c_list_loop_prev() - return previous list element, or head if none + * @what: list entry to operate on + * + * This is an O(1) accessor to the previous list element. If @what is the list + * front this will return a pointer to the list head. Hence, this never returns + * NULL. + * + * Return: Pointer to previous list element, or pointer to head if none. + */ +static inline CList *c_list_loop_prev(CList *what) { + return what->prev; +} + +/** + * c_list_for_each() - loop over all list entries + * @_iter: iterator to use + * @_list: list to loop over + * + * This is a macro to use as for-loop to iterate an entire list. It is meant as + * convenience macro. Feel free to code your own loop iterator. + */ +#define c_list_for_each(_iter, _list) \ + for (_iter = c_list_loop_first(_list); \ + _iter != (_list); \ + _iter = c_list_loop_next(_iter)) + + +/** + * c_list_for_each_safe() - loop over all list entries, safe for removal + * @_iter: iterator to use + * @_safe: used to store pointer to next element + * @_list: list to loop over + * + * This is a macro to use as for-loop to iterate an entire list, safe against + * removal of the current element. It is meant as convenience macro. Feel free + * to code your own loop iterator. + * + * Note that this fetches the next element prior to executing the loop body. + * This makes it safe against removal of the current entry, but it will go + * havoc if you remove other list entries. You better not modify anything but + * the current list entry. + */ +#define c_list_for_each_safe(_iter, _safe, _list) \ + for (_iter = c_list_loop_first(_list), _safe = c_list_loop_next(_iter); \ + _iter != (_list); \ + _iter = _safe, _safe = c_list_loop_next(_safe)) + +/** + * c_list_for_each_entry() - loop over all list entries + * @_iter: iterator to use + * @_list: list to loop over + * @_m: member name of CList object in list type + * + * This combines c_list_for_each() with c_list_entry(), making it easy to + * iterate over a list of a specific type. + */ +#define c_list_for_each_entry(_iter, _list, _m) \ + for (_iter = c_list_entry(c_list_loop_first(_list), __typeof__(*_iter), _m); \ + &_iter->_m != (_list); \ + _iter = c_list_entry(c_list_loop_next(&_iter->_m), __typeof__(*_iter), _m)) + +/** + * c_list_for_each_entry_safe() - loop over all list entries, safe for removal + * @_iter: iterator to use + * @_safe: used to store pointer to next element + * @_list: list to loop over + * @_m: member name of CList object in list type + * + * This combines c_list_for_each_safe() with c_list_entry(), making it easy to + * iterate over a list of a specific type. + */ +#define c_list_for_each_entry_safe(_iter, _safe, _list, _m) \ + for (_iter = c_list_entry(c_list_loop_first(_list), __typeof__(*_iter), _m), \ + _safe = c_list_entry(c_list_loop_next(&_iter->_m), __typeof__(*_iter), _m);\ + &_iter->_m != (_list); \ + _iter = _safe, \ + _safe = c_list_entry(c_list_loop_next(&_safe->_m), __typeof__(*_iter), _m)) + +/** + * c_list_first() - return pointer to first element, or NULL if empty + * @list: list to operate on, or NULL + * + * This returns a pointer to the first element, or NULL if empty. This never + * returns a pointer to the list head. + * + * Return: Pointer to first list element, or NULL if empty. + */ +static inline CList *c_list_first(CList *list) { + return c_list_is_empty(list) ? NULL : list->next; +} + +/** + * c_list_last() - return pointer to last element, or NULL if empty + * @list: list to operate on, or NULL + * + * This returns a pointer to the last element, or NULL if empty. This never + * returns a pointer to the list head. + * + * Return: Pointer to last list element, or NULL if empty. + */ +static inline CList *c_list_last(CList *list) { + return c_list_is_empty(list) ? NULL : list->prev; +} + +/** + * c_list_first_entry() - return pointer to first entry, or NULL if empty + * @_list: list to operate on, or NULL + * @_t: type of list entries + * @_m: name of CList member in @_t + * + * This is like c_list_first(), but also applies c_list_entry() on the result. + * + * Return: Pointer to first list entry, or NULL if empty. + */ +#define c_list_first_entry(_list, _t, _m) \ + c_list_entry(c_list_first(_list), _t, _m) + +/** + * c_list_last_entry() - return pointer to last entry, or NULL if empty + * @_list: list to operate on, or NULL + * @_t: type of list entries + * @_m: name of CList member in @_t + * + * This is like c_list_last(), but also applies c_list_entry() on the result. + * + * Return: Pointer to last list entry, or NULL if empty. + */ +#define c_list_last_entry(_list, _t, _m) \ + c_list_entry(c_list_last(_list), _t, _m) + +/** + * c_list_length() - return the number of linked entries, excluding the head + * @list: list to operate on + * + * Returns the number of entires in the list, excluding the list head + * @list. That is, for a list that is empty according to c_list_is_empty(), + * the returned length is 0. This requires to iterate the list and has + * thus O(n) runtime. + * + * Return: the number of items in the list + */ +static inline size_t c_list_length(const CList *list) { + CList *iter; + size_t n = 0; + + c_list_for_each(iter, (CList *)list) + n++; + return n; +} + +/** + * c_list_contains() - whether an item is linked in a certain list + * @list: list to operate on + * @what: the list entry to find + * + * Searches @list whether @what is a linked entry of the list + * in O(n). For the head @list, this also returns True. + * + * Return: True if @what is in @list + */ +static inline _Bool c_list_contains(const CList *list, const CList *what) { + const CList *iter = list; + + do { + if (iter == what) + return 1; + iter = iter->next; + } while (iter != list); + return 0; +} + +#ifdef __cplusplus +} +#endif From c12dd0d970122bd6c3a21c845bf0c4693cb4fda1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 6 May 2017 18:17:19 +0200 Subject: [PATCH 04/14] ifcfg-rh: refactor shvar.c to use CList instead of GList --- src/settings/plugins/ifcfg-rh/shvar.c | 140 ++++++++++++-------------- 1 file changed, 62 insertions(+), 78 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index ca6aa72e0f..f150b65251 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -39,10 +39,14 @@ #include "nm-core-internal.h" #include "nm-core-utils.h" #include "nm-utils/nm-enum-utils.h" +#include "nm-utils/c-list.h" /*****************************************************************************/ struct _shvarLine { + + CList lst; + /* There are three cases: * * 1) the line is not a valid variable assignment (that is, it doesn't @@ -69,7 +73,7 @@ typedef struct _shvarLine shvarLine; struct _shvarFile { char *fileName; int fd; - GList *lineList; + CList lst_head; gboolean modified; }; @@ -628,6 +632,7 @@ svFile_new (const char *name) s = g_slice_new0 (shvarFile); s->fd = -1; s->fileName = g_strdup (name); + c_list_init (&s->lst_head); return s; } @@ -688,6 +693,7 @@ line_new_parse (const char *value, gsize len) nm_assert (value); line = g_slice_new0 (shvarLine); + c_list_init (&line->lst); for (k = 0; k < len; k++) { if (g_ascii_isspace (value[k])) @@ -725,6 +731,7 @@ line_new_build (const char *key, const char *value) value = svEscape (value, &value_escaped); line = g_slice_new (shvarLine); + c_list_init (&line->lst); line->line = value_escaped ?: g_strdup (value); line->key_with_prefix = g_strdup (key); line->key = line->key_with_prefix; @@ -769,6 +776,7 @@ line_free (shvarLine *line) ASSERT_shvarLine (line); g_free (line->line); g_free (line->key_with_prefix); + c_list_unlink (&line->lst); g_slice_free (shvarLine, line); } @@ -788,7 +796,6 @@ svOpenFileInternal (const char *name, gboolean create, GError **error) const char *p, *q; GError *local = NULL; nm_auto_close int fd = -1; - GList *lineList = NULL; if (create) fd = open (name, O_RDWR | O_CLOEXEC); /* NOT O_CREAT */ @@ -824,15 +831,13 @@ svOpenFileInternal (const char *name, gboolean create, GError **error) return NULL; } - for (p = arena; (q = strchr (p, '\n')) != NULL; p = q + 1) - lineList = g_list_prepend (lineList, line_new_parse (p, q - p)); - if (p[0]) - lineList = g_list_prepend (lineList, line_new_parse (p, strlen (p))); - g_free (arena); - lineList = g_list_reverse (lineList); - s = svFile_new (name); - s->lineList = lineList; + + for (p = arena; (q = strchr (p, '\n')) != NULL; p = q + 1) + c_list_link_tail (&s->lst_head, &line_new_parse (p, q - p)->lst); + if (p[0]) + c_list_link_tail (&s->lst_head, &line_new_parse (p, strlen (p))->lst); + g_free (arena); /* closefd is set if we opened the file read-only, so go ahead and * close it, because we can't write to it anyway */ @@ -862,37 +867,17 @@ svCreateFile (const char *name) /*****************************************************************************/ -static const GList * -shlist_find (const GList *current, const char *key) -{ - nm_assert (_shell_is_name (key, -1)); - - if (current) { - do { - shvarLine *line = current->data; - - ASSERT_shvarLine (line); - if (line->key && nm_streq (line->key, key)) - return current; - current = current->next; - } while (current); - } - return NULL; -} - -/*****************************************************************************/ - GHashTable * svGetKeys (shvarFile *s) { GHashTable *keys = NULL; - const GList *current; + CList *current; const shvarLine *line; nm_assert (s); - for (current = s->lineList; current; current = current->next) { - line = current->data; + c_list_for_each (current, &s->lst_head) { + line = c_list_entry (current, shvarLine, lst); if (line->key && line->line) { /* we don't clone the keys. The keys are only valid * until @s gets modified. */ @@ -909,34 +894,31 @@ svGetKeys (shvarFile *s) static const char * _svGetValue (shvarFile *s, const char *key, char **to_free) { - const GList *current, *last; - const shvarLine *line; + CList *current; + const shvarLine *line, *l; + const char *v; nm_assert (s); nm_assert (_shell_is_name (key, -1)); nm_assert (to_free); - last = NULL; - current = s->lineList; - while ((current = shlist_find (current, key))) { - last = current; - current = current->next; + line = NULL; + c_list_for_each (current, &s->lst_head) { + l = c_list_entry (current, shvarLine, lst); + if (l->key && nm_streq (l->key, key)) + line = l; } - if (last) { - line = last->data; - if (line->line) { - const char *v; - v = svUnescape (line->line, to_free); - if (!v) { - /* a wrongly quoted value is treated like the empty string. - * See also svWriteFile(), which handles unparsable values - * that way. */ - nm_assert (!*to_free); - return ""; - } - return v; + if (line && line->line) { + v = svUnescape (line->line, to_free); + if (!v) { + /* a wrongly quoted value is treated like the empty string. + * See also svWriteFile(), which handles unparsable values + * that way. */ + nm_assert (!*to_free); + return ""; } + return v; } *to_free = NULL; return NULL; @@ -1118,40 +1100,39 @@ svGetValueEnum (shvarFile *s, const char *key, void svSetValue (shvarFile *s, const char *key, const char *value) { - GList *current, *last; + CList *current; + shvarLine *line, *l; g_return_if_fail (s != NULL); g_return_if_fail (key != NULL); nm_assert (_shell_is_name (key, -1)); - last = NULL; - current = s->lineList; - while ((current = (GList *) shlist_find (current, key))) { - if (last) { - /* if we find multiple entries for the same key, we can - * delete all but the last. */ - line_free (last->data); - s->lineList = g_list_delete_link (s->lineList, last); - s->modified = TRUE; + line = NULL; + c_list_for_each (current, &s->lst_head) { + l = c_list_entry (current, shvarLine, lst); + if (l->key && nm_streq (l->key, key)) { + if (line) { + /* if we find multiple entries for the same key, we can + * delete all but the last. */ + line_free (line); + s->modified = TRUE; + } + line = l; } - last = current; - current = current->next; } if (!value) { - if (last) { - shvarLine *line = last->data; - + if (line) { if (nm_clear_g_free (&line->line)) s->modified = TRUE; } } else { - if (!last) { - s->lineList = g_list_append (s->lineList, line_new_build (key, value)); + if (!line) { + c_list_link_tail (&s->lst_head, &line_new_build (key, value)->lst); s->modified = TRUE; } else { - if (line_set (last->data, value)) + if (line_set (line, value)) s->modified = TRUE; } } @@ -1200,13 +1181,13 @@ svUnsetValue (shvarFile *s, const char *key) void svUnsetValuesWithPrefix (shvarFile *s, const char *prefix) { - GList *current; + CList *current; g_return_if_fail (s); g_return_if_fail (prefix); - for (current = s->lineList; current; current = current->next) { - shvarLine *line = current->data; + c_list_for_each (current, &s->lst_head) { + shvarLine *line = c_list_entry (current, shvarLine, lst); ASSERT_shvarLine (line); if ( line->key @@ -1231,7 +1212,7 @@ svWriteFile (shvarFile *s, int mode, GError **error) { FILE *f; int tmpfd; - const GList *current; + CList *current; if (s->modified) { if (s->fd == -1) @@ -1264,8 +1245,8 @@ svWriteFile (shvarFile *s, int mode, GError **error) } f = fdopen (tmpfd, "w"); fseek (f, 0, SEEK_SET); - for (current = s->lineList; current; current = current->next) { - const shvarLine *line = current->data; + c_list_for_each (current, &s->lst_head) { + const shvarLine *line = c_list_entry (current, shvarLine, lst); const char *str; char *s_tmp; gboolean valid_value; @@ -1304,11 +1285,14 @@ svWriteFile (shvarFile *s, int mode, GError **error) void svCloseFile (shvarFile *s) { + CList *current, *safe; + g_return_if_fail (s != NULL); if (s->fd != -1) close (s->fd); g_free (s->fileName); - g_list_free_full (s->lineList, (GDestroyNotify) line_free); + c_list_for_each_safe (current, safe, &s->lst_head) + line_free (c_list_entry (current, shvarLine, lst)); g_slice_free (shvarFile, s); } From c63a6772a4215eb7d0e2af0e523c7b92991f9a3f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 6 May 2017 20:27:29 +0200 Subject: [PATCH 05/14] libnm: use CList instead of GSList for pending_activations in "nm-manager.c" --- libnm/nm-manager.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c index d3df73375b..095b83d6e1 100644 --- a/libnm/nm-manager.c +++ b/libnm/nm-manager.c @@ -34,6 +34,7 @@ #include "nm-active-connection.h" #include "nm-vpn-connection.h" #include "nm-dbus-helpers.h" +#include "nm-utils/c-list.h" #include "introspection/org.freedesktop.NetworkManager.h" @@ -71,7 +72,7 @@ typedef struct { /* Activations waiting for their NMActiveConnection * to appear and then their callback to be called. */ - GSList *pending_activations; + CList pending_activations; gboolean networking_enabled; gboolean wireless_enabled; @@ -128,6 +129,8 @@ nm_manager_init (NMManager *manager) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (manager); + c_list_init (&priv->pending_activations); + priv->state = NM_STATE_UNKNOWN; priv->connectivity = NM_CONNECTIVITY_UNKNOWN; @@ -735,6 +738,7 @@ nm_manager_get_activating_connection (NMManager *manager) } typedef struct { + CList lst; NMManager *manager; GSimpleAsyncResult *simple; GCancellable *cancellable; @@ -748,15 +752,13 @@ activate_info_complete (ActivateInfo *info, NMActiveConnection *active, GError *error) { - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (info->manager); - if (active) g_simple_async_result_set_op_res_gpointer (info->simple, g_object_ref (active), g_object_unref); else g_simple_async_result_set_from_error (info->simple, error); g_simple_async_result_complete (info->simple); - priv->pending_activations = g_slist_remove (priv->pending_activations, info); + c_list_unlink (&info->lst); g_free (info->active_path); g_free (info->new_connection_path); @@ -790,7 +792,7 @@ static void recheck_pending_activations (NMManager *self) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - GSList *iter, *next; + CList *iter, *safe; NMActiveConnection *candidate; const GPtrArray *devices; NMDevice *device; @@ -804,12 +806,10 @@ recheck_pending_activations (NMManager *self) * device have both updated their properties to point to each other, and * call the pending connection's callback. */ - for (iter = priv->pending_activations; iter; iter = next) { - ActivateInfo *info = iter->data; + c_list_for_each_safe (iter, safe, &priv->pending_activations) { + ActivateInfo *info = c_list_entry (iter, ActivateInfo, lst); gs_unref_object GDBusObject *dbus_obj = NULL; - next = g_slist_next (iter); - if (!info->active_path) continue; @@ -901,14 +901,15 @@ nm_manager_activate_connection_async (NMManager *manager, if (connection) g_return_if_fail (NM_IS_CONNECTION (connection)); + priv = NM_MANAGER_GET_PRIVATE (manager); + info = g_slice_new0 (ActivateInfo); info->manager = manager; info->simple = g_simple_async_result_new (G_OBJECT (manager), callback, user_data, nm_manager_activate_connection_async); info->cancellable = cancellable ? g_object_ref (cancellable) : NULL; - priv = NM_MANAGER_GET_PRIVATE (manager); - priv->pending_activations = g_slist_prepend (priv->pending_activations, info); + c_list_link_tail (&priv->pending_activations, &info->lst); nmdbus_manager_call_activate_connection (priv->proxy, connection ? nm_connection_get_path (connection) : "/", @@ -977,14 +978,15 @@ nm_manager_add_and_activate_connection_async (NMManager *manager, if (partial) g_return_if_fail (NM_IS_CONNECTION (partial)); + priv = NM_MANAGER_GET_PRIVATE (manager); + info = g_slice_new0 (ActivateInfo); info->manager = manager; info->simple = g_simple_async_result_new (G_OBJECT (manager), callback, user_data, nm_manager_add_and_activate_connection_async); info->cancellable = cancellable ? g_object_ref (cancellable) : NULL; - priv = NM_MANAGER_GET_PRIVATE (manager); - priv->pending_activations = g_slist_prepend (priv->pending_activations, info); + c_list_link_tail (&priv->pending_activations, &info->lst); if (partial) dict = nm_connection_to_dbus (partial, NM_CONNECTION_SERIALIZE_ALL); @@ -1294,7 +1296,7 @@ dispose (GObject *object) /* Each activation should hold a ref on @manager, so if we're being disposed, * there shouldn't be any pending. */ - g_warn_if_fail (priv->pending_activations == NULL); + g_warn_if_fail (c_list_is_empty (&priv->pending_activations)); g_hash_table_destroy (priv->permissions); priv->permissions = NULL; From 35f37151df1dbdeeffa43aae80f28197823507b4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 6 May 2017 21:14:30 +0200 Subject: [PATCH 06/14] libnm: use CList instead of GSList for notify_items in "nm-object.c" --- libnm/nm-object.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/libnm/nm-object.c b/libnm/nm-object.c index 95346e06e9..5a521f0b8f 100644 --- a/libnm/nm-object.c +++ b/libnm/nm-object.c @@ -33,6 +33,7 @@ #include "nm-dbus-helpers.h" #include "nm-client.h" #include "nm-core-internal.h" +#include "nm-utils/c-list.h" static gboolean debug = FALSE; #define dbgmsg(f,...) if (G_UNLIKELY (debug)) { g_message (f, ## __VA_ARGS__ ); } @@ -77,7 +78,7 @@ typedef struct { GSList *waiters; /* if async init did not finish, users of this object need * to defer their notifications by adding themselves here. */ - GSList *notify_items; + CList notify_items; guint32 notify_id; GSList *reload_results; @@ -152,6 +153,7 @@ typedef enum { } NotifySignalPending; typedef struct { + CList lst; const char *property; const char *signal_prefix; NotifySignalPending pending; @@ -161,6 +163,7 @@ typedef struct { static void notify_item_free (NotifyItem *item) { + c_list_unlink (&item->lst); g_clear_object (&item->changed); g_slice_free (NotifyItem, item); } @@ -171,7 +174,8 @@ deferred_notify_cb (gpointer data) NMObject *object = NM_OBJECT (data); NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object); NMObjectClass *object_class = NM_OBJECT_GET_CLASS (object); - GSList *props, *iter; + CList props; + CList *iter, *safe; priv->notify_id = 0; @@ -184,16 +188,16 @@ deferred_notify_cb (gpointer data) * during the g_object_notify() call separately from the property * list we're iterating. */ - props = g_slist_reverse (priv->notify_items); - priv->notify_items = NULL; + c_list_link_after (&priv->notify_items, &props); + c_list_unlink_init (&priv->notify_items); g_object_ref (object); /* Emit added/removed signals first since some of our internal objects * use the added/removed signals for new object processing. */ - for (iter = props; iter; iter = g_slist_next (iter)) { - NotifyItem *item = iter->data; + c_list_for_each (iter, &props) { + NotifyItem *item = c_list_entry (iter, NotifyItem, lst); char buf[50]; gint ret = 0; @@ -219,8 +223,8 @@ deferred_notify_cb (gpointer data) } /* Emit property change notifications second */ - for (iter = props; iter; iter = g_slist_next (iter)) { - NotifyItem *item = iter->data; + c_list_for_each (iter, &props) { + NotifyItem *item = c_list_entry (iter, NotifyItem, lst); if (item->property) g_object_notify (G_OBJECT (object), item->property); @@ -228,7 +232,9 @@ deferred_notify_cb (gpointer data) g_object_unref (object); - g_slist_free_full (props, (GDestroyNotify) notify_item_free); + c_list_for_each_safe (iter, safe, &props) + notify_item_free (c_list_entry (iter, NotifyItem, lst)); + return G_SOURCE_REMOVE; } @@ -250,7 +256,7 @@ _nm_object_queue_notify_full (NMObject *object, { NMObjectPrivate *priv; NotifyItem *item; - GSList *iter; + CList *iter; g_return_if_fail (NM_IS_OBJECT (object)); g_return_if_fail (!signal_prefix != !property); @@ -261,8 +267,8 @@ _nm_object_queue_notify_full (NMObject *object, property = g_intern_string (property); signal_prefix = g_intern_string (signal_prefix); - for (iter = priv->notify_items; iter; iter = g_slist_next (iter)) { - item = iter->data; + c_list_for_each (iter, &priv->notify_items) { + item = c_list_entry (iter, NotifyItem, lst); if (property && (property == item->property)) return; @@ -314,7 +320,7 @@ _nm_object_queue_notify_full (NMObject *object, item->pending = added ? NOTIFY_SIGNAL_PENDING_ADDED : NOTIFY_SIGNAL_PENDING_REMOVED; item->changed = changed ? g_object_ref (changed) : NULL; } - priv->notify_items = g_slist_prepend (priv->notify_items, item); + c_list_link_tail (&priv->notify_items, &item->lst); } void @@ -1194,7 +1200,10 @@ nm_object_async_initable_iface_init (GAsyncInitableIface *iface) static void nm_object_init (NMObject *object) { - NM_OBJECT_GET_PRIVATE (object)->proxies = g_ptr_array_new (); + NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object); + + c_list_init (&priv->notify_items); + priv->proxies = g_ptr_array_new (); } static void @@ -1249,12 +1258,13 @@ static void dispose (GObject *object) { NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object); + CList *iter, *safe; guint i; nm_clear_g_source (&priv->notify_id); - g_slist_free_full (priv->notify_items, (GDestroyNotify) notify_item_free); - priv->notify_items = NULL; + c_list_for_each_safe (iter, safe, &priv->notify_items) + notify_item_free (c_list_entry (iter, NotifyItem, lst)); g_slist_free_full (priv->waiters, odata_free); From 65b440c9fa90f8c592db2c15a8dede8a9b1aa564 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 6 May 2017 21:14:30 +0200 Subject: [PATCH 07/14] libnm: use CList instead of GSList for pending in "nm-object.c" --- libnm/nm-object.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/libnm/nm-object.c b/libnm/nm-object.c index 5a521f0b8f..8cc4674e5e 100644 --- a/libnm/nm-object.c +++ b/libnm/nm-object.c @@ -85,7 +85,7 @@ typedef struct { guint reload_remaining; GError *reload_error; - GSList *pending; /* ordered list of pending property updates. */ + CList pending; /* ordered list of pending property updates. */ GPtrArray *proxies; } NMObjectPrivate; @@ -330,6 +330,7 @@ _nm_object_queue_notify (NMObject *object, const char *property) } typedef struct { + CList lst_pending; NMObject *self; PropertyInfo *pi; @@ -345,6 +346,7 @@ odata_free (gpointer data) { ObjectCreatedData *odata = data; + c_list_unlink (&odata->lst_pending); g_object_unref (odata->self); g_free (odata->objects); g_slice_free (ObjectCreatedData, odata); @@ -451,9 +453,10 @@ object_property_maybe_complete (NMObject *self) /* The odata may hold the last reference. */ _nm_unused gs_unref_object NMObject *self_keep_alive = g_object_ref (self); int i; + CList *iter, *safe; - while (priv->pending) { - ObjectCreatedData *odata = priv->pending->data; + c_list_for_each_safe (iter, safe, &priv->pending) { + ObjectCreatedData *odata = c_list_entry (iter, ObjectCreatedData, lst_pending); PropertyInfo *pi = odata->pi; gboolean different = TRUE; @@ -511,16 +514,16 @@ object_property_maybe_complete (NMObject *self) /* Emit added & removed */ for (i = 0; i < removed->len; i++) { queue_added_removed_signal (self, - pi->signal_prefix, - g_ptr_array_index (removed, i), - FALSE); + pi->signal_prefix, + g_ptr_array_index (removed, i), + FALSE); } for (i = 0; i < added->len; i++) { queue_added_removed_signal (self, - pi->signal_prefix, - g_ptr_array_index (added, i), - TRUE); + pi->signal_prefix, + g_ptr_array_index (added, i), + TRUE); } different = removed->len || added->len; @@ -553,7 +556,6 @@ object_property_maybe_complete (NMObject *self) if (--priv->reload_remaining == 0) reload_complete (self, TRUE); - priv->pending = g_slist_remove (priv->pending, odata); odata_free (odata); } } @@ -594,7 +596,8 @@ handle_object_property (NMObject *self, const char *property_name, GVariant *val odata->array = FALSE; odata->property_name = property_name; - priv->pending = g_slist_append (priv->pending, odata); + c_list_link_tail (&priv->pending, &odata->lst_pending); + priv->reload_remaining++; path = g_variant_get_string (value, NULL); @@ -650,7 +653,8 @@ handle_object_array_property (NMObject *self, const char *property_name, GVarian odata->array = TRUE; odata->property_name = property_name; - priv->pending = g_slist_append (priv->pending, odata); + c_list_link_tail (&priv->pending, &odata->lst_pending); + priv->reload_remaining++; if (npaths == 0) { @@ -950,7 +954,7 @@ _nm_object_register_properties (NMObject *object, proxy = _nm_object_get_proxy (object, interface); g_signal_connect (proxy, "g-properties-changed", - G_CALLBACK (properties_changed), object); + G_CALLBACK (properties_changed), object); g_ptr_array_add (priv->proxies, proxy); instance = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); @@ -1203,6 +1207,7 @@ nm_object_init (NMObject *object) NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object); c_list_init (&priv->notify_items); + c_list_init (&priv->pending); priv->proxies = g_ptr_array_new (); } From d6c1e565ff8fe57def5a2d4ac0d95fb3dd88e62f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 May 2017 12:38:57 +0200 Subject: [PATCH 08/14] libnm: remove unused code reload_results and reload_error from "nm-object.c" Fixes: 1f5b48a59eb46c40cb10bf4381b2b21a19a9f471 --- libnm/nm-object.c | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/libnm/nm-object.c b/libnm/nm-object.c index 8cc4674e5e..93784884cd 100644 --- a/libnm/nm-object.c +++ b/libnm/nm-object.c @@ -81,9 +81,7 @@ typedef struct { CList notify_items; guint32 notify_id; - GSList *reload_results; guint reload_remaining; - GError *reload_error; CList pending; /* ordered list of pending property updates. */ GPtrArray *proxies; @@ -1014,34 +1012,12 @@ static void reload_complete (NMObject *object, gboolean emit_now) { NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object); - GSimpleAsyncResult *simple; - GSList *results, *iter; - GError *error; if (emit_now) { nm_clear_g_source (&priv->notify_id); deferred_notify_cb (object); } else _nm_object_defer_notify (object); - - results = priv->reload_results; - priv->reload_results = NULL; - error = priv->reload_error; - priv->reload_error = NULL; - - for (iter = results; iter; iter = iter->next) { - simple = iter->data; - - if (error) - g_simple_async_result_set_from_error (simple, error); - else - g_simple_async_result_set_op_res_gboolean (simple, TRUE); - - g_simple_async_result_complete (simple); - g_object_unref (simple); - } - g_slist_free (results); - g_clear_error (&error); } GDBusObjectManager * From e6744bcb655f42699e4bf32520022c884e359a49 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 May 2017 12:40:05 +0200 Subject: [PATCH 09/14] libnm: fix type for "notify_id" source id in "nm-object.c" --- libnm/nm-object.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libnm/nm-object.c b/libnm/nm-object.c index 93784884cd..f292cfbcc1 100644 --- a/libnm/nm-object.c +++ b/libnm/nm-object.c @@ -79,7 +79,7 @@ typedef struct { * to defer their notifications by adding themselves here. */ CList notify_items; - guint32 notify_id; + guint notify_id; guint reload_remaining; From 8296de1823ea32896dddf006e11c555d34dedb82 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 May 2017 20:19:50 +0200 Subject: [PATCH 10/14] firewall: use CList to track pending_calls --- src/nm-firewall-manager.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/src/nm-firewall-manager.c b/src/nm-firewall-manager.c index 4a887b7994..b9878592a6 100644 --- a/src/nm-firewall-manager.c +++ b/src/nm-firewall-manager.c @@ -25,6 +25,7 @@ #include #include "NetworkManagerUtils.h" +#include "nm-utils/c-list.h" /*****************************************************************************/ @@ -39,7 +40,7 @@ typedef struct { GDBusProxy *proxy; GCancellable *proxy_cancellable; - GHashTable *pending_calls; + CList pending_calls; bool running; } NMFirewallManagerPrivate; @@ -76,6 +77,7 @@ typedef enum { } CBInfoMode; struct _NMFirewallManagerCallId { + CList lst; NMFirewallManager *self; CBInfoOpsType ops_type; union { @@ -176,8 +178,7 @@ _cb_info_create (NMFirewallManager *self, } else info->mode_mutable = CB_INFO_MODE_IDLE; - if (!nm_g_hash_table_add (priv->pending_calls, info)) - nm_assert_not_reached (); + c_list_link_tail (&priv->pending_calls, &info->lst); return info; } @@ -185,6 +186,7 @@ _cb_info_create (NMFirewallManager *self, static void _cb_info_free (CBInfo *info) { + c_list_unlink (&info->lst); if (info->mode != CB_INFO_MODE_IDLE) { if (info->dbus.arg) g_variant_unref (info->dbus.arg); @@ -209,8 +211,9 @@ _cb_info_complete_normal (CBInfo *info, GError *error) { NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (info->self); - if (!g_hash_table_remove (priv->pending_calls, info)) - g_return_if_reached (); + nm_assert (c_list_contains (&priv->pending_calls, &info->lst)); + + c_list_unlink_init (&info->lst); _cb_info_callback (info, error); _cb_info_free (info); @@ -423,8 +426,9 @@ nm_firewall_manager_cancel_call (NMFirewallManagerCallId call) self = info->self; priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self); - if (!g_hash_table_remove (priv->pending_calls, info)) - g_return_if_reached (); + nm_assert (c_list_contains (&priv->pending_calls, &info->lst)); + + c_list_unlink_init (&info->lst); nm_utils_error_set_cancelled (&error, FALSE, "NMFirewallManager"); @@ -488,8 +492,8 @@ _proxy_new_cb (GObject *source_object, NMFirewallManagerPrivate *priv; GDBusProxy *proxy; gs_free_error GError *error = NULL; - GHashTableIter iter; CBInfo *info; + CList *iter; proxy = g_dbus_proxy_new_for_bus_finish (result, &error); if ( !proxy @@ -513,8 +517,9 @@ _proxy_new_cb (GObject *source_object, _LOGD (NULL, "firewall %s", "initialized (not running)"); again: - g_hash_table_iter_init (&iter, priv->pending_calls); - while (g_hash_table_iter_next (&iter, (gpointer *) &info, NULL)) { + c_list_for_each (iter, &priv->pending_calls) { + info = c_list_entry (iter, CBInfo, lst); + if (info->mode != CB_INFO_MODE_DBUS_WAITING) continue; if (priv->running) { @@ -522,7 +527,7 @@ again: _handle_dbus_start (self, info); } else { _LOGD (info, "complete: fake success"); - g_hash_table_iter_remove (&iter); + c_list_unlink_init (&info->lst); _cb_info_callback (info, NULL); _cb_info_free (info); goto again; @@ -541,7 +546,7 @@ nm_firewall_manager_init (NMFirewallManager * self) { NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self); - priv->pending_calls = g_hash_table_new (g_direct_hash, g_direct_equal); + c_list_init (&priv->pending_calls); } static void @@ -572,13 +577,9 @@ dispose (GObject *object) NMFirewallManager *self = NM_FIREWALL_MANAGER (object); NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self); - if (priv->pending_calls) { - /* as every pending operation takes a reference to the manager, - * we don't expect pending operations at this point. */ - g_assert (g_hash_table_size (priv->pending_calls) == 0); - g_hash_table_unref (priv->pending_calls); - priv->pending_calls = NULL; - } + /* as every pending operation takes a reference to the manager, + * we don't expect pending operations at this point. */ + nm_assert (c_list_is_empty (&priv->pending_calls)); nm_clear_g_cancellable (&priv->proxy_cancellable); g_clear_object (&priv->proxy); From 04dfff7db8845e528c054906769ae0b0e45cd23d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 May 2017 20:28:44 +0200 Subject: [PATCH 11/14] secret-agent: use CList to track requests --- src/settings/nm-secret-agent.c | 41 +++++++++++++++++----------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index e493c387fa..5fe4dd1782 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -30,6 +30,7 @@ #include "nm-auth-subject.h" #include "nm-simple-connection.h" #include "NetworkManagerUtils.h" +#include "nm-utils/c-list.h" #include "introspection/org.freedesktop.NetworkManager.SecretAgent.h" @@ -58,7 +59,7 @@ typedef struct { gboolean connection_is_private; gulong on_disconnected_id; - GHashTable *requests; + CList requests; } NMSecretAgentPrivate; struct _NMSecretAgent { @@ -99,6 +100,7 @@ G_DEFINE_TYPE (NMSecretAgent, nm_secret_agent, G_TYPE_OBJECT) /*****************************************************************************/ struct _NMSecretAgentCallId { + CList lst; NMSecretAgent *agent; GCancellable *cancellable; char *path; @@ -129,6 +131,8 @@ request_new (NMSecretAgent *self, r->callback = callback; r->callback_data = callback_data; r->cancellable = g_cancellable_new (); + c_list_link_tail (&NM_SECRET_AGENT_GET_PRIVATE (self)->requests, + &r->lst); _LOGt ("request "LOG_REQ_FMT": created", LOG_REQ_ARG (r)); return r; } @@ -140,6 +144,7 @@ request_free (Request *r) NMSecretAgent *self = r->agent; _LOGt ("request "LOG_REQ_FMT": destroyed", LOG_REQ_ARG (r)); + c_list_unlink (&r->lst); g_free (r->path); g_free (r->setting_name); if (r->cancellable) @@ -150,17 +155,15 @@ request_free (Request *r) static gboolean request_check_return (Request *r) { - NMSecretAgentPrivate *priv; - if (!r->cancellable) return FALSE; g_return_val_if_fail (NM_IS_SECRET_AGENT (r->agent), FALSE); - priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); + nm_assert (c_list_contains (&NM_SECRET_AGENT_GET_PRIVATE (r->agent)->requests, + &r->lst)); - if (!g_hash_table_remove (priv->requests, r)) - g_return_val_if_reached (FALSE); + c_list_unlink_init (&r->lst); return TRUE; } @@ -373,7 +376,6 @@ nm_secret_agent_get_secrets (NMSecretAgent *self, r = request_new (self, "GetSecrets", path, setting_name, callback, callback_data); r->is_get_secrets = TRUE; - g_hash_table_add (priv->requests, r); /* Increase the timeout only for this call */ g_dbus_proxy_set_default_timeout (G_DBUS_PROXY (priv->proxy), 120000); @@ -467,15 +469,15 @@ do_cancel_secrets (NMSecretAgent *self, Request *r, gboolean disposing) void nm_secret_agent_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId call_id) { - NMSecretAgentPrivate *priv; Request *r = call_id; g_return_if_fail (NM_IS_SECRET_AGENT (self)); g_return_if_fail (r); - priv = NM_SECRET_AGENT_GET_PRIVATE (self); - if (!g_hash_table_remove (priv->requests, r)) - g_return_if_reached (); + nm_assert (c_list_contains (&NM_SECRET_AGENT_GET_PRIVATE (self)->requests, + &r->lst)); + + c_list_unlink_init (&r->lst); do_cancel_secrets (self, r, FALSE); } @@ -523,7 +525,6 @@ nm_secret_agent_save_secrets (NMSecretAgent *self, dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL); r = request_new (self, "SaveSecrets", path, NULL, callback, callback_data); - g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_save_secrets (priv->proxy, dict, path, @@ -576,7 +577,6 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self, dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_NO_SECRETS); r = request_new (self, "DeleteSecrets", path, NULL, callback, callback_data); - g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_delete_secrets (priv->proxy, dict, path, @@ -747,7 +747,7 @@ nm_secret_agent_init (NMSecretAgent *self) { NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); - priv->requests = g_hash_table_new (g_direct_hash, g_direct_equal); + c_list_init (&priv->requests); } static void @@ -755,13 +755,13 @@ dispose (GObject *object) { NMSecretAgent *self = NM_SECRET_AGENT (object); NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); - GHashTableIter iter; - Request *r; + CList *iter; - g_hash_table_iter_init (&iter, priv->requests); - while (g_hash_table_iter_next (&iter, (gpointer *) &r, NULL)) { - g_hash_table_iter_remove (&iter); - do_cancel_secrets (self, r, TRUE); +again: + c_list_for_each (iter, &priv->requests) { + c_list_unlink_init (iter); + do_cancel_secrets (self, c_list_entry (iter, Request, lst), TRUE); + goto again; } _on_disconnected_cleanup (priv); @@ -783,7 +783,6 @@ finalize (GObject *object) g_free (priv->dbus_owner); g_slist_free_full (priv->permissions, g_free); - g_hash_table_destroy (priv->requests); G_OBJECT_CLASS (nm_secret_agent_parent_class)->finalize (object); From a844b98259705f786bf8fec984447d4413661f68 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 10 May 2017 10:01:26 +0200 Subject: [PATCH 12/14] agent-manager/trivial: move code --- src/settings/nm-agent-manager.c | 94 +++++++++++++++++---------------- 1 file changed, 48 insertions(+), 46 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 3d6b1cfba5..ce5e595419 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -155,6 +155,54 @@ _request_type_to_string (RequestType request_type, gboolean verbose) /*****************************************************************************/ +struct _NMAgentManagerCallId { + NMAgentManager *self; + + RequestType request_type; + + char *detail; + + NMAuthSubject *subject; + + /* Current agent being asked for secrets */ + NMSecretAgent *current; + NMSecretAgentCallId current_call_id; + + /* Stores the sorted list of NMSecretAgents which will be asked for secrets */ + GSList *pending; + + guint idle_id; + + union { + struct { + char *path; + NMConnection *connection; + + NMAuthChain *chain; + + /* Whether the agent currently being asked for secrets + * has the system.modify privilege. + */ + gboolean current_has_modify; + + union { + struct { + NMSecretAgentGetSecretsFlags flags; + char *setting_name; + char **hints; + + GVariant *existing_secrets; + + NMAgentSecretsResultFunc callback; + gpointer callback_data; + } get; + }; + } con; + }; +}; + +/*****************************************************************************/ + static gboolean remove_agent (NMAgentManager *self, const char *owner) { @@ -450,52 +498,6 @@ done: /*****************************************************************************/ -struct _NMAgentManagerCallId { - NMAgentManager *self; - - RequestType request_type; - - char *detail; - - NMAuthSubject *subject; - - /* Current agent being asked for secrets */ - NMSecretAgent *current; - NMSecretAgentCallId current_call_id; - - /* Stores the sorted list of NMSecretAgents which will be asked for secrets */ - GSList *pending; - - guint idle_id; - - union { - struct { - char *path; - NMConnection *connection; - - NMAuthChain *chain; - - /* Whether the agent currently being asked for secrets - * has the system.modify privilege. - */ - gboolean current_has_modify; - - union { - struct { - NMSecretAgentGetSecretsFlags flags; - char *setting_name; - char **hints; - - GVariant *existing_secrets; - - NMAgentSecretsResultFunc callback; - gpointer callback_data; - } get; - }; - } con; - }; -}; - static Request * request_new (NMAgentManager *self, RequestType request_type, From e8279256460d6e77288d953db294c80456a2e6d9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 May 2017 20:28:44 +0200 Subject: [PATCH 13/14] agent-manager: use CList to track requests --- src/settings/nm-agent-manager.c | 94 +++++++++++++-------------------- 1 file changed, 37 insertions(+), 57 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index ce5e595419..137f40329f 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -36,6 +36,7 @@ #include "nm-simple-connection.h" #include "NetworkManagerUtils.h" #include "nm-core-internal.h" +#include "nm-utils/c-list.h" #include "introspection/org.freedesktop.NetworkManager.AgentManager.h" @@ -50,6 +51,7 @@ static guint signals[LAST_SIGNAL] = { 0 }; typedef struct { NMAuthManager *auth_mgr; + NMSessionMonitor *session_monitor; /* Auth chains for checking agent permissions */ GSList *chains; @@ -59,7 +61,7 @@ typedef struct { */ GHashTable *agents; - GHashTable *requests; + CList requests; } NMAgentManagerPrivate; struct _NMAgentManager { @@ -123,7 +125,7 @@ typedef struct _NMAgentManagerCallId Request; static void request_add_agent (Request *req, NMSecretAgent *agent); -static void request_remove_agent (Request *req, NMSecretAgent *agent, GSList **pending_reqs); +static void request_remove_agent (Request *req, NMSecretAgent *agent); static void request_next_agent (Request *req); @@ -156,6 +158,8 @@ _request_type_to_string (RequestType request_type, gboolean verbose) /*****************************************************************************/ struct _NMAgentManagerCallId { + CList lst_request; + NMAgentManager *self; RequestType request_type; @@ -208,9 +212,7 @@ remove_agent (NMAgentManager *self, const char *owner) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); NMSecretAgent *agent; - GHashTableIter iter; - gpointer data; - GSList *pending_reqs = NULL; + CList *iter, *safe; g_return_val_if_fail (owner != NULL, FALSE); @@ -222,16 +224,8 @@ remove_agent (NMAgentManager *self, const char *owner) _LOGD (agent, "agent unregistered or disappeared"); /* Remove this agent from any in-progress secrets requests */ - g_hash_table_iter_init (&iter, priv->requests); - while (g_hash_table_iter_next (&iter, &data, NULL)) - request_remove_agent ((Request *) data, agent, &pending_reqs); - - /* We cannot call request_next_agent() from within hash iterating loop, - * because it may remove the request from the hash table, which invalidates - * the iterator. So, only remove the agent from requests. And store the requests - * that should be sent to other agent to a temporary list to proceed afterwards. - */ - g_slist_free_full (pending_reqs, (GDestroyNotify) request_next_agent); + c_list_for_each_safe (iter, safe, &priv->requests) + request_remove_agent (c_list_entry (iter, Request, lst_request), agent); /* And dispose of the agent */ g_hash_table_remove (priv->agents, owner); @@ -318,8 +312,7 @@ agent_register_permissions_done (NMAuthChain *chain, const char *sender; GError *local = NULL; NMAuthCallResult result; - GHashTableIter iter; - Request *req; + CList *iter; g_assert (context); @@ -352,9 +345,8 @@ agent_register_permissions_done (NMAuthChain *chain, g_signal_emit (self, signals[AGENT_REGISTERED], 0, agent); /* Add this agent to any in-progress secrets requests */ - g_hash_table_iter_init (&iter, priv->requests); - while (g_hash_table_iter_next (&iter, (gpointer) &req, NULL)) - request_add_agent (req, agent); + c_list_for_each (iter, &priv->requests) + request_add_agent (c_list_entry (iter, Request, lst_request), agent); } nm_auth_chain_unref (chain); @@ -511,6 +503,7 @@ request_new (NMAgentManager *self, req->request_type = request_type; req->detail = g_strdup (detail); req->subject = g_object_ref (subject); + c_list_link_tail (&NM_AGENT_MANAGER_GET_PRIVATE (self)->requests, &req->lst_request); return req; } @@ -599,7 +592,7 @@ req_complete_cancel (Request *req, gboolean is_disposing) gs_free_error GError *error = NULL; nm_assert (req && req->self); - nm_assert (!g_hash_table_contains (req->self->_priv.requests, req)); + nm_assert (!c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (req->self)->requests, &req->lst_request)); nm_utils_error_set_cancelled (&error, is_disposing, "NMAgentManager"); req_complete_release (req, NULL, NULL, NULL, error); @@ -613,10 +606,11 @@ req_complete (Request *req, GError *error) { NMAgentManager *self = req->self; - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); - if (!g_hash_table_remove (priv->requests, req)) - g_return_if_reached (); + nm_assert (c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (self)->requests, &req->lst_request)); + + c_list_unlink_init (&req->lst_request); + req_complete_release (req, secrets, agent_dbus_owner, agent_username, error); } @@ -632,6 +626,7 @@ agent_compare_func (gconstpointer aa, gconstpointer bb, gpointer user_data) NMSecretAgent *a = (NMSecretAgent *)aa; NMSecretAgent *b = (NMSecretAgent *)bb; Request *req = user_data; + NMSessionMonitor *sm; gboolean a_active, b_active; gulong a_pid, b_pid, requester; @@ -650,8 +645,9 @@ agent_compare_func (gconstpointer aa, gconstpointer bb, gpointer user_data) } /* Prefer agents in active sessions */ - a_active = nm_session_monitor_session_exists (nm_session_monitor_get (), nm_secret_agent_get_owner_uid (a), TRUE); - b_active = nm_session_monitor_session_exists (nm_session_monitor_get (), nm_secret_agent_get_owner_uid (b), TRUE); + sm = NM_AGENT_MANAGER_GET_PRIVATE (req->self)->session_monitor; + a_active = nm_session_monitor_session_exists (sm, nm_secret_agent_get_owner_uid (a), TRUE); + b_active = nm_session_monitor_session_exists (sm, nm_secret_agent_get_owner_uid (b), TRUE); if (a_active && !b_active) return -1; else if (a_active == b_active) @@ -736,7 +732,7 @@ request_next_agent (Request *req) nm_secret_agent_cancel_secrets (req->current, req->current_call_id); g_clear_object (&req->current); } - g_warn_if_fail (!req->current_call_id); + nm_assert (!req->current_call_id); if (req->pending) { /* Send the request to the next agent */ @@ -771,7 +767,7 @@ request_next_agent (Request *req) } static void -request_remove_agent (Request *req, NMSecretAgent *agent, GSList **pending_reqs) +request_remove_agent (Request *req, NMSecretAgent *agent) { NMAgentManager *self; @@ -800,7 +796,7 @@ request_remove_agent (Request *req, NMSecretAgent *agent, GSList **pending_reqs) g_assert_not_reached (); } - *pending_reqs = g_slist_prepend (*pending_reqs, req); + request_next_agent (req); } else if (g_slist_find (req->pending, agent)) { req->pending = g_slist_remove (req->pending, agent); @@ -1221,7 +1217,6 @@ nm_agent_manager_get_secrets (NMAgentManager *self, NMAgentSecretsResultFunc callback, gpointer callback_data) { - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); Request *req; g_return_val_if_fail (self != NULL, NULL); @@ -1255,9 +1250,6 @@ nm_agent_manager_get_secrets (NMAgentManager *self, req->con.get.callback = callback; req->con.get.callback_data = callback_data; - if (!nm_g_hash_table_add (priv->requests, req)) - g_assert_not_reached (); - /* Kick off the request */ if (!(req->con.get.flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM)) request_add_agents (self, req); @@ -1273,9 +1265,9 @@ nm_agent_manager_cancel_secrets (NMAgentManager *self, g_return_if_fail (request_id); g_return_if_fail (request_id->request_type == REQUEST_TYPE_CON_GET); - if (!g_hash_table_remove (NM_AGENT_MANAGER_GET_PRIVATE (self)->requests, - request_id)) - g_return_if_reached (); + nm_assert (c_list_contains (&NM_AGENT_MANAGER_GET_PRIVATE (self)->requests, &request_id->lst_request)); + + c_list_unlink_init (&request_id->lst_request); req_complete_cancel (request_id, FALSE); } @@ -1343,7 +1335,6 @@ nm_agent_manager_save_secrets (NMAgentManager *self, NMConnection *connection, NMAuthSubject *subject) { - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); Request *req; g_return_if_fail (self); @@ -1361,8 +1352,6 @@ nm_agent_manager_save_secrets (NMAgentManager *self, subject); req->con.path = g_strdup (path); req->con.connection = g_object_ref (connection); - if (!nm_g_hash_table_add (priv->requests, req)) - g_assert_not_reached (); /* Kick off the request */ request_add_agents (self, req); @@ -1428,7 +1417,6 @@ nm_agent_manager_delete_secrets (NMAgentManager *self, const char *path, NMConnection *connection) { - NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); NMAuthSubject *subject; Request *req; @@ -1449,8 +1437,6 @@ nm_agent_manager_delete_secrets (NMAgentManager *self, req->con.path = g_strdup (path); req->con.connection = g_object_ref (connection); g_object_unref (subject); - if (!nm_g_hash_table_add (priv->requests, req)) - g_assert_not_reached (); /* Kick off the request */ request_add_agents (self, req); @@ -1572,8 +1558,8 @@ nm_agent_manager_init (NMAgentManager *self) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); + c_list_init (&priv->requests); priv->agents = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_object_unref); - priv->requests = g_hash_table_new (g_direct_hash, g_direct_equal); } static void @@ -1584,6 +1570,7 @@ constructed (GObject *object) G_OBJECT_CLASS (nm_agent_manager_parent_class)->constructed (object); priv->auth_mgr = g_object_ref (nm_auth_manager_get ()); + priv->session_monitor = g_object_ref (nm_session_monitor_get ()); nm_exported_object_export (NM_EXPORTED_OBJECT (object)); @@ -1591,28 +1578,19 @@ constructed (GObject *object) NM_AUTH_MANAGER_SIGNAL_CHANGED, G_CALLBACK (authority_changed_cb), object); - - NM_UTILS_KEEP_ALIVE (object, nm_session_monitor_get (), "NMAgentManager-depends-on-NMSessionMonitor"); } static void dispose (GObject *object) { NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE ((NMAgentManager *) object); - - if (priv->requests) { - GHashTableIter iter; - Request *req; + CList *iter; cancel_more: - g_hash_table_iter_init (&iter, priv->requests); - if (g_hash_table_iter_next (&iter, (gpointer *) &req, NULL)) { - g_hash_table_iter_remove (&iter); - req_complete_cancel (req, TRUE); - goto cancel_more; - } - g_hash_table_unref (priv->requests); - priv->requests = NULL; + c_list_for_each (iter, &priv->requests) { + c_list_unlink_init (iter); + req_complete_cancel (c_list_entry (iter, Request, lst_request), TRUE); + goto cancel_more; } g_slist_free_full (priv->chains, (GDestroyNotify) nm_auth_chain_unref); @@ -1632,6 +1610,8 @@ cancel_more: nm_exported_object_unexport (NM_EXPORTED_OBJECT (object)); + g_clear_object (&priv->session_monitor); + G_OBJECT_CLASS (nm_agent_manager_parent_class)->dispose (object); } From 8894e8c61b94af764e0a0e23f1a76d5e32ce5b0c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 May 2017 12:04:24 +0200 Subject: [PATCH 14/14] proxy: use CList to track configs in NMPacrunnnerManager - config->removed can be replaced by c_list_is_empty(&config->lst) - downgrade some assertions to nm_assert(). Even without the assert we crash a few lines later with a NULL pointer access. That gives almost the same debuggability and discoverability of the bug. - use exact type signature for GAsyncReadyCallback and avoid casting. - when the name owner disappears, cancel all asynchronous operations. Note how the new pacrunner instance will anyway start without configuration, so for all intended purpose, all pending operations are at that moment obsolete. --- src/nm-pacrunner-manager.c | 173 +++++++++++++++++++++---------------- 1 file changed, 98 insertions(+), 75 deletions(-) diff --git a/src/nm-pacrunner-manager.c b/src/nm-pacrunner-manager.c index 87e0a36437..0a1cd823f4 100644 --- a/src/nm-pacrunner-manager.c +++ b/src/nm-pacrunner-manager.c @@ -27,8 +27,7 @@ #include "nm-proxy-config.h" #include "nm-ip4-config.h" #include "nm-ip6-config.h" - -static void pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data); +#include "nm-utils/c-list.h" #define PACRUNNER_DBUS_SERVICE "org.pacrunner" #define PACRUNNER_DBUS_INTERFACE "org.pacrunner.Manager" @@ -37,11 +36,15 @@ static void pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointe /*****************************************************************************/ struct _NMPacrunnerCallId { - NMPacrunnerManager *manager; + CList lst; + + /* this might be a dangling pointer after the async operation + * is cancelled. */ + NMPacrunnerManager *manager_maybe_dangling; + GVariant *args; char *path; guint refcount; - bool removed; }; typedef struct _NMPacrunnerCallId Config; @@ -49,8 +52,8 @@ typedef struct _NMPacrunnerCallId Config; typedef struct { char *iface; GDBusProxy *pacrunner; - GCancellable *pacrunner_cancellable; - GList *configs; + GCancellable *cancellable; + CList configs; } NMPacrunnerManagerPrivate; struct _NMPacrunnerManager { @@ -80,49 +83,59 @@ NM_DEFINE_SINGLETON_GETTER (NMPacrunnerManager, nm_pacrunner_manager_get, NM_TYP G_STMT_START { \ nm_log ((level), _NMLOG_DOMAIN, NULL, NULL, \ "%s%p]: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ - "pacrunner: call[", \ + _NMLOG2_PREFIX_NAME": call[", \ (config) \ _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ } G_STMT_END /*****************************************************************************/ +static void pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data); + +/*****************************************************************************/ + static Config * config_new (NMPacrunnerManager *manager, GVariant *args) { Config *config; config = g_slice_new0 (Config); - config->manager = manager; + config->manager_maybe_dangling = manager; config->args = g_variant_ref_sink (args); config->refcount = 1; + c_list_link_tail (&NM_PACRUNNER_MANAGER_GET_PRIVATE (manager)->configs, + &config->lst); return config; } -static void +static Config * config_ref (Config *config) { - g_assert (config); - g_assert (config->refcount > 0); + nm_assert (config); + nm_assert (config->refcount > 0); config->refcount++; + return config; } static void config_unref (Config *config) { - g_assert (config); - g_assert (config->refcount > 0); + nm_assert (config); + nm_assert (config->refcount > 0); if (config->refcount == 1) { g_variant_unref (config->args); g_free (config->path); + c_list_unlink (&config->lst); g_slice_free (Config, config); } else config->refcount--; } +/*****************************************************************************/ + static void add_proxy_config (GVariantBuilder *proxy_data, const NMProxyConfig *proxy_config) { @@ -220,8 +233,18 @@ get_ip6_domains (GPtrArray *domains, NMIP6Config *ip6) } } +/*****************************************************************************/ + +static GCancellable * +_ensure_cancellable (NMPacrunnerManagerPrivate *priv) +{ + if (G_UNLIKELY (!priv->cancellable)) + priv->cancellable = g_cancellable_new (); + return priv->cancellable; +} + static void -pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) +pacrunner_send_done (GObject *source, GAsyncResult *res, gpointer user_data) { Config *config = user_data; NMPacrunnerManager *self; @@ -230,15 +253,13 @@ pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) gs_unref_variant GVariant *variant = NULL; const char *path = NULL; - g_return_if_fail (!config->path); + nm_assert (!config->path); - variant = g_dbus_proxy_call_finish (proxy, res, &error); - if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - config_unref (config); - return; - } + variant = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, &error); + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + goto out; - self = NM_PACRUNNER_MANAGER (config->manager); + self = NM_PACRUNNER_MANAGER (config->manager_maybe_dangling); priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); if (!variant) @@ -246,21 +267,23 @@ pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) else { g_variant_get (variant, "(&o)", &path); - config->path = g_strdup (path); - _LOG2D (config, "sent"); - - if (config->removed) { - config_ref (config); + if (c_list_is_empty (&config->lst)) { + _LOG2D (config, "sent (%s), but destory it right away", path); g_dbus_proxy_call (priv->pacrunner, "DestroyProxyConfiguration", - g_variant_new ("(o)", config->path), + g_variant_new ("(o)", path), G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, - priv->pacrunner_cancellable, - (GAsyncReadyCallback) pacrunner_remove_done, - config); + _ensure_cancellable (priv), + pacrunner_remove_done, + config_ref (config)); + } else { + _LOG2D (config, "sent (%s)", path); + config->path = g_strdup (path); } } + +out: config_unref (config); } @@ -272,17 +295,15 @@ pacrunner_send_config (NMPacrunnerManager *self, Config *config) if (priv->pacrunner) { _LOG2T (config, "sending..."); - config_ref (config); - g_clear_pointer (&config->path, g_free); - + nm_assert (!config->path); g_dbus_proxy_call (priv->pacrunner, "CreateProxyConfiguration", config->args, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, - priv->pacrunner_cancellable, - (GAsyncReadyCallback) pacrunner_send_done, - config); + _ensure_cancellable (priv), + pacrunner_send_done, + config_ref (config)); } } @@ -291,15 +312,18 @@ name_owner_changed (NMPacrunnerManager *self) { NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); gs_free char *owner = NULL; - GList *iter = NULL; + CList *iter; owner = g_dbus_proxy_get_name_owner (priv->pacrunner); if (owner) { _LOGD ("name owner appeared (%s)", owner); - for (iter = g_list_first (priv->configs); iter; iter = g_list_next (iter)) - pacrunner_send_config (self, iter->data); + c_list_for_each (iter, &priv->configs) + pacrunner_send_config (self, c_list_entry (iter, Config, lst)); } else { _LOGD ("name owner disappeared"); + nm_clear_g_cancellable (&priv->cancellable); + c_list_for_each (iter, &priv->configs) + nm_clear_g_free (&c_list_entry (iter, Config, lst)->path); } } @@ -316,21 +340,19 @@ pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data) { NMPacrunnerManager *self = user_data; NMPacrunnerManagerPrivate *priv; - GError *error = NULL; + gs_free_error GError *error = NULL; GDBusProxy *proxy; proxy = g_dbus_proxy_new_for_bus_finish (res, &error); if (!proxy) { if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - _LOGW ("failed to connect to pacrunner via DBus: %s", error->message); - g_error_free (error); + _LOGE ("failed to create D-Bus proxy for pacrunner: %s", error->message); return; } priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); priv->pacrunner = proxy; - g_signal_connect (priv->pacrunner, "notify::g-name-owner", G_CALLBACK (name_owner_changed_cb), self); name_owner_changed (self); @@ -420,7 +442,6 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self, } config = config_new (self, g_variant_new ("(a{sv})", &proxy_data)); - priv->configs = g_list_append (priv->configs, config); { gs_free char *args_str = NULL; @@ -439,26 +460,24 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self, } static void -pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data) +pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data) { Config *config = user_data; NMPacrunnerManager *self; gs_free_error GError *error = NULL; gs_unref_variant GVariant *ret = NULL; - ret = g_dbus_proxy_call_finish (proxy, res, &error); - if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - config_unref (config); - return; - } - - self = NM_PACRUNNER_MANAGER (config->manager); + ret = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, &error); + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + goto out; + self = NM_PACRUNNER_MANAGER (config->manager_maybe_dangling); if (!ret) _LOG2D (config, "remove failed: %s", error->message); else _LOG2D (config, "removed"); +out: config_unref (config); } @@ -472,7 +491,6 @@ nm_pacrunner_manager_remove (NMPacrunnerManager *self, NMPacrunnerCallId *call_i { NMPacrunnerManagerPrivate *priv; Config *config; - GList *list; g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self)); g_return_if_fail (call_id); @@ -482,31 +500,29 @@ nm_pacrunner_manager_remove (NMPacrunnerManager *self, NMPacrunnerCallId *call_i _LOG2T (config, "removing..."); - list = g_list_find (priv->configs, config); - if (!list) - g_return_if_reached (); + nm_assert (c_list_contains (&priv->configs, &config->lst)); if (priv->pacrunner) { if (!config->path) { - /* send() failed or is still pending. Mark the item as - * removed, so that we ask pacrunner to drop it when the - * send() completes. + /* send() failed or is still pending. The item is unlinked from + * priv->configs, so pacrunner_send_done() knows to call + * DestroyProxyConfiguration right away. */ - config->removed = TRUE; - config_unref (config); } else { g_dbus_proxy_call (priv->pacrunner, "DestroyProxyConfiguration", g_variant_new ("(o)", config->path), G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, - priv->pacrunner_cancellable, - (GAsyncReadyCallback) pacrunner_remove_done, - config); + _ensure_cancellable (priv), + pacrunner_remove_done, + config_ref (config)); + nm_clear_g_free (&config->path); } - } else - config_unref (config); - priv->configs = g_list_delete_link (priv->configs, list); + } + + c_list_unlink_init (&config->lst); + config_unref (config); } gboolean @@ -532,16 +548,15 @@ nm_pacrunner_manager_init (NMPacrunnerManager *self) { NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); - priv->pacrunner_cancellable = g_cancellable_new (); - + c_list_init (&priv->configs); g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, NULL, PACRUNNER_DBUS_SERVICE, PACRUNNER_DBUS_PATH, PACRUNNER_DBUS_INTERFACE, - priv->pacrunner_cancellable, - (GAsyncReadyCallback) pacrunner_proxy_cb, + _ensure_cancellable (priv), + pacrunner_proxy_cb, self); } @@ -549,14 +564,22 @@ static void dispose (GObject *object) { NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE ((NMPacrunnerManager *) object); + CList *iter, *safe; + + c_list_for_each_safe (iter, safe, &priv->configs) { + c_list_unlink_init (iter); + config_unref (c_list_entry (iter, Config, lst)); + } + + /* we cancel all pending operations. Note that pacrunner automatically + * removes all configuration once NetworkManager disconnects from + * the bus -- which happens soon after we destroy the pacrunner manager. + */ + nm_clear_g_cancellable (&priv->cancellable); g_clear_pointer (&priv->iface, g_free); - nm_clear_g_cancellable (&priv->pacrunner_cancellable); g_clear_object (&priv->pacrunner); - g_list_free_full (priv->configs, (GDestroyNotify) config_unref); - priv->configs = NULL; - G_OBJECT_CLASS (nm_pacrunner_manager_parent_class)->dispose (object); }