From 4401c6d5672718b7f87e79a5dd2e50520f0d0811 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 Nov 2020 13:35:13 +0100 Subject: [PATCH 1/4] dns: cleanup RequestItem and track ifindex and self parameter We will need these changes next: - add "self" and "ifindex" fields to RequestItem struct. We will pass on these structs are user-data for the callbacks, so that we afterwards know which request completed. - add DBUS_OP_SET_LINK_DEFAULT_ROUTE global variable. We don't clone the "operation" string but use string literals. However, string literals are not guaranteed to be deduplicated, so we should only compare them with strcmp(). The static variable avoids this: we can use pointer equality to compare it. This will be used next. (cherry picked from commit 8af6647cdaac067cbe37f84bc7d47351f343fa34) --- src/dns/nm-dns-systemd-resolved.c | 71 +++++++++++++++++++------------ 1 file changed, 43 insertions(+), 28 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 0e2c85c2a..690bd4752 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -34,6 +34,9 @@ #define SYSTEMD_RESOLVED_MANAGER_IFACE "org.freedesktop.resolve1.Manager" #define SYSTEMD_RESOLVED_DBUS_PATH "/org/freedesktop/resolve1" +/* define a variable, so that we can compare the operation with pointer equality. */ +static const char *const DBUS_OP_SET_LINK_DEFAULT_ROUTE = "SetLinkDefaultRoute"; + /*****************************************************************************/ typedef struct { @@ -42,9 +45,11 @@ typedef struct { } InterfaceConfig; typedef struct { - CList request_queue_lst; - const char *operation; - GVariant * argument; + CList request_queue_lst; + const char * operation; + GVariant * argument; + NMDnsSystemdResolved *self; + int ifindex; } RequestItem; /*****************************************************************************/ @@ -88,18 +93,26 @@ _request_item_free(RequestItem *request_item) { c_list_unlink_stale(&request_item->request_queue_lst); g_variant_unref(request_item->argument); - g_slice_free(RequestItem, request_item); + nm_g_slice_free(request_item); } static void -_request_item_append(CList *request_queue_lst_head, const char *operation, GVariant *argument) +_request_item_append(NMDnsSystemdResolved *self, + const char * operation, + int ifindex, + GVariant * argument) { - RequestItem *request_item; + NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + RequestItem * request_item; - request_item = g_slice_new(RequestItem); - request_item->operation = operation; - request_item->argument = g_variant_ref_sink(argument); - c_list_link_tail(request_queue_lst_head, &request_item->request_queue_lst); + request_item = g_slice_new(RequestItem); + *request_item = (RequestItem){ + .operation = operation, + .argument = g_variant_ref_sink(argument), + .self = self, + .ifindex = ifindex, + }; + c_list_link_tail(&priv->request_queue_lst_head, &request_item->request_queue_lst); } /*****************************************************************************/ @@ -116,13 +129,14 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) { gs_unref_variant GVariant *v = NULL; gs_free_error GError * error = NULL; - NMDnsSystemdResolved * self = (NMDnsSystemdResolved *) user_data; + NMDnsSystemdResolved * self; NMDnsSystemdResolvedPrivate *priv; v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), r, &error); - if (!v && g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + if (nm_utils_error_is_cancelled(error)) return; + self = user_data; priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); if (!v) { @@ -198,14 +212,14 @@ free_pending_updates(NMDnsSystemdResolved *self) static gboolean prepare_one_interface(NMDnsSystemdResolved *self, InterfaceConfig *ic) { - NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); - GVariantBuilder dns, domains; - NMCListElem * elem; - NMSettingConnectionMdns mdns = NM_SETTING_CONNECTION_MDNS_DEFAULT; - NMSettingConnectionLlmnr llmnr = NM_SETTING_CONNECTION_LLMNR_DEFAULT; - const char * mdns_arg = NULL, *llmnr_arg = NULL; - gboolean has_config = FALSE; - gboolean has_default_route = FALSE; + GVariantBuilder dns; + GVariantBuilder domains; + NMCListElem * elem; + NMSettingConnectionMdns mdns = NM_SETTING_CONNECTION_MDNS_DEFAULT; + NMSettingConnectionLlmnr llmnr = NM_SETTING_CONNECTION_LLMNR_DEFAULT; + const char * mdns_arg = NULL, *llmnr_arg = NULL; + gboolean has_config = FALSE; + gboolean has_default_route = FALSE; g_variant_builder_init(&dns, G_VARIANT_TYPE("(ia(iay))")); g_variant_builder_add(&dns, "i", ic->ifindex); @@ -268,19 +282,20 @@ prepare_one_interface(NMDnsSystemdResolved *self, InterfaceConfig *ic) if (!nm_str_is_empty(mdns_arg) || !nm_str_is_empty(llmnr_arg)) has_config = TRUE; - _request_item_append(&priv->request_queue_lst_head, - "SetLinkDomains", - g_variant_builder_end(&domains)); - _request_item_append(&priv->request_queue_lst_head, - "SetLinkDefaultRoute", + _request_item_append(self, "SetLinkDomains", ic->ifindex, g_variant_builder_end(&domains)); + _request_item_append(self, + DBUS_OP_SET_LINK_DEFAULT_ROUTE, + ic->ifindex, g_variant_new("(ib)", ic->ifindex, has_default_route)); - _request_item_append(&priv->request_queue_lst_head, + _request_item_append(self, "SetLinkMulticastDNS", + ic->ifindex, g_variant_new("(is)", ic->ifindex, mdns_arg ?: "")); - _request_item_append(&priv->request_queue_lst_head, + _request_item_append(self, "SetLinkLLMNR", + ic->ifindex, g_variant_new("(is)", ic->ifindex, llmnr_arg ?: "")); - _request_item_append(&priv->request_queue_lst_head, "SetLinkDNS", g_variant_builder_end(&dns)); + _request_item_append(self, "SetLinkDNS", ic->ifindex, g_variant_builder_end(&dns)); return has_config; } From 3cb7b3a8a26ef295ba198f6214f847853ab21ce1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 Nov 2020 13:45:44 +0100 Subject: [PATCH 2/4] dns: minor cleanup of call_done() in "nm-dns-systemd-resolved.c" (cherry picked from commit 42d47d1cd762b6d2b6d8b92b66647bd9308ad09e) --- src/dns/nm-dns-systemd-resolved.c | 39 +++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 690bd4752..7889ca500 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -131,22 +131,37 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) gs_free_error GError * error = NULL; NMDnsSystemdResolved * self; 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; + goto out; - self = user_data; - priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + request_item = user_data; + self = request_item->self; + priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); - if (!v) { - if (!priv->send_updates_warn_ratelimited) { - priv->send_updates_warn_ratelimited = TRUE; - _LOGW("send-updates failed to update systemd-resolved: %s", error->message); - } else - _LOGD("send-updates failed: %s", error->message); - } else + if (v) { priv->send_updates_warn_ratelimited = FALSE; + goto out; + } + + log_level = LOGL_DEBUG; + + if (!priv->send_updates_warn_ratelimited) { + 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); + +out: + _request_item_free(request_item); } static gboolean @@ -364,8 +379,8 @@ send_updates(NMDnsSystemdResolved *self) -1, priv->cancellable, call_done, - self); - _request_item_free(request_item); + request_item); + c_list_unlink(&request_item->request_queue_lst); } } From 3f16b988a49a731eb0fd94d2f96b0fc505cd62da Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 Nov 2020 13:49:35 +0100 Subject: [PATCH 3/4] dns: preserve DNS settings for systemd-resolved to resend When the DNS settings change, we update the request_queue_lst_head list, with all the requests we want to send. Then, send_updates() will try to send it. It might not do it right away, if resolved is not on the bus or the D-Bus connection is not fully inialized (meaning, we don't know the name owner yet). In those cases, we would keep the list of requests, and send them later. However, when sending them, we would also forget about the configuration. That means, if you restart systemd-resolved, then the daemon drops off the bus and reappears. I think that systemd-resolved in fact persists the configuration during restart. So, usually the settings are still the same after restart. However, we should do better here: if the service appears, we should send the settings again. This means to not forget the requests after we send them once -- at least, until a new update replaces them. (cherry picked from commit 4fc44952f7fd44771a1c006529cea6db937290ec) --- src/dns/nm-dns-systemd-resolved.c | 35 ++++++++++++++++++------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 7889ca500..9290eeeb4 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -64,6 +64,7 @@ typedef struct { bool try_start_blocked : 1; bool dbus_has_owner : 1; bool dbus_initied : 1; + bool request_queue_to_send : 1; } NMDnsSystemdResolvedPrivate; struct _NMDnsSystemdResolved { @@ -136,7 +137,7 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), r, &error); if (nm_utils_error_is_cancelled(error)) - goto out; + return; request_item = user_data; self = request_item->self; @@ -144,7 +145,7 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) if (v) { priv->send_updates_warn_ratelimited = FALSE; - goto out; + return; } log_level = LOGL_DEBUG; @@ -159,9 +160,6 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) request_item->operation, request_item->ifindex, error->message); - -out: - _request_item_free(request_item); } static gboolean @@ -321,7 +319,7 @@ send_updates(NMDnsSystemdResolved *self) NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); RequestItem * request_item; - if (c_list_is_empty(&priv->request_queue_lst_head)) { + if (!priv->request_queue_to_send) { /* nothing to do. */ return; } @@ -351,15 +349,21 @@ send_updates(NMDnsSystemdResolved *self) return; } - _LOGT("send-updates: start %lu requests", c_list_length(&priv->request_queue_lst_head)); - nm_clear_g_cancellable(&priv->cancellable); + if (c_list_is_empty(&priv->request_queue_lst_head)) { + _LOGT("send-updates: no requests to send"); + priv->request_queue_to_send = FALSE; + return; + } + + _LOGT("send-updates: start %lu requests", c_list_length(&priv->request_queue_lst_head)); + priv->cancellable = g_cancellable_new(); - while ( - (request_item = - c_list_first_entry(&priv->request_queue_lst_head, RequestItem, request_queue_lst))) { + priv->request_queue_to_send = FALSE; + + c_list_for_each_entry (request_item, &priv->request_queue_lst_head, request_queue_lst) { /* Above we explicitly call "StartServiceByName" trying to avoid D-Bus activating systmd-resolved * multiple times. There is still a race, were we might hit this line although actually * the service just quit this very moment. In that case, we would try to D-Bus activate the @@ -380,7 +384,6 @@ send_updates(NMDnsSystemdResolved *self) priv->cancellable, call_done, request_item); - c_list_unlink(&request_item->request_queue_lst); } } @@ -452,8 +455,8 @@ update(NMDnsPlugin * plugin, } } + priv->request_queue_to_send = TRUE; send_updates(self); - return TRUE; } @@ -472,8 +475,10 @@ name_owner_changed(NMDnsSystemdResolved *self, const char *owner) _LOGT("D-Bus name for systemd-resolved has owner %s", owner); priv->dbus_has_owner = !!owner; - if (owner) - priv->try_start_blocked = FALSE; + if (owner) { + priv->try_start_blocked = FALSE; + priv->request_queue_to_send = TRUE; + } send_updates(self); } From c182984469389ece405180874040de3c21ee269c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 25 Nov 2020 17:27:04 +0100 Subject: [PATCH 4/4] dns: detect support of systemd-resolved's SetLinkDefaultRoute() and avoid it We now always use SetLinkDefaultRoute(), but that API was only added in systemd v240 ([1]). We could just always call the non-existing method, and ignore the error. However, that feels ugly. Would systemd-resolved log warnings about that? Should we suppress all messages about that failure (not good for debugging). Instead, make an effort to detect support of the function, and avoid calling it. That is significantly more complicated than just always calling the method and not care. Note that even if systemd-resolved does not support SetLinkDefaultRoute(), we cannot do anything smart about that. We would simply rely on systemd-resolved (hopefully) doing the right thing automatically. That's better and simpler than explicitly adding a "~." domain in the fallback case. Also, detecting support is straight forward in the common case, where there is either success or a clear "org.freedesktop.DBus.Error.UnknownMethod" error. In cases where there is any other failure, we don't really know. In that case, we keep trying to use the API under the assumption that it should work. [1] https://github.com/systemd/systemd/commit/7 ## 7673795dcf5797491e7f785cbf5077d29a15db4 (cherry picked from commit 44ebb99cfaf4c5db350f090b133b5048e188b9d5) --- src/dns/nm-dns-systemd-resolved.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 9290eeeb4..8e22041e4 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -65,6 +65,7 @@ typedef struct { bool dbus_has_owner : 1; bool dbus_initied : 1; bool request_queue_to_send : 1; + NMTernary has_link_default_route : 3; } NMDnsSystemdResolvedPrivate; struct _NMDnsSystemdResolved { @@ -144,17 +145,29 @@ call_done(GObject *source, GAsyncResult *r, gpointer user_data) priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); if (v) { + if (request_item->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"); + } priv->send_updates_warn_ratelimited = FALSE; return; } - log_level = LOGL_DEBUG; + if (request_item->operation == DBUS_OP_SET_LINK_DEFAULT_ROUTE + && nm_g_error_matches(error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD)) { + if (priv->has_link_default_route == NM_TERNARY_DEFAULT) { + priv->has_link_default_route = NM_TERNARY_FALSE; + _LOGD("systemd-resolved support for SetLinkDefaultRoute(): API not supported"); + } + return; + } + log_level = LOGL_DEBUG; if (!priv->send_updates_warn_ratelimited) { priv->send_updates_warn_ratelimited = TRUE; log_level = LOGL_WARN; } - _NMLOG(log_level, "send-updates %s@%d failed: %s", request_item->operation, @@ -364,6 +377,15 @@ send_updates(NMDnsSystemdResolved *self) priv->request_queue_to_send = FALSE; c_list_for_each_entry (request_item, &priv->request_queue_lst_head, request_queue_lst) { + if (request_item->operation == DBUS_OP_SET_LINK_DEFAULT_ROUTE + && priv->has_link_default_route == NM_TERNARY_FALSE) { + /* The "SetLinkDefaultRoute" API is only supported since v240. + * We detected that it is not supported, and skip the call. There + * is no special workaround, because in this case we rely on systemd-resolved + * to do the right thing automatically. */ + continue; + } + /* Above we explicitly call "StartServiceByName" trying to avoid D-Bus activating systmd-resolved * multiple times. There is still a race, were we might hit this line although actually * the service just quit this very moment. In that case, we would try to D-Bus activate the @@ -478,7 +500,8 @@ name_owner_changed(NMDnsSystemdResolved *self, const char *owner) if (owner) { priv->try_start_blocked = FALSE; priv->request_queue_to_send = TRUE; - } + } else + priv->has_link_default_route = NM_TERNARY_DEFAULT; send_updates(self); } @@ -551,6 +574,8 @@ nm_dns_systemd_resolved_init(NMDnsSystemdResolved *self) { NMDnsSystemdResolvedPrivate *priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE(self); + priv->has_link_default_route = NM_TERNARY_DEFAULT; + c_list_init(&priv->request_queue_lst_head); priv->dirty_interfaces = g_hash_table_new(nm_direct_hash, NULL);