From 78aad6cf51ebd55b754b5a916158c6459b36191a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 17:18:17 +0100 Subject: [PATCH 01/23] glib-aux: add "name_ptr" union field to NMUtilsNamedValue NMUtilsNamedValue is a key-value tuple, usually the key is a string (hence the name "Named"). But this struct is also useful for keys that are not strings. Add another "name_ptr" union field to access the key that way. The alternative would be to add another struct, which serves a very similar purpose though. --- src/libnm-glib-aux/nm-shared-utils.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 941312bd0e..6e78c5f928 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1977,6 +1977,7 @@ typedef struct { NMUtilsNamedEntry named_entry; const char *name; char *name_mutable; + gpointer name_ptr; }; union { const char *value_str; From de926723f0d0ef07e6f905bfa920fbbb4ffd246e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 17:18:32 +0100 Subject: [PATCH 02/23] glib-aux: add nm_utils_hash_to_array() helper We effectively already have this function, with the name nm_utils_named_values_from_strdict(). Which is a decent name, if you have a strdict. But it seems odd to use for other dictionaries. Instead, add a variant with a different name. Naming is important, and just to have the better name, the function is effectively duplicated. --- src/libnm-glib-aux/nm-shared-utils.c | 91 ++++++++++++++-------------- src/libnm-glib-aux/nm-shared-utils.h | 53 +++++++++++++--- 2 files changed, 91 insertions(+), 53 deletions(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index ad99a6b929..1da2a68293 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -3432,51 +3432,6 @@ nm_utils_named_value_clear_with_g_free(NMUtilsNamedValue *val) G_STATIC_ASSERT(G_STRUCT_OFFSET(NMUtilsNamedValue, name) == 0); -NMUtilsNamedValue * -nm_utils_named_values_from_strdict_full(GHashTable *hash, - guint *out_len, - GCompareDataFunc compare_func, - gpointer user_data, - NMUtilsNamedValue *provided_buffer, - guint provided_buffer_len, - NMUtilsNamedValue **out_allocated_buffer) -{ - GHashTableIter iter; - NMUtilsNamedValue *values; - guint i, len; - - nm_assert(provided_buffer_len == 0 || provided_buffer); - nm_assert(!out_allocated_buffer || !*out_allocated_buffer); - - if (!hash || !(len = g_hash_table_size(hash))) { - NM_SET_OUT(out_len, 0); - return NULL; - } - - if (provided_buffer_len >= len + 1) { - /* the buffer provided by the caller is large enough. Use it. */ - values = provided_buffer; - } else { - /* allocate a new buffer. */ - values = g_new(NMUtilsNamedValue, len + 1); - NM_SET_OUT(out_allocated_buffer, values); - } - - i = 0; - g_hash_table_iter_init(&iter, hash); - while (g_hash_table_iter_next(&iter, (gpointer *) &values[i].name, &values[i].value_ptr)) - i++; - nm_assert(i == len); - values[i].name = NULL; - values[i].value_ptr = NULL; - - if (compare_func) - nm_utils_named_value_list_sort(values, len, compare_func, user_data); - - NM_SET_OUT(out_len, len); - return values; -} - gssize nm_utils_named_value_list_find(const NMUtilsNamedValue *arr, gsize len, @@ -3626,6 +3581,52 @@ nm_utils_hash_values_to_array(GHashTable *hash, return arr; } +NMUtilsNamedValue * +nm_utils_hash_to_array_full(GHashTable *hash, + guint *out_len, + GCompareDataFunc compare_func, + gpointer user_data, + NMUtilsNamedValue *provided_buffer, + guint provided_buffer_len, + NMUtilsNamedValue **out_allocated_buffer) +{ + GHashTableIter iter; + NMUtilsNamedValue *values; + guint len; + guint i; + + nm_assert(provided_buffer_len == 0 || provided_buffer); + nm_assert(!out_allocated_buffer || !*out_allocated_buffer); + + if (!hash || ((len = g_hash_table_size(hash)) == 0)) { + NM_SET_OUT(out_len, 0); + return NULL; + } + + if (provided_buffer_len >= len + 1) { + /* the buffer provided by the caller is large enough. Use it. */ + values = provided_buffer; + } else { + /* allocate a new buffer. */ + values = g_new(NMUtilsNamedValue, len + 1); + NM_SET_OUT(out_allocated_buffer, values); + } + + i = 0; + g_hash_table_iter_init(&iter, hash); + while (g_hash_table_iter_next(&iter, &values[i].name_ptr, &values[i].value_ptr)) + i++; + nm_assert(i == len); + values[i].name_ptr = NULL; + values[i].value_ptr = NULL; + + if (compare_func && len > 1) + g_qsort_with_data(values, len, sizeof(NMUtilsNamedValue), compare_func, user_data); + + NM_SET_OUT(out_len, len); + return values; +} + /*****************************************************************************/ /** diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 6e78c5f928..0d2403ca66 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1991,14 +1991,28 @@ typedef struct { .name = (n), .value_ptr = (v) \ } -NMUtilsNamedValue * -nm_utils_named_values_from_strdict_full(GHashTable *hash, - guint *out_len, - GCompareDataFunc compare_func, - gpointer user_data, - NMUtilsNamedValue *provided_buffer, - guint provided_buffer_len, - NMUtilsNamedValue **out_allocated_buffer); +NMUtilsNamedValue *nm_utils_hash_to_array_full(GHashTable *hash, + guint *out_len, + GCompareDataFunc compare_func, + gpointer user_data, + NMUtilsNamedValue *provided_buffer, + guint provided_buffer_len, + NMUtilsNamedValue **out_allocated_buffer); + +#define nm_utils_named_values_from_strdict_full(hash, \ + out_len, \ + compare_func, \ + user_data, \ + provided_buffer, \ + provided_buffer_len, \ + out_allocated_buffer) \ + nm_utils_hash_to_array_full((hash), \ + (out_len), \ + (compare_func), \ + (user_data), \ + (provided_buffer), \ + (provided_buffer_len), \ + (out_allocated_buffer)) #define nm_utils_named_values_from_strdict(hash, out_len, array, out_allocated_buffer) \ nm_utils_named_values_from_strdict_full((hash), \ @@ -2039,6 +2053,29 @@ gpointer *nm_utils_hash_values_to_array(GHashTable *hash, gpointer user_data, guint *out_len); +static inline NMUtilsNamedValue * +nm_utils_hash_to_array(GHashTable *hash, + GCompareDataFunc compare_func, + gpointer user_data, + guint *out_len) +{ + return nm_utils_hash_to_array_full(hash, out_len, compare_func, user_data, NULL, 0, NULL); +} + +#define nm_utils_hash_to_array_with_buffer(hash, \ + out_len, \ + compare_func, \ + user_data, \ + array, \ + out_allocated_buffer) \ + nm_utils_hash_to_array_full((hash), \ + (out_len), \ + (compare_func), \ + (user_data), \ + (array), \ + G_N_ELEMENTS(array), \ + (out_allocated_buffer)) + static inline const char ** nm_strdict_get_keys(const GHashTable *hash, gboolean sorted, guint *out_length) { From bd95a5c0ec00d6a52df2e3c0da5fb221eb5052f9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Apr 2022 21:57:32 +0200 Subject: [PATCH 03/23] dns: register NMDnsPlugin instance as wait-obj for shutdown nm_shutdown_wait_obj_register_object() today has no practical effect. In the future it will block shutdown until the object gets destroyed. We will want that NMDnsPlugin gets wrapped up during shut down, before quitting. --- src/core/dns/nm-dns-plugin.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/dns/nm-dns-plugin.c b/src/core/dns/nm-dns-plugin.c index 847d783996..4d1cd8c2aa 100644 --- a/src/core/dns/nm-dns-plugin.c +++ b/src/core/dns/nm-dns-plugin.c @@ -106,7 +106,9 @@ nm_dns_plugin_stop(NMDnsPlugin *self) static void nm_dns_plugin_init(NMDnsPlugin *self) -{} +{ + nm_shutdown_wait_obj_register_object(self, "dns-plugin"); +} static void nm_dns_plugin_class_init(NMDnsPluginClass *plugin_class) From f7b41fc18c45d9357ca1a1fe64ed1e34f18666ee Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 8 Apr 2022 12:45:42 +0200 Subject: [PATCH 04/23] dns: avoid printing pointer value for NMDnsManager logging statements We avoid printing raw pointer values. Also, in this case this is a singleton, and we only create one instance of this type. Note that we would still have printed the pointer instance while constructing the instances, before setting it as singleton. Just drop this. --- src/core/dns/nm-dns-manager.c | 39 +++++++++++++++-------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/src/core/dns/nm-dns-manager.c b/src/core/dns/nm-dns-manager.c index 566f3d6626..f50a6c2418 100644 --- a/src/core/dns/nm-dns-manager.c +++ b/src/core/dns/nm-dns-manager.c @@ -137,28 +137,23 @@ NM_DEFINE_SINGLETON_GETTER(NMDnsManager, nm_dns_manager_get, NM_TYPE_DNS_MANAGER #define _NMLOG_PREFIX_NAME "dns-mgr" #define _NMLOG_DOMAIN LOGD_DNS -#define _NMLOG(level, ...) \ - G_STMT_START \ - { \ - const NMLogLevel __level = (level); \ - \ - if (nm_logging_enabled(__level, _NMLOG_DOMAIN)) { \ - char __prefix[20]; \ - const NMDnsManager *const __self = (self); \ - \ - _nm_log(__level, \ - _NMLOG_DOMAIN, \ - 0, \ - NULL, \ - NULL, \ - "%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ - _NMLOG_PREFIX_NAME, \ - ((!__self || __self == singleton_instance) \ - ? "" \ - : nm_sprintf_buf(__prefix, "[%p]", __self)) \ - _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ - } \ - } \ +#define _NMLOG(level, ...) \ + G_STMT_START \ + { \ + const NMLogLevel __level = (level); \ + \ + if (nm_logging_enabled(__level, _NMLOG_DOMAIN)) { \ + _nm_unused const NMDnsManager *const __self = (self); \ + \ + _nm_log(__level, \ + _NMLOG_DOMAIN, \ + 0, \ + NULL, \ + NULL, \ + "%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + _NMLOG_PREFIX_NAME _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } \ + } \ G_STMT_END /*****************************************************************************/ From 068ca09d161b374c979a381c177e020626624755 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 8 Apr 2022 12:49:32 +0200 Subject: [PATCH 05/23] dns: obfuscate pointer value for NMDnsPlugin logging --- src/core/dns/nm-dns-plugin.c | 43 +++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/core/dns/nm-dns-plugin.c b/src/core/dns/nm-dns-plugin.c index 4d1cd8c2aa..9c905b21af 100644 --- a/src/core/dns/nm-dns-plugin.c +++ b/src/core/dns/nm-dns-plugin.c @@ -32,26 +32,29 @@ G_DEFINE_ABSTRACT_TYPE(NMDnsPlugin, nm_dns_plugin, G_TYPE_OBJECT) #define _NMLOG_PREFIX_NAME "dns-plugin" #define _NMLOG_DOMAIN LOGD_DNS -#define _NMLOG(level, ...) \ - G_STMT_START \ - { \ - const NMLogLevel __level = (level); \ - \ - if (nm_logging_enabled(__level, _NMLOG_DOMAIN)) { \ - char __prefix[20]; \ - const NMDnsPlugin *const __self = (self); \ - \ - _nm_log(__level, \ - _NMLOG_DOMAIN, \ - 0, \ - NULL, \ - NULL, \ - "%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ - _NMLOG_PREFIX_NAME, \ - (!__self ? "" : nm_sprintf_buf(__prefix, "[%p]", __self)) \ - _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ - } \ - } \ +#define _NMLOG(level, ...) \ + G_STMT_START \ + { \ + const NMLogLevel __level = (level); \ + \ + if (nm_logging_enabled(__level, _NMLOG_DOMAIN)) { \ + char __prefix[20]; \ + const NMDnsPlugin *const __self = (self); \ + \ + _nm_log(__level, \ + _NMLOG_DOMAIN, \ + 0, \ + NULL, \ + NULL, \ + "%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + _NMLOG_PREFIX_NAME, \ + (!__self ? "" \ + : nm_sprintf_buf(__prefix, \ + "[" NM_HASH_OBFUSCATE_PTR_FMT "]", \ + NM_HASH_OBFUSCATE_PTR( \ + __self))) _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } \ + } \ G_STMT_END /*****************************************************************************/ From 0001a2fd0c0d48f90762d05986ad10a1ceaa9a0c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Apr 2022 13:29:33 +0200 Subject: [PATCH 06/23] dns: fix NMDnsPluginPrivate and drop unused fields NM_DNS_PLUGIN_GET_PRIVATE() macro was broken. Also NMDnsPluginPrivate contained unused fields. Fix that. The private data is unused at the moment, but will be used next. Hence it is fixed and not removed. --- src/core/dns/nm-dns-plugin.c | 19 +++++++++++++------ src/core/dns/nm-dns-plugin.h | 5 ++++- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/core/dns/nm-dns-plugin.c b/src/core/dns/nm-dns-plugin.c index 9c905b21af..371e515860 100644 --- a/src/core/dns/nm-dns-plugin.c +++ b/src/core/dns/nm-dns-plugin.c @@ -18,10 +18,7 @@ /*****************************************************************************/ typedef struct _NMDnsPluginPrivate { - GPid pid; - guint watch_id; - char *progname; - char *pidfile; + int _dummy; } NMDnsPluginPrivate; G_DEFINE_ABSTRACT_TYPE(NMDnsPlugin, nm_dns_plugin, G_TYPE_OBJECT) @@ -110,9 +107,19 @@ nm_dns_plugin_stop(NMDnsPlugin *self) static void nm_dns_plugin_init(NMDnsPlugin *self) { + NMDnsPluginPrivate *priv; + + priv = G_TYPE_INSTANCE_GET_PRIVATE(self, NM_TYPE_DNS_PLUGIN, NMDnsPluginPrivate); + + self->_priv = priv; + nm_shutdown_wait_obj_register_object(self, "dns-plugin"); } static void -nm_dns_plugin_class_init(NMDnsPluginClass *plugin_class) -{} +nm_dns_plugin_class_init(NMDnsPluginClass *klass) +{ + GObjectClass *object_class = G_OBJECT_CLASS(klass); + + g_type_class_add_private(object_class, sizeof(NMDnsPluginPrivate)); +} diff --git a/src/core/dns/nm-dns-plugin.h b/src/core/dns/nm-dns-plugin.h index f9c424abfa..e527576952 100644 --- a/src/core/dns/nm-dns-plugin.h +++ b/src/core/dns/nm-dns-plugin.h @@ -19,8 +19,11 @@ #define NM_DNS_PLUGIN_GET_CLASS(obj) \ (G_TYPE_INSTANCE_GET_CLASS((obj), NM_TYPE_DNS_PLUGIN, NMDnsPluginClass)) +struct _NMDnsPluginPrivate; + typedef struct { - GObject parent; + GObject parent; + struct _NMDnsPluginPrivate *_priv; } NMDnsPlugin; typedef struct { From f68230fbe96be0fc69de242725c9a71be4cb0c6b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 8 Apr 2022 11:19:54 +0200 Subject: [PATCH 07/23] dns: call nm_dns_plugin_stop() also for NMDnsSystemdResolved instance Currently NMDnsSystemdResolved does not implement "stop()". That is about to change. Make sure to call stop before unreferencing the instance. --- src/core/dns/nm-dns-manager.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/core/dns/nm-dns-manager.c b/src/core/dns/nm-dns-manager.c index f50a6c2418..916cee18cd 100644 --- a/src/core/dns/nm-dns-manager.c +++ b/src/core/dns/nm-dns-manager.c @@ -2122,6 +2122,19 @@ _clear_plugin(NMDnsManager *self) return FALSE; } +static gboolean +_clear_sd_resolved_plugin(NMDnsManager *self) +{ + NMDnsManagerPrivate *priv = NM_DNS_MANAGER_GET_PRIVATE(self); + + if (priv->sd_resolve_plugin) { + nm_dns_plugin_stop(priv->sd_resolve_plugin); + g_clear_object(&priv->sd_resolve_plugin); + return TRUE; + } + return FALSE; +} + static NMDnsManagerResolvConfManager _check_resconf_immutable(NMDnsManagerResolvConfManager rc_manager) { @@ -2354,7 +2367,7 @@ again: priv->sd_resolve_plugin = nm_dns_systemd_resolved_new(); systemd_resolved_changed = TRUE; } - } else if (nm_clear_g_object(&priv->sd_resolve_plugin)) + } else if (_clear_sd_resolved_plugin(self)) systemd_resolved_changed = TRUE; g_object_freeze_notify(G_OBJECT(self)); @@ -2636,7 +2649,7 @@ dispose(GObject *object) if (priv->config) g_signal_handlers_disconnect_by_func(priv->config, config_changed_cb, self); - g_clear_object(&priv->sd_resolve_plugin); + _clear_sd_resolved_plugin(self); _clear_plugin(self); c_list_for_each_entry_safe (ip_data, ip_data_safe, &priv->ip_data_lst_head, ip_data_lst) From b7ca08e971567c4c114a06a9ec6f2bff3cfe4bcc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 19:56:31 +0100 Subject: [PATCH 08/23] dns: add "update-pending" state to NMDnsPlugin Theoretically, this should be a GObject property, and not a signal. But then I'd also have to implement the get_property() function, which is more hazzle than necessary. A signal will do nicely. --- src/core/dns/nm-dns-plugin.c | 97 +++++++++++++++++++++++++++++++++++- src/core/dns/nm-dns-plugin.h | 8 +++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/src/core/dns/nm-dns-plugin.c b/src/core/dns/nm-dns-plugin.c index 371e515860..41a0dbc138 100644 --- a/src/core/dns/nm-dns-plugin.c +++ b/src/core/dns/nm-dns-plugin.c @@ -17,8 +17,16 @@ /*****************************************************************************/ +enum { + UPDATE_PENDING_CHANGED, + LAST_SIGNAL, +}; + +static guint signals[LAST_SIGNAL] = {0}; + typedef struct _NMDnsPluginPrivate { - int _dummy; + bool update_pending_inited : 1; + bool update_pending : 1; } NMDnsPluginPrivate; G_DEFINE_ABSTRACT_TYPE(NMDnsPlugin, nm_dns_plugin, G_TYPE_OBJECT) @@ -104,6 +112,79 @@ nm_dns_plugin_stop(NMDnsPlugin *self) /*****************************************************************************/ +static gboolean +_get_update_pending(NMDnsPlugin *self) +{ + NMDnsPluginClass *klass; + + nm_assert(NM_IS_DNS_PLUGIN(self)); + + klass = NM_DNS_PLUGIN_GET_CLASS(self); + if (klass->get_update_pending) { + if (klass->get_update_pending(self)) + return TRUE; + } + return FALSE; +} + +gboolean +nm_dns_plugin_get_update_pending(NMDnsPlugin *self) +{ + NMDnsPluginPrivate *priv; + + g_return_val_if_fail(NM_IS_DNS_PLUGIN(self), FALSE); + + priv = NM_DNS_PLUGIN_GET_PRIVATE(self); + + /* We cache the boolean and rely on the subclass to call + * _nm_dns_plugin_update_pending_maybe_changed(). The subclass + * anyway must get it right to notify us when the value (maybe) + * changes. By caching the value, the subclass is free to notify + * even if the value did not actually change. + * + * Also, this allows the base implementation to combine multiple + * sources/reasons (if we need that in the future). */ + + if (!priv->update_pending_inited) { + priv->update_pending_inited = TRUE; + priv->update_pending = _get_update_pending(self); + _LOGD("[%s] update-pending changed (%spending)", + nm_dns_plugin_get_name(self), + priv->update_pending ? "" : "not "); + } else + nm_assert(priv->update_pending == _get_update_pending(self)); + + return priv->update_pending; +} + +void +_nm_dns_plugin_update_pending_maybe_changed(NMDnsPlugin *self) +{ + NMDnsPluginPrivate *priv; + gboolean v; + + g_return_if_fail(NM_IS_DNS_PLUGIN(self)); + + priv = NM_DNS_PLUGIN_GET_PRIVATE(self); + + v = _get_update_pending(self); + + if (!priv->update_pending_inited) + priv->update_pending_inited = TRUE; + else if (priv->update_pending == v) + return; + + priv->update_pending = v; + + _LOGD("[%s] update-pending changed (%spending)", + nm_dns_plugin_get_name(self), + priv->update_pending ? "" : "not "); + + g_signal_emit(self, signals[UPDATE_PENDING_CHANGED], 0, (gboolean) priv->update_pending); +} + +/*****************************************************************************/ + static void nm_dns_plugin_init(NMDnsPlugin *self) { @@ -113,6 +194,9 @@ nm_dns_plugin_init(NMDnsPlugin *self) self->_priv = priv; + nm_assert(priv->update_pending_inited == FALSE); + nm_assert(priv->update_pending == FALSE); + nm_shutdown_wait_obj_register_object(self, "dns-plugin"); } @@ -122,4 +206,15 @@ nm_dns_plugin_class_init(NMDnsPluginClass *klass) GObjectClass *object_class = G_OBJECT_CLASS(klass); g_type_class_add_private(object_class, sizeof(NMDnsPluginPrivate)); + + signals[UPDATE_PENDING_CHANGED] = g_signal_new(NM_DNS_PLUGIN_UPDATE_PENDING_CHANGED, + G_OBJECT_CLASS_TYPE(klass), + G_SIGNAL_RUN_FIRST, + 0, + NULL, + NULL, + NULL, + G_TYPE_NONE, + 1, + G_TYPE_BOOLEAN); } diff --git a/src/core/dns/nm-dns-plugin.h b/src/core/dns/nm-dns-plugin.h index e527576952..24d6083be1 100644 --- a/src/core/dns/nm-dns-plugin.h +++ b/src/core/dns/nm-dns-plugin.h @@ -19,6 +19,8 @@ #define NM_DNS_PLUGIN_GET_CLASS(obj) \ (G_TYPE_INSTANCE_GET_CLASS((obj), NM_TYPE_DNS_PLUGIN, NMDnsPluginClass)) +#define NM_DNS_PLUGIN_UPDATE_PENDING_CHANGED "update-pending-changed" + struct _NMDnsPluginPrivate; typedef struct { @@ -42,6 +44,8 @@ typedef struct { void (*stop)(NMDnsPlugin *self); + gboolean (*get_update_pending)(NMDnsPlugin *self); + const char *plugin_name; /* Types should set to TRUE if they start a local caching nameserver @@ -66,4 +70,8 @@ gboolean nm_dns_plugin_update(NMDnsPlugin *self, void nm_dns_plugin_stop(NMDnsPlugin *self); +gboolean nm_dns_plugin_get_update_pending(NMDnsPlugin *self); + +void _nm_dns_plugin_update_pending_maybe_changed(NMDnsPlugin *self); + #endif /* __NM_DNS_PLUGIN_H__ */ From a60b97100341757c1894cfb0d6a5e0422a8e526a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 20:29:42 +0100 Subject: [PATCH 09/23] dns: add update-pending property to NMDnsManager --- src/core/dns/nm-dns-manager.c | 87 ++++++++++++++++++++++++++++++++++- src/core/dns/nm-dns-manager.h | 9 ++-- 2 files changed, 92 insertions(+), 4 deletions(-) diff --git a/src/core/dns/nm-dns-manager.c b/src/core/dns/nm-dns-manager.c index 916cee18cd..03f3eceddf 100644 --- a/src/core/dns/nm-dns-manager.c +++ b/src/core/dns/nm-dns-manager.c @@ -78,7 +78,11 @@ enum { LAST_SIGNAL }; -NM_GOBJECT_PROPERTIES_DEFINE(NMDnsManager, PROP_MODE, PROP_RC_MANAGER, PROP_CONFIGURATION, ); +NM_GOBJECT_PROPERTIES_DEFINE(NMDnsManager, + PROP_MODE, + PROP_RC_MANAGER, + PROP_CONFIGURATION, + PROP_UPDATE_PENDING, ); static guint signals[LAST_SIGNAL] = {0}; @@ -98,6 +102,8 @@ typedef struct { bool config_changed : 1; + bool update_pending : 1; + char *hostdomain; guint updates_queue; @@ -109,6 +115,9 @@ typedef struct { NMDnsPlugin *sd_resolve_plugin; NMDnsPlugin *plugin; + gulong update_changed_signal_id_sd; + gulong update_changed_signal_id; + NMConfig *config; struct { @@ -202,6 +211,53 @@ static NM_UTILS_LOOKUP_STR_DEFINE( /*****************************************************************************/ +static gboolean +_update_pending_detect(NMDnsManager *self) +{ + NMDnsManagerPrivate *priv = NM_DNS_MANAGER_GET_PRIVATE(self); + + if (priv->plugin && nm_dns_plugin_get_update_pending(priv->plugin)) + return TRUE; + if (priv->sd_resolve_plugin && nm_dns_plugin_get_update_pending(priv->sd_resolve_plugin)) + return TRUE; + return FALSE; +} + +static void +_update_pending_maybe_changed(NMDnsManager *self) +{ + NMDnsManagerPrivate *priv = NM_DNS_MANAGER_GET_PRIVATE(self); + gboolean update_pending; + + update_pending = _update_pending_detect(self); + if (priv->update_pending == update_pending) + return; + + priv->update_pending = update_pending; + _LOGD("update-pending changed: %spending", update_pending ? "" : "not "); + _notify(self, PROP_UPDATE_PENDING); +} + +static void +_update_pending_changed_cb(NMDnsPlugin *plugin, gboolean update_pending, NMDnsManager *self) +{ + _update_pending_maybe_changed(self); +} + +gboolean +nm_dns_manager_get_update_pending(NMDnsManager *self) +{ + NMDnsManagerPrivate *priv; + + g_return_val_if_fail(NM_IS_DNS_MANAGER(self), FALSE); + + priv = NM_DNS_MANAGER_GET_PRIVATE(self); + nm_assert(priv->update_pending == _update_pending_detect(self)); + return priv->update_pending; +} + +/*****************************************************************************/ + static int _dns_config_ip_data_get_dns_priority1(const NML3ConfigData *l3cd, int addr_family) { @@ -2115,6 +2171,7 @@ _clear_plugin(NMDnsManager *self) nm_clear_g_source(&priv->plugin_ratelimit.timer); if (priv->plugin) { + nm_clear_g_signal_handler(priv->plugin, &priv->update_changed_signal_id); nm_dns_plugin_stop(priv->plugin); g_clear_object(&priv->plugin); return TRUE; @@ -2128,6 +2185,7 @@ _clear_sd_resolved_plugin(NMDnsManager *self) NMDnsManagerPrivate *priv = NM_DNS_MANAGER_GET_PRIVATE(self); if (priv->sd_resolve_plugin) { + nm_clear_g_signal_handler(priv->sd_resolve_plugin, &priv->update_changed_signal_id_sd); nm_dns_plugin_stop(priv->sd_resolve_plugin); g_clear_object(&priv->sd_resolve_plugin); return TRUE; @@ -2398,6 +2456,23 @@ again: "")); } + if (plugin_changed && priv->plugin && priv->update_changed_signal_id == 0) { + priv->update_changed_signal_id = g_signal_connect(priv->plugin, + NM_DNS_PLUGIN_UPDATE_PENDING_CHANGED, + G_CALLBACK(_update_pending_changed_cb), + self); + } + + if (systemd_resolved_changed && priv->sd_resolve_plugin + && priv->update_changed_signal_id_sd == 0) { + priv->update_changed_signal_id_sd = g_signal_connect(priv->sd_resolve_plugin, + NM_DNS_PLUGIN_UPDATE_PENDING_CHANGED, + G_CALLBACK(_update_pending_changed_cb), + self); + } + + _update_pending_maybe_changed(self); + g_object_thaw_notify(G_OBJECT(self)); } @@ -2602,6 +2677,9 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) case PROP_CONFIGURATION: g_value_set_variant(value, _get_config_variant(self)); break; + case PROP_UPDATE_PENDING: + g_value_set_boolean(value, nm_dns_manager_get_update_pending(self)); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; @@ -2727,6 +2805,13 @@ nm_dns_manager_class_init(NMDnsManagerClass *klass) NULL, G_PARAM_READABLE | G_PARAM_STATIC_STRINGS); + obj_properties[PROP_UPDATE_PENDING] = + g_param_spec_boolean(NM_DNS_MANAGER_UPDATE_PENDING, + "", + "", + FALSE, + G_PARAM_READABLE | G_PARAM_STATIC_STRINGS); + g_object_class_install_properties(object_class, _PROPERTY_ENUMS_LAST, obj_properties); signals[CONFIG_CHANGED] = g_signal_new(NM_DNS_MANAGER_CONFIG_CHANGED, diff --git a/src/core/dns/nm-dns-manager.h b/src/core/dns/nm-dns-manager.h index c30d4b3ac6..210f9f6cb3 100644 --- a/src/core/dns/nm-dns-manager.h +++ b/src/core/dns/nm-dns-manager.h @@ -80,9 +80,10 @@ typedef struct _NMDnsConfigData { (G_TYPE_INSTANCE_GET_CLASS((o), NM_TYPE_DNS_MANAGER, NMDnsManagerClass)) /* properties */ -#define NM_DNS_MANAGER_MODE "mode" -#define NM_DNS_MANAGER_RC_MANAGER "rc-manager" -#define NM_DNS_MANAGER_CONFIGURATION "configuration" +#define NM_DNS_MANAGER_MODE "mode" +#define NM_DNS_MANAGER_RC_MANAGER "rc-manager" +#define NM_DNS_MANAGER_CONFIGURATION "configuration" +#define NM_DNS_MANAGER_UPDATE_PENDING "update-pending" /* internal signals */ #define NM_DNS_MANAGER_CONFIG_CHANGED "config-changed" @@ -149,6 +150,8 @@ void nm_dns_manager_stop(NMDnsManager *self); NMDnsPlugin *nm_dns_manager_get_systemd_resolved(NMDnsManager *self); +gboolean nm_dns_manager_get_update_pending(NMDnsManager *self); + /*****************************************************************************/ char *nmtst_dns_create_resolv_conf(const char *const *searches, From 4564adfb53148aadd8e8e330a0a0b05fc468a7bf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 17:17:47 +0100 Subject: [PATCH 10/23] dns/resolved: minor cleanups in "nm-dns-systemd-resolved.c" --- src/core/dns/nm-dns-systemd-resolved.c | 52 +++++++++++++++----------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/src/core/dns/nm-dns-systemd-resolved.c b/src/core/dns/nm-dns-systemd-resolved.c index c4993884d2..300706e8a9 100644 --- a/src/core/dns/nm-dns-systemd-resolved.c +++ b/src/core/dns/nm-dns-systemd-resolved.c @@ -179,7 +179,7 @@ static void _interface_config_free(InterfaceConfig *config) { nm_c_list_elem_free_all(&config->configs_lst_head, NULL); - g_slice_free(InterfaceConfig, config); + nm_g_slice_free(config); } static void @@ -258,8 +258,12 @@ update_add_ip_config(NMDnsSystemdResolved *self, addr_size = nm_utils_addr_family_to_size(ip_data->addr_family); if ((!ip_data->domains.search || !ip_data->domains.search[0]) - && !ip_data->domains.has_default_route_exclusive && !ip_data->domains.has_default_route) + && !ip_data->domains.has_default_route_exclusive && !ip_data->domains.has_default_route) { + /* we have no search domain (which systemd-resolved uses to routing the request), but + * also the "DefaultRoute" is not set on the interface. This setting has no effect and + * gets ignored. */ return FALSE; + } nameservers = nm_l3_config_data_get_nameservers(ip_data->l3cd, ip_data->addr_family, &n); for (i = 0; i < n; i++) { @@ -306,10 +310,12 @@ prepare_one_interface(NMDnsSystemdResolved *self, InterfaceConfig *ic) GVariantBuilder dns; GVariantBuilder domains; NMCListElem *elem; - NMSettingConnectionMdns mdns = NM_SETTING_CONNECTION_MDNS_DEFAULT; - NMSettingConnectionLlmnr llmnr = NM_SETTING_CONNECTION_LLMNR_DEFAULT; - NMSettingConnectionDnsOverTls dns_over_tls = NM_SETTING_CONNECTION_DNS_OVER_TLS_DEFAULT; - const char *mdns_arg = NULL, *llmnr_arg = NULL, *dns_over_tls_arg = NULL; + NMSettingConnectionMdns mdns = NM_SETTING_CONNECTION_MDNS_DEFAULT; + NMSettingConnectionLlmnr llmnr = NM_SETTING_CONNECTION_LLMNR_DEFAULT; + NMSettingConnectionDnsOverTls dns_over_tls = NM_SETTING_CONNECTION_DNS_OVER_TLS_DEFAULT; + const char *mdns_arg = NULL; + const char *llmnr_arg = NULL; + const char *dns_over_tls_arg = NULL; gboolean has_config = FALSE; gboolean has_default_route = FALSE; @@ -324,7 +330,8 @@ prepare_one_interface(NMDnsSystemdResolved *self, InterfaceConfig *ic) c_list_for_each_entry (elem, &ic->configs_lst_head, lst) { NMDnsConfigIPData *ip_data = elem->data; - has_config |= update_add_ip_config(self, &dns, &domains, ip_data); + if (update_add_ip_config(self, &dns, &domains, ip_data)) + has_config = TRUE; if (ip_data->domains.has_default_route) has_default_route = TRUE; @@ -559,19 +566,19 @@ update(NMDnsPlugin *plugin, gs_unref_hashtable GHashTable *interfaces = NULL; gs_free gpointer *interfaces_keys = NULL; guint interfaces_len; - int ifindex; gpointer pointer; NMDnsConfigIPData *ip_data; GHashTableIter iter; guint i; + /* Group configs by ifindex/interfaces. */ interfaces = g_hash_table_new_full(nm_direct_hash, NULL, NULL, (GDestroyNotify) _interface_config_free); c_list_for_each_entry (ip_data, ip_data_lst_head, ip_data_lst) { - InterfaceConfig *ic = NULL; + InterfaceConfig *ic = NULL; + int ifindex = ip_data->data->ifindex; - ifindex = ip_data->data->ifindex; nm_assert(ifindex == nm_l3_config_data_get_ifindex(ip_data->l3cd)); ic = g_hash_table_lookup(interfaces, GINT_TO_POINTER(ifindex)); @@ -602,19 +609,22 @@ update(NMDnsPlugin *plugin, * resolved, and the current update doesn't contain that interface, * reset the resolved configuration for that ifindex. */ g_hash_table_iter_init(&iter, priv->dirty_interfaces); - while (g_hash_table_iter_next(&iter, (gpointer *) &pointer, NULL)) { - ifindex = GPOINTER_TO_INT(pointer); - if (!g_hash_table_contains(interfaces, GINT_TO_POINTER(ifindex))) { - InterfaceConfig ic; + while (g_hash_table_iter_next(&iter, &pointer, NULL)) { + int ifindex = GPOINTER_TO_INT(pointer); + InterfaceConfig ic; - _LOGT("clear previously configured ifindex %d", ifindex); - ic = (InterfaceConfig){ - .ifindex = ifindex, - .configs_lst_head = C_LIST_INIT(ic.configs_lst_head), - }; - prepare_one_interface(self, &ic); - g_hash_table_iter_remove(&iter); + if (g_hash_table_contains(interfaces, GINT_TO_POINTER(ifindex))) { + /* the interface is still tracked and still dirty. Keep. */ + continue; } + + _LOGT("clear previously configured ifindex %d", ifindex); + ic = (InterfaceConfig){ + .ifindex = ifindex, + .configs_lst_head = C_LIST_INIT(ic.configs_lst_head), + }; + prepare_one_interface(self, &ic); + g_hash_table_iter_remove(&iter); } priv->send_updates_waiting = TRUE; From 39b68d72d358b3c7bbb66f6a328ae96372d078b4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 18:28:53 +0100 Subject: [PATCH 11/23] dns/resolved: add const to parameters in "nm-dns-systemd-resolved.c" --- src/core/dns/nm-dns-systemd-resolved.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/core/dns/nm-dns-systemd-resolved.c b/src/core/dns/nm-dns-systemd-resolved.c index 300706e8a9..9ac549d4ef 100644 --- a/src/core/dns/nm-dns-systemd-resolved.c +++ b/src/core/dns/nm-dns-systemd-resolved.c @@ -242,10 +242,10 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) } static gboolean -update_add_ip_config(NMDnsSystemdResolved *self, - GVariantBuilder *dns, - GVariantBuilder *domains, - NMDnsConfigIPData *ip_data) +update_add_ip_config(NMDnsSystemdResolved *self, + GVariantBuilder *dns, + GVariantBuilder *domains, + const NMDnsConfigIPData *ip_data) { gsize addr_size; guint n; @@ -305,7 +305,7 @@ free_pending_updates(NMDnsSystemdResolved *self) } static gboolean -prepare_one_interface(NMDnsSystemdResolved *self, InterfaceConfig *ic) +prepare_one_interface(NMDnsSystemdResolved *self, const InterfaceConfig *ic) { GVariantBuilder dns; GVariantBuilder domains; @@ -328,7 +328,7 @@ prepare_one_interface(NMDnsSystemdResolved *self, InterfaceConfig *ic) g_variant_builder_open(&domains, G_VARIANT_TYPE("a(sb)")); c_list_for_each_entry (elem, &ic->configs_lst_head, lst) { - NMDnsConfigIPData *ip_data = elem->data; + const NMDnsConfigIPData *ip_data = elem->data; if (update_add_ip_config(self, &dns, &domains, ip_data)) has_config = TRUE; @@ -597,7 +597,8 @@ update(NMDnsPlugin *plugin, interfaces_keys = nm_utils_hash_keys_to_array(interfaces, nm_cmp_int2ptr_p_with_data, NULL, &interfaces_len); for (i = 0; i < interfaces_len; i++) { - InterfaceConfig *ic = g_hash_table_lookup(interfaces, GINT_TO_POINTER(interfaces_keys[i])); + const InterfaceConfig *ic = + g_hash_table_lookup(interfaces, GINT_TO_POINTER(interfaces_keys[i])); if (prepare_one_interface(self, ic)) g_hash_table_add(priv->dirty_interfaces, GINT_TO_POINTER(ic->ifindex)); From 51cec67253a014569f7807b5df56a58a7d4f30ae Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 18:48:26 +0100 Subject: [PATCH 12/23] dns/resolved: sort dirty interfaces to prune in "nm-dns-systemd-resolved.c" When we do something where the order makes a visible difference, we should do it in a consistent way, that does not depend on arbitray things. Sort the ifindexes from dirty_interfaces hash table. --- src/core/dns/nm-dns-systemd-resolved.c | 28 ++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/core/dns/nm-dns-systemd-resolved.c b/src/core/dns/nm-dns-systemd-resolved.c index 9ac549d4ef..00928504be 100644 --- a/src/core/dns/nm-dns-systemd-resolved.c +++ b/src/core/dns/nm-dns-systemd-resolved.c @@ -569,6 +569,7 @@ update(NMDnsPlugin *plugin, gpointer pointer; NMDnsConfigIPData *ip_data; GHashTableIter iter; + gs_unref_array GArray *dirty_array = NULL; guint i; /* Group configs by ifindex/interfaces. */ @@ -611,22 +612,33 @@ update(NMDnsPlugin *plugin, * reset the resolved configuration for that ifindex. */ g_hash_table_iter_init(&iter, priv->dirty_interfaces); while (g_hash_table_iter_next(&iter, &pointer, NULL)) { - int ifindex = GPOINTER_TO_INT(pointer); - InterfaceConfig ic; + int ifindex = GPOINTER_TO_INT(pointer); if (g_hash_table_contains(interfaces, GINT_TO_POINTER(ifindex))) { /* the interface is still tracked and still dirty. Keep. */ continue; } - _LOGT("clear previously configured ifindex %d", ifindex); - ic = (InterfaceConfig){ - .ifindex = ifindex, - .configs_lst_head = C_LIST_INIT(ic.configs_lst_head), - }; - prepare_one_interface(self, &ic); + if (!dirty_array) + dirty_array = g_array_new(FALSE, FALSE, sizeof(int)); + g_array_append_val(dirty_array, ifindex); + g_hash_table_iter_remove(&iter); } + if (dirty_array) { + g_array_sort_with_data(dirty_array, nm_cmp_int2ptr_p_with_data, NULL); + for (i = 0; i < dirty_array->len; i++) { + int ifindex = g_array_index(dirty_array, int, i); + InterfaceConfig ic; + + _LOGT("clear previously configured ifindex %d", ifindex); + ic = (InterfaceConfig){ + .ifindex = ifindex, + .configs_lst_head = C_LIST_INIT(ic.configs_lst_head), + }; + prepare_one_interface(self, &ic); + } + } priv->send_updates_waiting = TRUE; send_updates(self); From eb25c9ecd2728ab3cd99c8c02f5fbae2242b31fd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 18:22:15 +0100 Subject: [PATCH 13/23] dns/resolved: use nm_utils_hash_to_array_with_buffer() in NMDnsSystemdResolved's update() We copy the content of the hash table to an array, so that we can sort the entries and they have a defined order. We are not only interested in the keys, but the keys and the values. Hence, use nm_utils_hash_to_array_with_buffer() which gives both at the same time. --- src/core/dns/nm-dns-systemd-resolved.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/core/dns/nm-dns-systemd-resolved.c b/src/core/dns/nm-dns-systemd-resolved.c index 00928504be..581ddcb4e1 100644 --- a/src/core/dns/nm-dns-systemd-resolved.c +++ b/src/core/dns/nm-dns-systemd-resolved.c @@ -561,10 +561,12 @@ update(NMDnsPlugin *plugin, const char *hostdomain, GError **error) { - NMDnsSystemdResolved *self = NM_DNS_SYSTEMD_RESOLVED(plugin); - NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); - gs_unref_hashtable GHashTable *interfaces = NULL; - gs_free gpointer *interfaces_keys = NULL; + NMDnsSystemdResolved *self = NM_DNS_SYSTEMD_RESOLVED(plugin); + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + gs_unref_hashtable GHashTable *interfaces = NULL; + const NMUtilsNamedValue *interfaces_arr; + NMUtilsNamedValue interfaces_arr_stack[50]; + gs_free NMUtilsNamedValue *interfaces_arr_heap = NULL; guint interfaces_len; gpointer pointer; NMDnsConfigIPData *ip_data; @@ -595,11 +597,14 @@ update(NMDnsPlugin *plugin, free_pending_updates(self); - interfaces_keys = - nm_utils_hash_keys_to_array(interfaces, nm_cmp_int2ptr_p_with_data, NULL, &interfaces_len); + interfaces_arr = nm_utils_hash_to_array_with_buffer(interfaces, + &interfaces_len, + nm_cmp_int2ptr_p_with_data, + NULL, + interfaces_arr_stack, + &interfaces_arr_heap); for (i = 0; i < interfaces_len; i++) { - const InterfaceConfig *ic = - g_hash_table_lookup(interfaces, GINT_TO_POINTER(interfaces_keys[i])); + const InterfaceConfig *ic = interfaces_arr[i].value_ptr; if (prepare_one_interface(self, ic)) g_hash_table_add(priv->dirty_interfaces, GINT_TO_POINTER(ic->ifindex)); From 2f1feb9651beb887683520eff0b4cedbed77931a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 19:04:17 +0100 Subject: [PATCH 14/23] dns/resolved: use GPtrArray to collect ip datas in NMDnsSystemdResolved's update() CList is a great, simple data structure. Especially, if we can embed it into the data we track. Here we just create a (temporary) list of pointers. A GPtrArray is the better data structure for that. --- src/core/dns/nm-dns-systemd-resolved.c | 45 ++++++++++++++------------ 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/src/core/dns/nm-dns-systemd-resolved.c b/src/core/dns/nm-dns-systemd-resolved.c index 581ddcb4e1..e3d38ed44a 100644 --- a/src/core/dns/nm-dns-systemd-resolved.c +++ b/src/core/dns/nm-dns-systemd-resolved.c @@ -40,8 +40,8 @@ static const char *const DBUS_OP_SET_LINK_DNS_OVER_TLS = "SetLinkDNSOverTLS"; /*****************************************************************************/ typedef struct { - int ifindex; - CList configs_lst_head; + int ifindex; + GPtrArray *ip_data_list; } InterfaceConfig; typedef struct { @@ -178,7 +178,7 @@ _request_item_append(NMDnsSystemdResolved *self, static void _interface_config_free(InterfaceConfig *config) { - nm_c_list_elem_free_all(&config->configs_lst_head, NULL); + nm_g_ptr_array_unref(config->ip_data_list); nm_g_slice_free(config); } @@ -309,7 +309,6 @@ prepare_one_interface(NMDnsSystemdResolved *self, const InterfaceConfig *ic) { GVariantBuilder dns; GVariantBuilder domains; - NMCListElem *elem; NMSettingConnectionMdns mdns = NM_SETTING_CONNECTION_MDNS_DEFAULT; NMSettingConnectionLlmnr llmnr = NM_SETTING_CONNECTION_LLMNR_DEFAULT; NMSettingConnectionDnsOverTls dns_over_tls = NM_SETTING_CONNECTION_DNS_OVER_TLS_DEFAULT; @@ -318,6 +317,7 @@ prepare_one_interface(NMDnsSystemdResolved *self, const InterfaceConfig *ic) const char *dns_over_tls_arg = NULL; gboolean has_config = FALSE; gboolean has_default_route = FALSE; + guint i; g_variant_builder_init(&dns, G_VARIANT_TYPE("(ia(iay))")); g_variant_builder_add(&dns, "i", ic->ifindex); @@ -327,19 +327,22 @@ prepare_one_interface(NMDnsSystemdResolved *self, const InterfaceConfig *ic) g_variant_builder_add(&domains, "i", ic->ifindex); g_variant_builder_open(&domains, G_VARIANT_TYPE("a(sb)")); - c_list_for_each_entry (elem, &ic->configs_lst_head, lst) { - const NMDnsConfigIPData *ip_data = elem->data; + if (ic->ip_data_list) { + for (i = 0; i < ic->ip_data_list->len; i++) { + const NMDnsConfigIPData *ip_data = ic->ip_data_list->pdata[i]; - if (update_add_ip_config(self, &dns, &domains, ip_data)) - has_config = TRUE; + if (update_add_ip_config(self, &dns, &domains, ip_data)) + has_config = TRUE; - if (ip_data->domains.has_default_route) - has_default_route = TRUE; + if (ip_data->domains.has_default_route) + has_default_route = TRUE; - if (NM_IS_IPv4(ip_data->addr_family)) { - mdns = NM_MAX(mdns, nm_l3_config_data_get_mdns(ip_data->l3cd)); - llmnr = NM_MAX(llmnr, nm_l3_config_data_get_llmnr(ip_data->l3cd)); - dns_over_tls = NM_MAX(dns_over_tls, nm_l3_config_data_get_dns_over_tls(ip_data->l3cd)); + if (NM_IS_IPv4(ip_data->addr_family)) { + mdns = NM_MAX(mdns, nm_l3_config_data_get_mdns(ip_data->l3cd)); + llmnr = NM_MAX(llmnr, nm_l3_config_data_get_llmnr(ip_data->l3cd)); + dns_over_tls = + NM_MAX(dns_over_tls, nm_l3_config_data_get_dns_over_tls(ip_data->l3cd)); + } } } @@ -586,13 +589,15 @@ update(NMDnsPlugin *plugin, ic = g_hash_table_lookup(interfaces, GINT_TO_POINTER(ifindex)); if (!ic) { - ic = g_slice_new(InterfaceConfig); - ic->ifindex = ifindex; - c_list_init(&ic->configs_lst_head); + ic = g_slice_new(InterfaceConfig); + *ic = (InterfaceConfig){ + .ifindex = ifindex, + .ip_data_list = g_ptr_array_sized_new(4), + }; g_hash_table_insert(interfaces, GINT_TO_POINTER(ifindex), ic); } - c_list_link_tail(&ic->configs_lst_head, &nm_c_list_elem_new_stale(ip_data)->lst); + g_ptr_array_add(ic->ip_data_list, ip_data); } free_pending_updates(self); @@ -638,8 +643,8 @@ update(NMDnsPlugin *plugin, _LOGT("clear previously configured ifindex %d", ifindex); ic = (InterfaceConfig){ - .ifindex = ifindex, - .configs_lst_head = C_LIST_INIT(ic.configs_lst_head), + .ifindex = ifindex, + .ip_data_list = NULL, }; prepare_one_interface(self, &ic); } From a74a517f49fa3a2b6137334542774a8b53c556fc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 19:43:40 +0100 Subject: [PATCH 15/23] dns/resolved: track pending counter for D-Bus calls in NMDnsSystemdResolved This is used to signal that an update is pending or in progress. For this to work, we also need to implement the stop() handle. Otherwise, we couldn't abort pending requests, which is necessary during shutdown (not today, but in the future). --- src/core/dns/nm-dns-systemd-resolved.c | 161 ++++++++++++++++++------- 1 file changed, 116 insertions(+), 45 deletions(-) diff --git a/src/core/dns/nm-dns-systemd-resolved.c b/src/core/dns/nm-dns-systemd-resolved.c index e3d38ed44a..6eef5f2d4b 100644 --- a/src/core/dns/nm-dns-systemd-resolved.c +++ b/src/core/dns/nm-dns-systemd-resolved.c @@ -50,6 +50,7 @@ typedef struct { GVariant *argument; NMDnsSystemdResolved *self; int ifindex; + int ref_count; } RequestItem; struct _NMDnsSystemdResolvedResolveHandle { @@ -82,8 +83,10 @@ typedef struct { char *dbus_owner; CList handle_lst_head; guint name_owner_changed_id; + guint n_pending; bool send_updates_warn_ratelimited : 1; bool try_start_blocked : 1; + bool stopped : 1; bool dbus_initied : 1; bool send_updates_waiting : 1; /* These two variables ensure that the log is not spammed with @@ -146,10 +149,29 @@ static void _resolve_start(NMDnsSystemdResolved *self, NMDnsSystemdResolvedResol /*****************************************************************************/ -static void -_request_item_free(RequestItem *request_item) +static RequestItem * +_request_item_ref(RequestItem *request_item) { - c_list_unlink_stale(&request_item->request_queue_lst); + nm_assert(request_item); + nm_assert(request_item->ref_count > 0); + nm_assert(request_item->ref_count < G_MAXINT); + nm_assert(!c_list_is_empty(&request_item->request_queue_lst)); + + request_item->ref_count++; + return request_item; +} + +static void +_request_item_unref(RequestItem *request_item) +{ + nm_assert(request_item); + nm_assert(request_item->ref_count > 0); + + if (--request_item->ref_count > 0) + return; + + nm_assert(c_list_is_empty(&request_item->request_queue_lst)); + g_variant_unref(request_item->argument); nm_g_slice_free(request_item); } @@ -165,6 +187,7 @@ _request_item_append(NMDnsSystemdResolved *self, request_item = g_slice_new(RequestItem); *request_item = (RequestItem){ + .ref_count = 1, .operation = operation, .argument = g_variant_ref_sink(argument), .self = self, @@ -191,42 +214,48 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) NMDnsSystemdResolvedPrivate *priv; RequestItem *request_item; NMLogLevel log_level; - - v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), r, &error); - if (nm_utils_error_is_cancelled(error)) - return; + const char *operation; + int ifindex; request_item = user_data; self = request_item->self; - priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + operation = request_item->operation; + ifindex = request_item->ifindex; + _request_item_unref(request_item); + + priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + + v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), r, &error); + if (nm_utils_error_is_cancelled(error)) + goto out_dec_pending; if (v) { - if (request_item->operation == DBUS_OP_SET_LINK_DEFAULT_ROUTE + if (operation == DBUS_OP_SET_LINK_DEFAULT_ROUTE && priv->has_link_default_route == NM_TERNARY_DEFAULT) { priv->has_link_default_route = NM_TERNARY_TRUE; _LOGD("systemd-resolved support for SetLinkDefaultRoute(): API supported"); } - if (request_item->operation == DBUS_OP_SET_LINK_DNS_OVER_TLS + if (operation == DBUS_OP_SET_LINK_DNS_OVER_TLS && priv->has_link_dns_over_tls == NM_TERNARY_DEFAULT) { priv->has_link_dns_over_tls = NM_TERNARY_TRUE; _LOGD("systemd-resolved support for SetLinkDNSOverTLS(): API supported"); } priv->send_updates_warn_ratelimited = FALSE; - return; + goto out_dec_pending; } if (nm_g_error_matches(error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD)) { if (priv->has_link_default_route == NM_TERNARY_DEFAULT - && request_item->operation == DBUS_OP_SET_LINK_DEFAULT_ROUTE) { + && operation == DBUS_OP_SET_LINK_DEFAULT_ROUTE) { priv->has_link_default_route = NM_TERNARY_FALSE; _LOGD("systemd-resolved support for SetLinkDefaultRoute(): API not supported"); } if (priv->has_link_dns_over_tls == NM_TERNARY_DEFAULT - && request_item->operation == DBUS_OP_SET_LINK_DNS_OVER_TLS) { + && operation == DBUS_OP_SET_LINK_DNS_OVER_TLS) { priv->has_link_dns_over_tls = NM_TERNARY_FALSE; _LOGD("systemd-resolved support for SetLinkDNSOverTLS(): API not supported"); } - return; + goto out_dec_pending; } log_level = LOGL_DEBUG; @@ -234,11 +263,17 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) priv->send_updates_warn_ratelimited = TRUE; log_level = LOGL_WARN; } - _NMLOG(log_level, - "send-updates %s@%d failed: %s", - request_item->operation, - request_item->ifindex, - error->message); + _NMLOG(log_level, "send-updates %s@%d failed: %s", operation, ifindex, error->message); + +out_dec_pending: + nm_assert(priv->n_pending > 0); + if (--priv->n_pending <= 0) { + /* We keep @self alive while pending operations are in progress. It's simpler + * to implement. But this requires that we implement "stop()" signal to cancel + * all pending requests. Cancelling is necessary, because during shutdown, + * we must wrap up fast, and not hang an undefined amount time. */ + g_object_unref(self); + } } static gboolean @@ -299,9 +334,12 @@ free_pending_updates(NMDnsSystemdResolved *self) NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); RequestItem *request_item; - while ((request_item = - c_list_first_entry(&priv->request_queue_lst_head, RequestItem, request_queue_lst))) - _request_item_free(request_item); + while ( + (request_item = + c_list_first_entry(&priv->request_queue_lst_head, RequestItem, request_queue_lst))) { + c_list_unlink(&request_item->request_queue_lst); + _request_item_unref(request_item); + } } static gboolean @@ -457,6 +495,9 @@ ensure_resolved_running(NMDnsSystemdResolved *self) { NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + if (priv->stopped) + return NM_TERNARY_FALSE; + if (!priv->dbus_initied) return NM_TERNARY_DEFAULT; @@ -533,6 +574,9 @@ send_updates(NMDnsSystemdResolved *self) request_item->operation, (ss = g_variant_print(request_item->argument, FALSE))); + if (priv->n_pending++ == 0) + g_object_ref(self); + g_dbus_connection_call(priv->dbus_connection, priv->dbus_owner, SYSTEMD_RESOLVED_DBUS_PATH, @@ -544,7 +588,7 @@ send_updates(NMDnsSystemdResolved *self) -1, priv->cancellable, call_done, - request_item); + _request_item_ref(request_item)); } start_resolve: @@ -577,6 +621,8 @@ update(NMDnsPlugin *plugin, gs_unref_array GArray *dirty_array = NULL; guint i; + nm_assert(!priv->stopped); + /* Group configs by ifindex/interfaces. */ interfaces = g_hash_table_new_full(nm_direct_hash, NULL, NULL, (GDestroyNotify) _interface_config_free); @@ -989,6 +1035,48 @@ nm_dns_systemd_resolved_resolve_cancel(NMDnsSystemdResolvedResolveHandle *handle /*****************************************************************************/ +static void +stop(NMDnsPlugin *plugin) +{ + NMDnsSystemdResolved *self = NM_DNS_SYSTEMD_RESOLVED(plugin); + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + NMDnsSystemdResolvedResolveHandle *handle; + + /* This function must be re-entrant!! + * + * Currently there is no concept of unregistering/shutting down. It's not + * clear whether we should de-configure anything in systemd-resolved, we + * don't. + * + * Implementing stop() is important because pending operations take a + * reference on @self. We can only cancel (fast shutdown) the instance + * by cancelling those requests. */ + + priv->stopped = TRUE; + priv->try_start_blocked = TRUE; + + nm_clear_g_cancellable(&priv->cancellable); + + nm_clear_g_free(&priv->dbus_owner); + + while ((handle = c_list_first_entry(&priv->handle_lst_head, + NMDnsSystemdResolvedResolveHandle, + handle_lst))) { + gs_free_error GError *error = NULL; + + nm_utils_error_set_cancelled(&error, TRUE, "NMDnsSystemdResolved"); + _resolve_complete_error(handle, error); + } + + free_pending_updates(self); + + nm_clear_g_dbus_connection_signal(priv->dbus_connection, &priv->name_owner_changed_id); + + nm_clear_g_source_inst(&priv->try_start_timeout_source); +} + +/*****************************************************************************/ + static void nm_dns_systemd_resolved_init(NMDnsSystemdResolved *self) { @@ -1031,33 +1119,15 @@ nm_dns_systemd_resolved_new(void) static void dispose(GObject *object) { - NMDnsSystemdResolved *self = NM_DNS_SYSTEMD_RESOLVED(object); - NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); - NMDnsSystemdResolvedResolveHandle *handle; + NMDnsSystemdResolved *self = NM_DNS_SYSTEMD_RESOLVED(object); + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); - while ((handle = c_list_first_entry(&priv->handle_lst_head, - NMDnsSystemdResolvedResolveHandle, - handle_lst))) { - gs_free_error GError *error = NULL; - - nm_utils_error_set_cancelled(&error, TRUE, "NMDnsSystemdResolved"); - _resolve_complete_error(handle, error); - } - - free_pending_updates(self); - - nm_clear_g_dbus_connection_signal(priv->dbus_connection, &priv->name_owner_changed_id); - - nm_clear_g_cancellable(&priv->cancellable); - - nm_clear_g_source_inst(&priv->try_start_timeout_source); + stop(NM_DNS_PLUGIN(self)); g_clear_object(&priv->dbus_connection); - nm_clear_pointer(&priv->dirty_interfaces, g_hash_table_unref); + nm_clear_pointer(&priv->dirty_interfaces, g_hash_table_destroy); G_OBJECT_CLASS(nm_dns_systemd_resolved_parent_class)->dispose(object); - - nm_clear_g_free(&priv->dbus_owner); } static void @@ -1070,5 +1140,6 @@ nm_dns_systemd_resolved_class_init(NMDnsSystemdResolvedClass *dns_class) plugin_class->plugin_name = "systemd-resolved"; plugin_class->is_caching = TRUE; + plugin_class->stop = stop; plugin_class->update = update; } From bbbb1b733946ed9e16a9153bf6f541dbbc8d9ff9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 19:59:58 +0100 Subject: [PATCH 16/23] dns/resolved: implement update-pending flag in NMDnsSystemdResolved plugin --- src/core/dns/nm-dns-systemd-resolved.c | 83 ++++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 6 deletions(-) diff --git a/src/core/dns/nm-dns-systemd-resolved.c b/src/core/dns/nm-dns-systemd-resolved.c index 6eef5f2d4b..ac6fe5dedb 100644 --- a/src/core/dns/nm-dns-systemd-resolved.c +++ b/src/core/dns/nm-dns-systemd-resolved.c @@ -89,6 +89,7 @@ typedef struct { bool stopped : 1; bool dbus_initied : 1; bool send_updates_waiting : 1; + bool update_pending : 1; /* These two variables ensure that the log is not spammed with * API (not) supported messages. * They can be removed when no distro uses systemd-resolved < v240 anymore @@ -109,7 +110,7 @@ struct _NMDnsSystemdResolvedClass { G_DEFINE_TYPE(NMDnsSystemdResolved, nm_dns_systemd_resolved, NM_TYPE_DNS_PLUGIN) #define NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self) \ - _NM_GET_PRIVATE(self, NMDnsSystemdResolved, NM_IS_DNS_SYSTEMD_RESOLVED) + _NM_GET_PRIVATE(self, NMDnsSystemdResolved, NM_IS_DNS_SYSTEMD_RESOLVED, NMDnsPlugin) /*****************************************************************************/ @@ -149,6 +150,65 @@ static void _resolve_start(NMDnsSystemdResolved *self, NMDnsSystemdResolvedResol /*****************************************************************************/ +static gboolean +_update_pending_detect(NMDnsSystemdResolved *self) +{ + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + + if (priv->n_pending > 0) { + /* we have pending calls. We definitely want to wait for them to complete. */ + return TRUE; + } + if (!priv->dbus_initied) { + if (!priv->dbus_connection) + return FALSE; + /* D-Bus not yet initialized (and we don't know the name owner yet). Pending. */ + return TRUE; + } + if (priv->try_start_timeout_source) { + /* We are waiting to D-Bus activate resolved. Pending. */ + return TRUE; + } + if (priv->try_start_blocked) { + /* We earlier tried to start resolved, but are rate limited. We are not pending an update + * (that we expect to complete any time soon). */ + return FALSE; + } + if (priv->send_updates_waiting) { + /* we wait to send updates. We are pending. */ + return TRUE; + } + return FALSE; +} + +static void +_update_pending_maybe_changed(NMDnsSystemdResolved *self) +{ + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + gboolean update_pending; + + /* Important: we need to make sure that we call _update_pending_maybe_changed(), when + * the state changes. */ + + update_pending = _update_pending_detect(self); + if (priv->update_pending != update_pending) { + priv->update_pending = update_pending; + _nm_dns_plugin_update_pending_maybe_changed(NM_DNS_PLUGIN(self)); + } +} + +static gboolean +get_update_pending(NMDnsPlugin *plugin) +{ + NMDnsSystemdResolved *self = NM_DNS_SYSTEMD_RESOLVED(plugin); + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + + nm_assert(priv->update_pending == _update_pending_detect(self)); + return priv->update_pending; +} + +/*****************************************************************************/ + static RequestItem * _request_item_ref(RequestItem *request_item) { @@ -268,6 +328,7 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) out_dec_pending: nm_assert(priv->n_pending > 0); if (--priv->n_pending <= 0) { + _update_pending_maybe_changed(self); /* We keep @self alive while pending operations are in progress. It's simpler * to implement. But this requires that we implement "stop()" signal to cancel * all pending requests. Cancelling is necessary, because during shutdown, @@ -487,6 +548,7 @@ again: goto again; } + _update_pending_maybe_changed(self); return G_SOURCE_CONTINUE; } @@ -520,6 +582,7 @@ ensure_resolved_running(NMDnsSystemdResolved *self) NULL, NULL, NULL); + _update_pending_maybe_changed(self); return NM_TERNARY_DEFAULT; } @@ -574,8 +637,11 @@ send_updates(NMDnsSystemdResolved *self) request_item->operation, (ss = g_variant_print(request_item->argument, FALSE))); - if (priv->n_pending++ == 0) + if (priv->n_pending++ == 0) { + /* We are inside send_updates(). All callers are already calling + * _update_pending_maybe_changed() afterwards. */ g_object_ref(self); + } g_dbus_connection_call(priv->dbus_connection, priv->dbus_owner, @@ -698,6 +764,7 @@ update(NMDnsPlugin *plugin, priv->send_updates_waiting = TRUE; send_updates(self); + _update_pending_maybe_changed(self); return TRUE; } @@ -728,6 +795,7 @@ name_owner_changed(NMDnsSystemdResolved *self, const char *owner) } send_updates(self); + _update_pending_maybe_changed(self); } static void @@ -1095,6 +1163,8 @@ nm_dns_systemd_resolved_init(NMDnsSystemdResolved *self) return; } + priv->update_pending = TRUE; + priv->name_owner_changed_id = nm_dbus_connection_signal_subscribe_name_owner_changed(priv->dbus_connection, SYSTEMD_RESOLVED_DBUS_SERVICE, @@ -1138,8 +1208,9 @@ nm_dns_systemd_resolved_class_init(NMDnsSystemdResolvedClass *dns_class) object_class->dispose = dispose; - plugin_class->plugin_name = "systemd-resolved"; - plugin_class->is_caching = TRUE; - plugin_class->stop = stop; - plugin_class->update = update; + plugin_class->plugin_name = "systemd-resolved"; + plugin_class->is_caching = TRUE; + plugin_class->stop = stop; + plugin_class->update = update; + plugin_class->get_update_pending = get_update_pending; } From 5da17c689be5e66ea2f63dea6f1846625e652998 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Apr 2022 11:59:56 +0200 Subject: [PATCH 17/23] dns/unbound: drop deprecated "unbound" DNS plugin The "unbound" DNS plugin was very rudimentary and is deprecated since commit 4a2fe09853cd ('man: mark [main].dns=unbound as deprecated') (Jun 2021). It is part of dnssec-trigger tool, but the dnssec-trigger tool doesn't actually use it. Instead it installs a dispatcher script "/usr/lib/NetworkManager/dispatcher.d/01-dnssec-trigger". Especially, since the plugin requires "/usr/libexec/dnssec-trigger-script", which is provided by "dnssec-trigger" package on Fedora. At the same time, the package provides the dispatcher script. So I don't this works or anybody is using this. https://mail.gnome.org/archives/networkmanager-list/2022-April/msg00002.html --- Makefile.am | 2 - config.h.meson | 3 -- configure.ac | 12 ----- man/NetworkManager.conf.xml | 11 +---- meson.build | 13 ++---- meson_options.txt | 1 - src/core/dns/nm-dns-manager.c | 15 +++---- src/core/dns/nm-dns-unbound.c | 84 ----------------------------------- src/core/dns/nm-dns-unbound.h | 27 ----------- src/core/meson.build | 1 - 10 files changed, 11 insertions(+), 158 deletions(-) delete mode 100644 src/core/dns/nm-dns-unbound.c delete mode 100644 src/core/dns/nm-dns-unbound.h diff --git a/Makefile.am b/Makefile.am index 91eaf67aec..72f224fc20 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2530,8 +2530,6 @@ src_core_libNetworkManager_la_SOURCES = \ src/core/dns/nm-dns-dnsmasq.h \ src/core/dns/nm-dns-systemd-resolved.c \ src/core/dns/nm-dns-systemd-resolved.h \ - src/core/dns/nm-dns-unbound.c \ - src/core/dns/nm-dns-unbound.h \ \ src/core/dnsmasq/nm-dnsmasq-manager.c \ src/core/dnsmasq/nm-dnsmasq-manager.h \ diff --git a/config.h.meson b/config.h.meson index 7d1feb53ad..7337165082 100644 --- a/config.h.meson +++ b/config.h.meson @@ -13,9 +13,6 @@ /* Define to path of dnsmasq binary */ #mesondefine DNSMASQ_PATH -/* Define to path of unbound dnssec-trigger-script */ -#mesondefine DNSSEC_TRIGGER_PATH - /* Gettext package */ #mesondefine GETTEXT_PACKAGE diff --git a/configure.ac b/configure.ac index 8ed50706b9..24107f163b 100644 --- a/configure.ac +++ b/configure.ac @@ -1006,18 +1006,6 @@ fi AC_DEFINE_UNQUOTED(DNSMASQ_PATH, "$DNSMASQ_PATH", [Define to path of dnsmasq binary]) AC_SUBST(DNSMASQ_PATH) -# dnssec-trigger-script path -AC_ARG_WITH(dnssec_trigger, - AS_HELP_STRING([--with-dnssec-trigger=/path/to/dnssec-trigger-script], [path to unbound dnssec-trigger-script])) -if test "x${with_dnssec_trigger}" = x; then - AC_PATH_PROG(DNSSEC_TRIGGER_PATH, dnssec-trigger-script, /usr/libexec/dnssec-trigger-script, - /usr/local/libexec:/usr/local/lib:/usr/local/lib/dnssec-trigger:/usr/libexec:/usr/lib:/usr/lib/dnssec-trigger) -else - DNSSEC_TRIGGER_PATH="$with_dnssec_trigger" -fi -AC_DEFINE_UNQUOTED(DNSSEC_TRIGGER_PATH, "$DNSSEC_TRIGGER_PATH", [Define to path of unbound dnssec-trigger-script]) -AC_SUBST(DNSSEC_TRIGGER_PATH) - # system CA certificates path AC_ARG_WITH(system-ca-path, AS_HELP_STRING([--with-system-ca-path=/path/to/ssl/certs], [path to system CA certificates])) diff --git a/man/NetworkManager.conf.xml b/man/NetworkManager.conf.xml index cb6b40afa0..18b25d9370 100644 --- a/man/NetworkManager.conf.xml +++ b/man/NetworkManager.conf.xml @@ -345,19 +345,12 @@ no-auto-default=* systemd-resolved: NetworkManager will push the DNS configuration to systemd-resolved - unbound: NetworkManager will talk - to unbound and dnssec-triggerd, using "Conditional Forwarding" - with DNSSEC support. /etc/resolv.conf - will be managed by dnssec-trigger daemon. This option is - deprecated. Note that dnssec-trigger ships a NetworkManager dispatcher - script so this DNS plugin is not necessary. - none: NetworkManager will not modify resolv.conf. This implies rc-manager unmanaged - Note that the plugins dnsmasq, systemd-resolved - and unbound are caching local nameservers. + Note that the plugins dnsmasq and systemd-resolved + are caching local nameservers. Hence, when NetworkManager writes &nmrundir;/resolv.conf and /etc/resolv.conf (according to rc-manager setting below), the name server there will be localhost only. diff --git a/meson.build b/meson.build index 45d6970894..edf4b377fa 100644 --- a/meson.build +++ b/meson.build @@ -683,18 +683,11 @@ endforeach # external misc tools paths default_paths = ['/sbin', '/usr/sbin'] -dnssec_ts_paths = ['/usr/local/libexec', - '/usr/local/lib', - '/usr/local/lib/dnssec-trigger', - '/usr/libexec', - '/usr/lib', - '/usr/lib/dnssec-trigger'] # 0: cmdline option, 1: paths, 2: fallback -progs = [['iptables', default_paths, '/usr/sbin/iptables'], - ['nft', default_paths, '/usr/sbin/nft'], - ['dnsmasq', default_paths, ''], - ['dnssec_trigger', dnssec_ts_paths, join_paths(nm_libexecdir, 'dnssec-trigger-script') ], +progs = [['iptables', default_paths, '/usr/sbin/iptables'], + ['nft', default_paths, '/usr/sbin/nft'], + ['dnsmasq', default_paths, ''], ] foreach prog : progs diff --git a/meson_options.txt b/meson_options.txt index 42f84711d0..cec0664186 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -7,7 +7,6 @@ option('kernel_firmware_dir', type: 'string', value: '/lib/firmware', descriptio option('iptables', type: 'string', value: '', description: 'path to iptables') option('nft', type: 'string', value: '', description: 'path to nft') option('dnsmasq', type: 'string', value: '', description: 'path to dnsmasq') -option('dnssec_trigger', type: 'string', value: '', description: 'path to unbound dnssec-trigger-script') # platform option('dist_version', type: 'string', value: '', description: 'Define the NM\'s distribution version string') diff --git a/src/core/dns/nm-dns-manager.c b/src/core/dns/nm-dns-manager.c index 03f3eceddf..0d6ade2b2d 100644 --- a/src/core/dns/nm-dns-manager.c +++ b/src/core/dns/nm-dns-manager.c @@ -35,7 +35,6 @@ #include "nm-dns-dnsmasq.h" #include "nm-dns-plugin.h" #include "nm-dns-systemd-resolved.h" -#include "nm-dns-unbound.h" #include "nm-ip-config.h" #include "nm-l3-config-data.h" #include "nm-manager.h" @@ -2379,16 +2378,14 @@ again: priv->plugin = nm_dns_dnsmasq_new(); plugin_changed = TRUE; } - } else if (nm_streq0(mode, "unbound")) { - if (force_reload_plugin || !NM_IS_DNS_UNBOUND(priv->plugin)) { - _clear_plugin(self); - priv->plugin = nm_dns_unbound_new(); - plugin_changed = TRUE; - } } else { if (!NM_IN_STRSET(mode, "none", "default")) { - if (mode) - _LOGW("init: unknown dns mode '%s'", mode); + if (mode) { + if (nm_streq(mode, "unbound")) + _LOGW("init: ns mode 'unbound' was removed. Update your configuration"); + else + _LOGW("init: unknown dns mode '%s'", mode); + } mode = "default"; } if (_clear_plugin(self)) diff --git a/src/core/dns/nm-dns-unbound.c b/src/core/dns/nm-dns-unbound.c deleted file mode 100644 index 8a75cf08f0..0000000000 --- a/src/core/dns/nm-dns-unbound.c +++ /dev/null @@ -1,84 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-or-later */ -/* - * Copyright (C) 2014 Red Hat, Inc. - * Author: Pavel Šimerda - */ - -#include "src/core/nm-default-daemon.h" - -#include "nm-dns-unbound.h" - -#include "NetworkManagerUtils.h" - -/*****************************************************************************/ - -struct _NMDnsUnbound { - NMDnsPlugin parent; -}; - -struct _NMDnsUnboundClass { - NMDnsPluginClass parent; -}; - -G_DEFINE_TYPE(NMDnsUnbound, nm_dns_unbound, NM_TYPE_DNS_PLUGIN) - -/*****************************************************************************/ - -static gboolean -update(NMDnsPlugin *plugin, - const NMGlobalDnsConfig *global_config, - const CList *ip_config_lst_head, - const char *hostdomain, - GError **error) -{ - char *argv[] = {DNSSEC_TRIGGER_PATH, "--async", "--update", NULL}; - gs_free_error GError *local = NULL; - int status; - - /* TODO: We currently call a script installed with the dnssec-trigger - * package that queries all information itself. Later, the dependency - * on that package will be optional and the only hard dependency will - * be unbound. - * - * Unbound configuration should be later handled by this plugin directly, - * without calling custom scripts. The dnssec-trigger functionality - * may be eventually merged into NetworkManager. - */ - if (!g_spawn_sync("/", argv, NULL, 0, NULL, NULL, NULL, NULL, &status, &local)) { - nm_utils_error_set(error, - NM_UTILS_ERROR_UNKNOWN, - "error spawning dns-trigger: %s", - local->message); - return FALSE; - } - if (status != 0) { - nm_utils_error_set(error, - NM_UTILS_ERROR_UNKNOWN, - "dns-trigger exited with error code %d", - status); - return FALSE; - } - return TRUE; -} - -/*****************************************************************************/ - -static void -nm_dns_unbound_init(NMDnsUnbound *unbound) -{} - -NMDnsPlugin * -nm_dns_unbound_new(void) -{ - return g_object_new(NM_TYPE_DNS_UNBOUND, NULL); -} - -static void -nm_dns_unbound_class_init(NMDnsUnboundClass *klass) -{ - NMDnsPluginClass *plugin_class = NM_DNS_PLUGIN_CLASS(klass); - - plugin_class->plugin_name = "unbound"; - plugin_class->is_caching = TRUE; - plugin_class->update = update; -} diff --git a/src/core/dns/nm-dns-unbound.h b/src/core/dns/nm-dns-unbound.h deleted file mode 100644 index feb3309913..0000000000 --- a/src/core/dns/nm-dns-unbound.h +++ /dev/null @@ -1,27 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-or-later */ -/* - * Copyright (C) 2014 Red Hat, Inc. - */ - -#ifndef __NETWORKMANAGER_DNS_UNBOUND_H__ -#define __NETWORKMANAGER_DNS_UNBOUND_H__ - -#include "nm-dns-plugin.h" - -#define NM_TYPE_DNS_UNBOUND (nm_dns_unbound_get_type()) -#define NM_DNS_UNBOUND(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), NM_TYPE_DNS_UNBOUND, NMDnsUnbound)) -#define NM_DNS_UNBOUND_CLASS(klass) \ - (G_TYPE_CHECK_CLASS_CAST((klass), NM_TYPE_DNS_UNBOUND, NMDnsUnboundClass)) -#define NM_IS_DNS_UNBOUND(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj), NM_TYPE_DNS_UNBOUND)) -#define NM_IS_DNS_UNBOUND_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass), NM_TYPE_DNS_UNBOUND)) -#define NM_DNS_UNBOUND_GET_CLASS(obj) \ - (G_TYPE_INSTANCE_GET_CLASS((obj), NM_TYPE_DNS_UNBOUND, NMDnsUnboundClass)) - -typedef struct _NMDnsUnbound NMDnsUnbound; -typedef struct _NMDnsUnboundClass NMDnsUnboundClass; - -GType nm_dns_unbound_get_type(void); - -NMDnsPlugin *nm_dns_unbound_new(void); - -#endif /* __NETWORKMANAGER_DNS_UNBOUND_H__ */ diff --git a/src/core/meson.build b/src/core/meson.build index 2148d23b76..f3359ad0f5 100644 --- a/src/core/meson.build +++ b/src/core/meson.build @@ -123,7 +123,6 @@ libNetworkManager = static_library( 'dns/nm-dns-manager.c', 'dns/nm-dns-plugin.c', 'dns/nm-dns-systemd-resolved.c', - 'dns/nm-dns-unbound.c', 'dnsmasq/nm-dnsmasq-manager.c', 'dnsmasq/nm-dnsmasq-utils.c', 'ppp/nm-ppp-manager-call.c', From ccf0e8d327190216d3c43fd376f7c155cbe747ac Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 8 Apr 2022 13:09:48 +0200 Subject: [PATCH 18/23] dns/dnsmasq: use GSource for timeout in NMDnsDnsmasq --- src/core/dns/nm-dns-dnsmasq.c | 36 +++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/core/dns/nm-dns-dnsmasq.c b/src/core/dns/nm-dns-dnsmasq.c index 43426882ed..7a6a2591f8 100644 --- a/src/core/dns/nm-dns-dnsmasq.c +++ b/src/core/dns/nm-dns-dnsmasq.c @@ -678,14 +678,14 @@ typedef struct { char *name_owner; + GSource *main_timeout_source; + GSource *burst_retry_timeout_source; + gint64 burst_start_at; GPid process_pid; guint name_owner_changed_id; - guint main_timeout_id; - - guint burst_retry_timeout_id; guint8 burst_count; @@ -928,14 +928,14 @@ _main_cleanup(NMDnsDnsmasq *self, gboolean emit_failed) nm_clear_g_dbus_connection_signal(priv->dbus_connection, &priv->name_owner_changed_id); - nm_clear_g_source(&priv->main_timeout_id); + nm_clear_g_source_inst(&priv->main_timeout_source); nm_clear_g_cancellable(&priv->update_cancellable); /* cancelling the main_cancellable will also cause _gl_pid_spawn*() to terminate the * process in the background. */ nm_clear_g_cancellable(&priv->main_cancellable); - if (!priv->is_stopped && priv->burst_retry_timeout_id == 0) { + if (!priv->is_stopped && !priv->burst_retry_timeout_source) { start_dnsmasq(self, FALSE, NULL); send_dnsmasq_update(self); } @@ -961,7 +961,7 @@ name_owner_changed(NMDnsDnsmasq *self, const char *name_owner) } _LOGT("D-Bus name for dnsmasq got owner %s", name_owner); - nm_clear_g_source(&priv->main_timeout_id); + nm_clear_g_source_inst(&priv->main_timeout_source); send_dnsmasq_update(self); } @@ -1047,11 +1047,11 @@ _burst_retry_timeout_cb(gpointer user_data) NMDnsDnsmasq *self = user_data; NMDnsDnsmasqPrivate *priv = NM_DNS_DNSMASQ_GET_PRIVATE(self); - priv->burst_retry_timeout_id = 0; + nm_clear_g_source_inst(&priv->burst_retry_timeout_source); start_dnsmasq(self, TRUE, NULL); send_dnsmasq_update(self); - return G_SOURCE_REMOVE; + return G_SOURCE_CONTINUE; } static gboolean @@ -1090,29 +1090,29 @@ start_dnsmasq(NMDnsDnsmasq *self, gboolean force_start, GError **error) || priv->burst_start_at + RATELIMIT_INTERVAL_MSEC <= now) { priv->burst_start_at = now; priv->burst_count = 1; - nm_clear_g_source(&priv->burst_retry_timeout_id); + nm_clear_g_source_inst(&priv->burst_retry_timeout_source); _LOGT("rate-limit: start burst interval of %d seconds %s", RATELIMIT_INTERVAL_MSEC / 1000, force_start ? " (force)" : ""); } else if (priv->burst_count < RATELIMIT_BURST) { - nm_assert(priv->burst_retry_timeout_id == 0); + nm_assert(!priv->burst_retry_timeout_source); priv->burst_count++; _LOGT("rate-limit: %u try within burst interval of %d seconds", (guint) priv->burst_count, RATELIMIT_INTERVAL_MSEC / 1000); } else { - if (priv->burst_retry_timeout_id == 0) { + if (!priv->burst_retry_timeout_source) { _LOGW("dnsmasq dies and gets respawned too quickly. Back off. Something is very wrong"); - priv->burst_retry_timeout_id = - g_timeout_add_seconds((2 * RATELIMIT_INTERVAL_MSEC) / 1000, - _burst_retry_timeout_cb, - self); + priv->burst_retry_timeout_source = + nm_g_timeout_add_seconds_source((2 * RATELIMIT_INTERVAL_MSEC) / 1000, + _burst_retry_timeout_cb, + self); } else _LOGT("rate-limit: currently rate-limited from restart"); return TRUE; } - priv->main_timeout_id = g_timeout_add(10000, spawn_timeout_cb, self); + priv->main_timeout_source = nm_g_timeout_add_source(10000, spawn_timeout_cb, self); priv->main_cancellable = g_cancellable_new(); @@ -1151,7 +1151,7 @@ stop(NMDnsPlugin *plugin) priv->is_stopped = TRUE; priv->burst_start_at = 0; - nm_clear_g_source(&priv->burst_retry_timeout_id); + nm_clear_g_source_inst(&priv->burst_retry_timeout_source); /* Cancelling the cancellable will also terminate the * process (in the background). */ @@ -1178,7 +1178,7 @@ dispose(GObject *object) priv->is_stopped = TRUE; - nm_clear_g_source(&priv->burst_retry_timeout_id); + nm_clear_g_source_inst(&priv->burst_retry_timeout_source); _main_cleanup(self, FALSE); From f2abcf20827deaaaf54082dbc74657ab19666dcd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 8 Apr 2022 13:23:08 +0200 Subject: [PATCH 19/23] dns/dnsmasq: implement update-pending flag in NMDnsDnsmasq plugin We want to know when we are busy (have an update pending or on-going). Implement that. --- src/core/dns/nm-dns-dnsmasq.c | 87 +++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 5 deletions(-) diff --git a/src/core/dns/nm-dns-dnsmasq.c b/src/core/dns/nm-dns-dnsmasq.c index 7a6a2591f8..7d0f0490e0 100644 --- a/src/core/dns/nm-dns-dnsmasq.c +++ b/src/core/dns/nm-dns-dnsmasq.c @@ -691,6 +691,10 @@ typedef struct { bool is_stopped : 1; + bool set_server_ex_args_dirty : 1; + + bool update_pending : 1; + } NMDnsDnsmasqPrivate; struct _NMDnsDnsmasq { @@ -704,7 +708,8 @@ struct _NMDnsDnsmasqClass { G_DEFINE_TYPE(NMDnsDnsmasq, nm_dns_dnsmasq, NM_TYPE_DNS_PLUGIN) -#define NM_DNS_DNSMASQ_GET_PRIVATE(self) _NM_GET_PRIVATE(self, NMDnsDnsmasq, NM_IS_DNS_DNSMASQ) +#define NM_DNS_DNSMASQ_GET_PRIVATE(self) \ + _NM_GET_PRIVATE(self, NMDnsDnsmasq, NM_IS_DNS_DNSMASQ, NMDnsPlugin) /*****************************************************************************/ @@ -717,6 +722,55 @@ static gboolean start_dnsmasq(NMDnsDnsmasq *self, gboolean force_start, GError * /*****************************************************************************/ +static gboolean +_update_pending_detect(NMDnsDnsmasq *self) +{ + NMDnsDnsmasqPrivate *priv = NM_DNS_DNSMASQ_GET_PRIVATE(self); + + if (priv->is_stopped) + return FALSE; + if (priv->main_timeout_source) { + /* we are waiting for dnsmasq to start. */ + return TRUE; + } + if (priv->update_cancellable) { + /* An update is in progress. Busy. */ + return TRUE; + } + if (priv->set_server_ex_args_dirty) { + /* the args just changed and were not yet sent. Busy. */ + return TRUE; + } + + return FALSE; +} + +static void +_update_pending_maybe_changed(NMDnsDnsmasq *self) +{ + NMDnsDnsmasqPrivate *priv = NM_DNS_DNSMASQ_GET_PRIVATE(self); + gboolean update_pending; + + update_pending = _update_pending_detect(self); + if (priv->update_pending == update_pending) + return; + + priv->update_pending = update_pending; + _nm_dns_plugin_update_pending_maybe_changed(NM_DNS_PLUGIN(self)); +} + +static gboolean +get_update_pending(NMDnsPlugin *plugin) +{ + NMDnsDnsmasq *self = NM_DNS_DNSMASQ(plugin); + NMDnsDnsmasqPrivate *priv = NM_DNS_DNSMASQ_GET_PRIVATE(self); + + nm_assert(priv->update_pending == _update_pending_detect(self)); + return priv->update_pending; +} + +/*****************************************************************************/ + static void add_dnsmasq_nameserver(NMDnsDnsmasq *self, GVariantBuilder *servers, @@ -871,6 +925,7 @@ static void dnsmasq_update_done(GObject *source_object, GAsyncResult *res, gpointer user_data) { NMDnsDnsmasq *self; + NMDnsDnsmasqPrivate *priv; gs_free_error GError *error = NULL; gs_unref_variant GVariant *response = NULL; @@ -880,10 +935,16 @@ dnsmasq_update_done(GObject *source_object, GAsyncResult *res, gpointer user_dat return; self = user_data; + priv = NM_DNS_DNSMASQ_GET_PRIVATE(self); + + nm_clear_g_cancellable(&priv->update_cancellable); + if (!response) _LOGW("dnsmasq update failed: %s", error->message); else _LOGD("dnsmasq update successful"); + + _update_pending_maybe_changed(self); } static void @@ -899,6 +960,8 @@ send_dnsmasq_update(NMDnsDnsmasq *self) nm_clear_g_cancellable(&priv->update_cancellable); priv->update_cancellable = g_cancellable_new(); + priv->set_server_ex_args_dirty = FALSE; + g_dbus_connection_call(priv->dbus_connection, priv->name_owner, DNSMASQ_DBUS_PATH, @@ -911,6 +974,8 @@ send_dnsmasq_update(NMDnsDnsmasq *self) priv->update_cancellable, dnsmasq_update_done, self); + + _update_pending_maybe_changed(self); } /*****************************************************************************/ @@ -939,6 +1004,8 @@ _main_cleanup(NMDnsDnsmasq *self, gboolean emit_failed) start_dnsmasq(self, FALSE, NULL); send_dnsmasq_update(self); } + + _update_pending_maybe_changed(self); } static void @@ -963,6 +1030,8 @@ name_owner_changed(NMDnsDnsmasq *self, const char *name_owner) _LOGT("D-Bus name for dnsmasq got owner %s", name_owner); nm_clear_g_source_inst(&priv->main_timeout_source); send_dnsmasq_update(self); + + _update_pending_maybe_changed(self); } static void @@ -1117,6 +1186,8 @@ start_dnsmasq(NMDnsDnsmasq *self, gboolean force_start, GError **error) priv->main_cancellable = g_cancellable_new(); _gl_pid_spawn(dm_binary, priv->main_cancellable, spawn_notify, self); + + _update_pending_maybe_changed(self); return TRUE; } @@ -1136,8 +1207,11 @@ update(NMDnsPlugin *plugin, nm_clear_pointer(&priv->set_server_ex_args, g_variant_unref); priv->set_server_ex_args = g_variant_ref_sink(create_update_args(self, global_config, ip_data_lst_head, hostdomain)); + priv->set_server_ex_args_dirty = TRUE; send_dnsmasq_update(self); + + _update_pending_maybe_changed(self); return TRUE; } @@ -1156,6 +1230,8 @@ stop(NMDnsPlugin *plugin) /* Cancelling the cancellable will also terminate the * process (in the background). */ _main_cleanup(self, FALSE); + + _update_pending_maybe_changed(self); } /*****************************************************************************/ @@ -1197,8 +1273,9 @@ nm_dns_dnsmasq_class_init(NMDnsDnsmasqClass *dns_class) object_class->dispose = dispose; - plugin_class->plugin_name = "dnsmasq"; - plugin_class->is_caching = TRUE; - plugin_class->stop = stop; - plugin_class->update = update; + plugin_class->plugin_name = "dnsmasq"; + plugin_class->is_caching = TRUE; + plugin_class->stop = stop; + plugin_class->update = update; + plugin_class->get_update_pending = get_update_pending; } From cef5b8dd46126f19c208f2469c6b8b6e4aa04030 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 Apr 2022 13:09:05 +0200 Subject: [PATCH 20/23] dns: prevent update-pending to hang indefinitely For example, if you have a dnsmasq service running and bound to port 53, then NetworkManager's [main].dns=dnsmasq will fail to start. And we keep retrying to start it. But then update pending would hang indefinitely, and devices could not become active. That must not happen. Give the DNS update only 5 seconds. If it's not done by then, assume we have a problem and unblock. --- src/core/dns/nm-dns-manager.c | 43 ++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/core/dns/nm-dns-manager.c b/src/core/dns/nm-dns-manager.c index 0d6ade2b2d..afda300bd2 100644 --- a/src/core/dns/nm-dns-manager.c +++ b/src/core/dns/nm-dns-manager.c @@ -56,6 +56,8 @@ #define HAS_NETCONFIG 1 #endif +#define UPDATE_PENDING_UNBLOCK_TIMEOUT_MSEC 5000 + /*****************************************************************************/ typedef enum { SR_SUCCESS, SR_NOTFOUND, SR_ERROR } SpawnResult; @@ -92,6 +94,11 @@ typedef struct { CList ip_data_lst_head; GVariant *config_variant; + /* A DNS plugin should not be marked as pending indefinitely. + * We are only blocked if "update_pending" is TRUE and we have + * "update_pending_unblock" timer ticking. */ + GSource *update_pending_unblock; + bool ip_data_lst_need_sort : 1; bool configs_lst_need_sort : 1; @@ -222,6 +229,25 @@ _update_pending_detect(NMDnsManager *self) return FALSE; } +static gboolean +_update_pending_unblock_cb(gpointer user_data) +{ + NMDnsManager *self = user_data; + NMDnsManagerPrivate *priv = NM_DNS_MANAGER_GET_PRIVATE(self); + + nm_assert(priv->update_pending); + nm_assert(priv->update_pending_unblock); + nm_assert(_update_pending_detect(self)); + + nm_clear_g_source_inst(&priv->update_pending_unblock); + + _LOGW( + "update-pending changed: DNS plugin did not become ready again. Assume something is wrong"); + + _notify(self, PROP_UPDATE_PENDING); + return G_SOURCE_CONTINUE; +} + static void _update_pending_maybe_changed(NMDnsManager *self) { @@ -232,6 +258,14 @@ _update_pending_maybe_changed(NMDnsManager *self) if (priv->update_pending == update_pending) return; + if (update_pending) { + nm_assert(!priv->update_pending_unblock); + priv->update_pending_unblock = nm_g_timeout_add_source(UPDATE_PENDING_UNBLOCK_TIMEOUT_MSEC, + _update_pending_unblock_cb, + self); + } else + nm_clear_g_source_inst(&priv->update_pending_unblock); + priv->update_pending = update_pending; _LOGD("update-pending changed: %spending", update_pending ? "" : "not "); _notify(self, PROP_UPDATE_PENDING); @@ -252,7 +286,12 @@ nm_dns_manager_get_update_pending(NMDnsManager *self) priv = NM_DNS_MANAGER_GET_PRIVATE(self); nm_assert(priv->update_pending == _update_pending_detect(self)); - return priv->update_pending; + nm_assert(priv->update_pending || !priv->update_pending_unblock); + + /* update-pending can only be TRUE for a certain time (before we assume + * something is really wrong with the plugin). That is, as long as + * update_pending_unblock is ticking. */ + return !!priv->update_pending_unblock; } /*****************************************************************************/ @@ -2727,6 +2766,8 @@ dispose(GObject *object) _clear_sd_resolved_plugin(self); _clear_plugin(self); + nm_clear_g_source_inst(&priv->update_pending_unblock); + c_list_for_each_entry_safe (ip_data, ip_data_safe, &priv->ip_data_lst_head, ip_data_lst) _dns_config_ip_data_free(ip_data); From 6e35cf4a7d709385b7c8e38ab41f8e62873a34eb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 8 Apr 2022 11:40:42 +0200 Subject: [PATCH 21/23] core: add nm_manager_get_dns_manager() getter nm_dns_manager_get() is already a singleton. So users usually can just get it whenever they need -- except during shutdown after the singleton was destroyed. This is usually fine, because users really should not try to get it late during shutdown. However, if you subscribe a signal handler on the singleton, then you will also eventually want to unsubscribe it. While the moment when you subscribe it is clearly not during late-shutdown, it's not clear how to ensure that the signal listener gets destroyed before the DNS manager singleton. So usually, whenever you are going to subscribe a signal, you need to make sure that the target object stays alive long enough. Which may mean to keep a reference to it. Next, we will have NMDevice subscribe to the singleton. With above said, that would mean that potentially every NMDevice needs to keep a reference to the NMDnsManager. That is not best. Also, later NMManager will face the same problem, because it will also subscribe to NMDnsManager. So, instead let NMManager own a reference to the NMDnsManager. This ensures the lifetimes are properly guarded (NMDevice also references NMManager already). Also, access nm_dns_manager_get() lazy on first use, to only initialize it when needed the first time (which might be quite late). --- src/core/nm-manager.c | 27 +++++++++++++++++++++++++++ src/core/nm-manager.h | 4 ++++ 2 files changed, 31 insertions(+) diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index 521536979f..fab316f4d4 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -20,6 +20,7 @@ #include "devices/nm-device-factory.h" #include "devices/nm-device-generic.h" #include "devices/nm-device.h" +#include "dns/nm-dns-manager.h" #include "dhcp/nm-dhcp-manager.h" #include "libnm-core-aux-intern/nm-common-macros.h" #include "libnm-core-intern/nm-core-internal.h" @@ -144,6 +145,8 @@ NM_GOBJECT_PROPERTIES_DEFINE(NMManager, typedef struct { NMPlatform *platform; + NMDnsManager *dns_mgr; + GArray *capabilities; CList active_connections_lst_head; /* Oldest ACs at the beginning */ @@ -7790,6 +7793,28 @@ impl_manager_checkpoint_adjust_rollback_timeout(NMDBusObject /*****************************************************************************/ +NMDnsManager * +nm_manager_get_dns_manager(NMManager *self) +{ + NMManagerPrivate *priv; + + g_return_val_if_fail(NM_IS_MANAGER(self), NULL); + + priv = NM_MANAGER_GET_PRIVATE(self); + + if (G_UNLIKELY(!priv->dns_mgr)) { + /* Initialize lazily on first use. + * + * But keep a reference. This is to ensure proper lifetimes between + * singleton instances (i.e. nm_dns_manager_get() outlives NMManager). */ + priv->dns_mgr = g_object_ref(nm_dns_manager_get()); + } + + return priv->dns_mgr; +} + +/*****************************************************************************/ + static void auth_mgr_changed(NMAuthManager *auth_manager, gpointer user_data) { @@ -8250,6 +8275,8 @@ dispose(GObject *object) g_clear_object(&priv->concheck_mgr); } + g_clear_object(&priv->dns_mgr); + if (priv->auth_mgr) { g_signal_handlers_disconnect_by_func(priv->auth_mgr, G_CALLBACK(auth_mgr_changed), self); g_clear_object(&priv->auth_mgr); diff --git a/src/core/nm-manager.h b/src/core/nm-manager.h index f8563c3aa1..fcb0022720 100644 --- a/src/core/nm-manager.h +++ b/src/core/nm-manager.h @@ -200,6 +200,10 @@ NMMetered nm_manager_get_metered(NMManager *self); void nm_manager_notify_device_availability_maybe_changed(NMManager *self); +struct _NMDnsManager; + +struct _NMDnsManager *nm_manager_get_dns_manager(NMManager *self); + /*****************************************************************************/ void nm_manager_device_auth_request(NMManager *self, From 6c27e58d8dd835bee9ae7a53b31e8b28714907cc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 8 Apr 2022 12:01:51 +0200 Subject: [PATCH 22/23] core: delay startup complete while we have pending DNS updates While we have DNS updates pending, we cannot reach startup complete. --- src/core/nm-manager.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index fab316f4d4..191d65bf65 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -146,6 +146,7 @@ typedef struct { NMPlatform *platform; NMDnsManager *dns_mgr; + gulong dns_mgr_update_pending_signal_id; GArray *capabilities; @@ -349,6 +350,9 @@ static NMActiveConnection *_new_active_connection(NMManager *self, static void policy_activating_ac_changed(GObject *object, GParamSpec *pspec, gpointer user_data); +static void device_has_pending_action_changed(NMDevice *device, GParamSpec *pspec, NMManager *self); +static void check_if_startup_complete(NMManager *self); + static gboolean find_master(NMManager *self, NMConnection *connection, NMDevice *device, @@ -1604,7 +1608,11 @@ manager_device_state_changed(NMDevice *device, nm_settings_device_added(priv->settings, device); } -static void device_has_pending_action_changed(NMDevice *device, GParamSpec *pspec, NMManager *self); +static void +_dns_mgr_update_pending_cb(NMDevice *device, GParamSpec *pspec, NMManager *self) +{ + check_if_startup_complete(self); +} static void check_if_startup_complete(NMManager *self) @@ -1619,6 +1627,20 @@ check_if_startup_complete(NMManager *self) if (!priv->devices_inited) return; + if (nm_dns_manager_get_update_pending(nm_manager_get_dns_manager(self))) { + if (priv->dns_mgr_update_pending_signal_id == 0) { + priv->dns_mgr_update_pending_signal_id = + g_signal_connect(nm_manager_get_dns_manager(self), + "notify::" NM_DNS_MANAGER_UPDATE_PENDING, + G_CALLBACK(_dns_mgr_update_pending_cb), + self); + } + return; + } + + nm_clear_g_signal_handler(nm_manager_get_dns_manager(self), + &priv->dns_mgr_update_pending_signal_id); + c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) { reason = nm_device_has_pending_action_reason(device); if (reason) { @@ -8275,6 +8297,7 @@ dispose(GObject *object) g_clear_object(&priv->concheck_mgr); } + nm_clear_g_signal_handler(priv->dns_mgr, &priv->dns_mgr_update_pending_signal_id); g_clear_object(&priv->dns_mgr); if (priv->auth_mgr) { From 80c9e2d9eca328ee69e9046648bb08f02e05696e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Feb 2022 21:10:51 +0100 Subject: [PATCH 23/23] device: prevent IP state from reaching ready while DNS update pending The goal would be to ensure that a device cannot move to activated, while a DNS update is still pending. This does not really work for most cases. That is, because NMDevice does not directly push DNS updates to NMDnsManager, instead, NMPolicy is watching all device changes, and doing it. But when NMPolicy decides to to that, may not be the right moment. We really should let NMDevice (or better, NML3Cfg) directly talk to NMDnsManager. Why not? They have all the information when new DNS configuration is available. The only thing that NMPolicy does on top of that, is determining which device has the best default route. NMPolicy could continue to do that (or maybe NMDnsManager could), but the update needs to be directly triggered by NMDevice/NML3Cfg. --- src/core/devices/nm-device.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index f554ccfd25..90b1920cc0 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -604,6 +604,7 @@ typedef struct _NMDevicePrivate { const NMDeviceIPState state; NMDeviceIPState state_; }; + gulong dnsmgr_update_pending_signal_id; } ip_data; union { @@ -2929,6 +2930,13 @@ _add_capabilities(NMDevice *self, NMDeviceCapabilities capabilities) /*****************************************************************************/ +static void +_dev_ip_state_dnsmgr_update_pending_changed(NMDnsManager *dnsmgr, GParamSpec *pspec, NMDevice *self) +{ + _dev_ip_state_check(self, AF_INET); + _dev_ip_state_check(self, AF_INET6); +} + static void _dev_ip_state_req_timeout_cancel(NMDevice *self, int addr_family) { @@ -3321,6 +3329,27 @@ got_ip_state: combinedip_state = priv->ip_data.state; } + if (combinedip_state == NM_DEVICE_IP_STATE_READY + && priv->ip_data.state <= NM_DEVICE_IP_STATE_PENDING + && nm_dns_manager_get_update_pending(nm_manager_get_dns_manager(priv->manager))) { + /* We would be ready, but a DNS update is pending. That prevents us from getting fully ready. */ + if (priv->ip_data.dnsmgr_update_pending_signal_id == 0) { + priv->ip_data.dnsmgr_update_pending_signal_id = + g_signal_connect(nm_manager_get_dns_manager(priv->manager), + "notify::" NM_DNS_MANAGER_UPDATE_PENDING, + G_CALLBACK(_dev_ip_state_dnsmgr_update_pending_changed), + self); + _LOGT_ip(AF_UNSPEC, + "check-state: (combined) state: wait for DNS before becoming ready"); + } + combinedip_state = NM_DEVICE_IP_STATE_PENDING; + } + if (combinedip_state != NM_DEVICE_IP_STATE_PENDING + && priv->ip_data.dnsmgr_update_pending_signal_id != 0) { + nm_clear_g_signal_handler(nm_manager_get_dns_manager(priv->manager), + &priv->ip_data.dnsmgr_update_pending_signal_id); + } + _LOGT_ip(AF_UNSPEC, "check-state: (combined) state %s => %s", nm_device_ip_state_to_string(priv->ip_data.state), @@ -12329,7 +12358,8 @@ delete_on_deactivate_check_and_schedule(NMDevice *self) static void _cleanup_ip_pre(NMDevice *self, int addr_family, CleanupType cleanup_type, gboolean from_reapply) { - const int IS_IPv4 = NM_IS_IPv4(addr_family); + const int IS_IPv4 = NM_IS_IPv4(addr_family); + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); _dev_ipsharedx_cleanup(self, addr_family); @@ -12345,6 +12375,9 @@ _cleanup_ip_pre(NMDevice *self, int addr_family, CleanupType cleanup_type, gbool _dev_ipmanual_cleanup(self); + nm_clear_g_signal_handler(nm_manager_get_dns_manager(priv->manager), + &priv->ip_data.dnsmgr_update_pending_signal_id); + _dev_ip_state_cleanup(self, AF_UNSPEC, from_reapply); _dev_ip_state_cleanup(self, addr_family, from_reapply); }