From e4304fb8d46328b2587e3f0b8b538aa66b45a22c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Nov 2020 13:48:02 +0100 Subject: [PATCH 1/2] basic: add set_equal() helper --- src/basic/hashmap.c | 32 ++++++++++++++++++++ src/basic/set.h | 2 ++ src/test/test-set.c | 73 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+) diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c index c118289b99..e354c67adc 100644 --- a/src/basic/hashmap.c +++ b/src/basic/hashmap.c @@ -2036,3 +2036,35 @@ int set_strjoin(Set *s, const char *separator, bool wrap_with_separator, char ** *ret = TAKE_PTR(str); return 0; } + +bool set_equal(Set *a, Set *b) { + void *p; + + /* Checks whether each entry of 'a' is also in 'b' and vice versa, i.e. the two sets contain the same + * entries */ + + if (a == b) + return true; + + if (set_isempty(a) && set_isempty(b)) + return true; + + if (set_size(a) != set_size(b)) /* Cheap check that hopefully catches a lot of inequality cases + * already */ + return false; + + SET_FOREACH(p, a) + if (!set_contains(b, p)) + return false; + + /* If we have the same hashops, then we don't need to check things backwards given we compared the + * size and that all of a is in b. */ + if (a->b.hash_ops == b->b.hash_ops) + return true; + + SET_FOREACH(p, b) + if (!set_contains(a, p)) + return false; + + return true; +} diff --git a/src/basic/set.h b/src/basic/set.h index 57ff713039..52b6f4984c 100644 --- a/src/basic/set.h +++ b/src/basic/set.h @@ -152,3 +152,5 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(Set*, set_free_free); #define _cleanup_set_free_free_ _cleanup_(set_free_freep) int set_strjoin(Set *s, const char *separator, bool wrap_with_separator, char **ret); + +bool set_equal(Set *a, Set *b); diff --git a/src/test/test-set.c b/src/test/test-set.c index b4d07b2078..f89c968f21 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "random-util.h" #include "set.h" #include "strv.h" @@ -227,6 +228,77 @@ static void test_set_strjoin(void) { assert_se(STR_IN_SET(joined, "xxxaaaxxxbbbxxx", "xxxbbbxxxaaaxxx")); } +static void test_set_equal(void) { + _cleanup_set_free_ Set *a = NULL, *b = NULL; + void *p; + int r; + + assert_se(a = set_new(NULL)); + assert_se(b = set_new(NULL)); + + assert_se(set_equal(a, a)); + assert_se(set_equal(b, b)); + assert_se(set_equal(a, b)); + assert_se(set_equal(b, a)); + assert_se(set_equal(NULL, a)); + assert_se(set_equal(NULL, b)); + assert_se(set_equal(a, NULL)); + assert_se(set_equal(b, NULL)); + assert_se(set_equal(NULL, NULL)); + + for (unsigned i = 0; i < 333; i++) { + p = INT32_TO_PTR(1 + (random_u32() & 0xFFFU)); + + r = set_put(a, p); + assert_se(r >= 0 || r == -EEXIST); + } + + assert_se(set_put(a, INT32_TO_PTR(0x1000U)) >= 0); + + assert_se(set_size(a) >= 2); + assert_se(set_size(a) <= 334); + + assert_se(!set_equal(a, b)); + assert_se(!set_equal(b, a)); + assert_se(!set_equal(a, NULL)); + + SET_FOREACH(p, a) + assert_se(set_put(b, p) >= 0); + + assert_se(set_equal(a, b)); + assert_se(set_equal(b, a)); + + assert_se(set_remove(a, INT32_TO_PTR(0x1000U)) == INT32_TO_PTR(0x1000U)); + + assert_se(!set_equal(a, b)); + assert_se(!set_equal(b, a)); + + assert_se(set_remove(b, INT32_TO_PTR(0x1000U)) == INT32_TO_PTR(0x1000U)); + + assert_se(set_equal(a, b)); + assert_se(set_equal(b, a)); + + assert_se(set_put(b, INT32_TO_PTR(0x1001U)) >= 0); + + assert_se(!set_equal(a, b)); + assert_se(!set_equal(b, a)); + + assert_se(set_put(a, INT32_TO_PTR(0x1001U)) >= 0); + + assert_se(set_equal(a, b)); + assert_se(set_equal(b, a)); + + set_clear(a); + + assert_se(!set_equal(a, b)); + assert_se(!set_equal(b, a)); + + set_clear(b); + + assert_se(set_equal(a, b)); + assert_se(set_equal(b, a)); +} + int main(int argc, const char *argv[]) { test_set_steal_first(); test_set_free_with_destructor(); @@ -238,6 +310,7 @@ int main(int argc, const char *argv[]) { test_set_ensure_put(); test_set_ensure_consume(); test_set_strjoin(); + test_set_equal(); return 0; } From cbf23f385381f1df2149b18d1fa2411356d76ad8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Nov 2020 13:52:59 +0100 Subject: [PATCH 2/2] resolved: optimize change notification handling away if bus calls set the same values as were already set Prompted-by: #17577 (but doesn't fix this, since this commit only handles D-Bus-induced changes, not the ones made via networkd) --- src/resolve/resolved-dns-search-domain.c | 12 ++- src/resolve/resolved-dns-search-domain.h | 2 +- src/resolve/resolved-dns-server.c | 12 ++- src/resolve/resolved-dns-server.h | 2 +- src/resolve/resolved-link-bus.c | 107 ++++++++++++++--------- 5 files changed, 82 insertions(+), 53 deletions(-) diff --git a/src/resolve/resolved-dns-search-domain.c b/src/resolve/resolved-dns-search-domain.c index a01f3dced9..94a46570f2 100644 --- a/src/resolve/resolved-dns-search-domain.c +++ b/src/resolve/resolved-dns-search-domain.c @@ -151,18 +151,22 @@ void dns_search_domain_unlink_all(DnsSearchDomain *first) { dns_search_domain_unlink_all(next); } -void dns_search_domain_unlink_marked(DnsSearchDomain *first) { +bool dns_search_domain_unlink_marked(DnsSearchDomain *first) { DnsSearchDomain *next; + bool changed; if (!first) - return; + return false; next = first->domains_next; - if (first->marked) + if (first->marked) { dns_search_domain_unlink(first); + changed = true; + } else + changed = false; - dns_search_domain_unlink_marked(next); + return changed || dns_search_domain_unlink_marked(next); } void dns_search_domain_mark_all(DnsSearchDomain *first) { diff --git a/src/resolve/resolved-dns-search-domain.h b/src/resolve/resolved-dns-search-domain.h index ea91a4e2a4..f0d96aca37 100644 --- a/src/resolve/resolved-dns-search-domain.h +++ b/src/resolve/resolved-dns-search-domain.h @@ -44,7 +44,7 @@ void dns_search_domain_unlink(DnsSearchDomain *d); void dns_search_domain_move_back_and_unmark(DnsSearchDomain *d); void dns_search_domain_unlink_all(DnsSearchDomain *first); -void dns_search_domain_unlink_marked(DnsSearchDomain *first); +bool dns_search_domain_unlink_marked(DnsSearchDomain *first); void dns_search_domain_mark_all(DnsSearchDomain *first); int dns_search_domain_find(DnsSearchDomain *first, const char *name, DnsSearchDomain **ret); diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index 6a5a466af6..b0eeb04137 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -746,18 +746,22 @@ void dns_server_unlink_all(DnsServer *first) { dns_server_unlink_all(next); } -void dns_server_unlink_marked(DnsServer *first) { +bool dns_server_unlink_marked(DnsServer *first) { DnsServer *next; + bool changed; if (!first) - return; + return false; next = first->servers_next; - if (first->marked) + if (first->marked) { + changed = true; dns_server_unlink(first); + } else + changed = false; - dns_server_unlink_marked(next); + return changed || dns_server_unlink_marked(next); } void dns_server_mark_all(DnsServer *first) { diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h index 304d608f7a..689fd42db4 100644 --- a/src/resolve/resolved-dns-server.h +++ b/src/resolve/resolved-dns-server.h @@ -143,7 +143,7 @@ void dns_server_warn_downgrade(DnsServer *server); DnsServer *dns_server_find(DnsServer *first, int family, const union in_addr_union *in_addr, uint16_t port, int ifindex, const char *name); void dns_server_unlink_all(DnsServer *first); -void dns_server_unlink_marked(DnsServer *first); +bool dns_server_unlink_marked(DnsServer *first); void dns_server_mark_all(DnsServer *first); DnsServer *manager_get_first_dns_server(Manager *m, DnsServerType t); diff --git a/src/resolve/resolved-link-bus.c b/src/resolve/resolved-link-bus.c index 8649804674..d56d5de4df 100644 --- a/src/resolve/resolved-link-bus.c +++ b/src/resolve/resolved-link-bus.c @@ -255,6 +255,7 @@ static int verify_unmanaged_link(Link *l, sd_bus_error *error) { static int bus_link_method_set_dns_servers_internal(sd_bus_message *message, void *userdata, sd_bus_error *error, bool extended) { _cleanup_free_ char *j = NULL; struct in_addr_full **dns; + bool changed = false; Link *l = userdata; size_t n; int r; @@ -312,21 +313,26 @@ static int bus_link_method_set_dns_servers_internal(sd_bus_message *message, voi dns_server_unlink_all(l->dns_servers); goto finalize; } + + changed = true; } } - dns_server_unlink_marked(l->dns_servers); - link_allocate_scopes(l); + changed = dns_server_unlink_marked(l->dns_servers) || changed; - (void) link_save_user(l); - (void) manager_write_resolv_conf(l->manager); - (void) manager_send_changed(l->manager, "DNS"); + if (changed) { + link_allocate_scopes(l); - if (j) - log_link_info(l, "Bus client set DNS server list to: %s", j); - else - log_link_info(l, "Bus client reset DNS server list."); + (void) link_save_user(l); + (void) manager_write_resolv_conf(l->manager); + (void) manager_send_changed(l->manager, "DNS"); + + if (j) + log_link_info(l, "Bus client set DNS server list to: %s", j); + else + log_link_info(l, "Bus client reset DNS server list."); + } r = sd_bus_reply_method_return(message, NULL); @@ -349,6 +355,7 @@ int bus_link_method_set_dns_servers_ex(sd_bus_message *message, void *userdata, int bus_link_method_set_domains(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_free_ char *j = NULL; Link *l = userdata; + bool changed = false; int r; assert(message); @@ -431,6 +438,8 @@ int bus_link_method_set_domains(sd_bus_message *message, void *userdata, sd_bus_ r = dns_search_domain_new(l->manager, &d, DNS_SEARCH_DOMAIN_LINK, l, name); if (r < 0) goto clear; + + changed = true; } d->route_only = route_only; @@ -440,15 +449,17 @@ int bus_link_method_set_domains(sd_bus_message *message, void *userdata, sd_bus_ if (r < 0) goto clear; - dns_search_domain_unlink_marked(l->search_domains); + changed = dns_search_domain_unlink_marked(l->search_domains) || changed; - (void) link_save_user(l); - (void) manager_write_resolv_conf(l->manager); + if (changed) { + (void) link_save_user(l); + (void) manager_write_resolv_conf(l->manager); - if (j) - log_link_info(l, "Bus client set search domain list to: %s", j); - else - log_link_info(l, "Bus client reset search domain list."); + if (j) + log_link_info(l, "Bus client set search domain list to: %s", j); + else + log_link_info(l, "Bus client reset search domain list."); + } return sd_bus_reply_method_return(message, NULL); @@ -488,9 +499,9 @@ int bus_link_method_set_default_route(sd_bus_message *message, void *userdata, s (void) link_save_user(l); (void) manager_write_resolv_conf(l->manager); - } - log_link_info(l, "Bus client set default route setting: %s", yes_no(b)); + log_link_info(l, "Bus client set default route setting: %s", yes_no(b)); + } return sd_bus_reply_method_return(message, NULL); } @@ -531,13 +542,15 @@ int bus_link_method_set_llmnr(sd_bus_message *message, void *userdata, sd_bus_er bus_client_log(message, "LLMNR change"); - l->llmnr_support = mode; - link_allocate_scopes(l); - link_add_rrs(l, false); + if (l->llmnr_support != mode) { + l->llmnr_support = mode; + link_allocate_scopes(l); + link_add_rrs(l, false); - (void) link_save_user(l); + (void) link_save_user(l); - log_link_info(l, "Bus client set LLMNR setting: %s", resolve_support_to_string(mode)); + log_link_info(l, "Bus client set LLMNR setting: %s", resolve_support_to_string(mode)); + } return sd_bus_reply_method_return(message, NULL); } @@ -578,13 +591,15 @@ int bus_link_method_set_mdns(sd_bus_message *message, void *userdata, sd_bus_err bus_client_log(message, "mDNS change"); - l->mdns_support = mode; - link_allocate_scopes(l); - link_add_rrs(l, false); + if (l->mdns_support != mode) { + l->mdns_support = mode; + link_allocate_scopes(l); + link_add_rrs(l, false); - (void) link_save_user(l); + (void) link_save_user(l); - log_link_info(l, "Bus client set MulticastDNS setting: %s", resolve_support_to_string(mode)); + log_link_info(l, "Bus client set MulticastDNS setting: %s", resolve_support_to_string(mode)); + } return sd_bus_reply_method_return(message, NULL); } @@ -625,12 +640,14 @@ int bus_link_method_set_dns_over_tls(sd_bus_message *message, void *userdata, sd bus_client_log(message, "D-o-T change"); - link_set_dns_over_tls_mode(l, mode); + if (l->dns_over_tls_mode != mode) { + link_set_dns_over_tls_mode(l, mode); - (void) link_save_user(l); + (void) link_save_user(l); - log_link_info(l, "Bus client set DNSOverTLS setting: %s", - mode < 0 ? "default" : dns_over_tls_mode_to_string(mode)); + log_link_info(l, "Bus client set DNSOverTLS setting: %s", + mode < 0 ? "default" : dns_over_tls_mode_to_string(mode)); + } return sd_bus_reply_method_return(message, NULL); } @@ -671,12 +688,14 @@ int bus_link_method_set_dnssec(sd_bus_message *message, void *userdata, sd_bus_e bus_client_log(message, "DNSSEC change"); - link_set_dnssec_mode(l, mode); + if (l->dnssec_mode != mode) { + link_set_dnssec_mode(l, mode); - (void) link_save_user(l); + (void) link_save_user(l); - log_link_info(l, "Bus client set DNSSEC setting: %s", - mode < 0 ? "default" : dnssec_mode_to_string(mode)); + log_link_info(l, "Bus client set DNSSEC setting: %s", + mode < 0 ? "default" : dnssec_mode_to_string(mode)); + } return sd_bus_reply_method_return(message, NULL); } @@ -731,15 +750,17 @@ int bus_link_method_set_dnssec_negative_trust_anchors(sd_bus_message *message, v bus_client_log(message, "DNSSEC NTA change"); - set_free_free(l->dnssec_negative_trust_anchors); - l->dnssec_negative_trust_anchors = TAKE_PTR(ns); + if (!set_equal(ns, l->dnssec_negative_trust_anchors)) { + set_free_free(l->dnssec_negative_trust_anchors); + l->dnssec_negative_trust_anchors = TAKE_PTR(ns); - (void) link_save_user(l); + (void) link_save_user(l); - if (j) - log_link_info(l, "Bus client set NTA list to: %s", j); - else - log_link_info(l, "Bus client reset NTA list."); + if (j) + log_link_info(l, "Bus client set NTA list to: %s", j); + else + log_link_info(l, "Bus client reset NTA list."); + } return sd_bus_reply_method_return(message, NULL); }