resolve: several cleanups for managing objects (#43629)

src/resolve/resolved-bus.c

@@ -2055,6 +2055,15 @@ static int bus_method_register_service(sd_bus_message *message, void *userdata,
         if (r == 0)
                 return 1; /* Polkit will call us back */
 
+        /* First, attach the service to the manager. */
+        r = hashmap_ensure_put(&m->dnssd_registered_services, &dnssd_registered_service_hash_ops, service->id, service);
+        if (r < 0)
+                return r;
+
+        service->manager = m;
+
+        /* Then, attach the bus track to the service. As dnssd_registered_service_on_bus_track() requires
+         * service->manager is set, this must be done after attaching to the manager in the above. */
         r = sd_bus_track_new(sd_bus_message_get_bus(message), &bus_track, dnssd_registered_service_on_bus_track, service);
         if (r < 0)
                 return r;
@@ -2067,13 +2076,7 @@ static int bus_method_register_service(sd_bus_message *message, void *userdata,
          * bus, hence keep the bus track object pinned for the lifetime of the service. */
         service->bus_track = TAKE_PTR(bus_track);
 
-        service->manager = m;
-
-        r = hashmap_ensure_put(&m->dnssd_registered_services, &string_hash_ops, service->id, service);
-        if (r < 0)
-                return r;
-
-        service = NULL;
+        TAKE_PTR(service);
 
         manager_refresh_rrs(m);
 

src/resolve/resolved-dns-browse-services.c

@@ -108,6 +108,8 @@ static int mdns_maintenance_query(sd_event_source *s, uint64_t usec, void *userd
         _cleanup_(dns_query_freep) DnsQuery *q = NULL;
         int r;
 
+        assert(service->service_browser);
+
         /* Check if the TTL state has reached the maximum value, then revisit
          * cache */
         if (service->rr_ttl_state++ == DNS_RECORD_TTL_STATE_100_PERCENT)
@@ -155,11 +157,12 @@ static int mdns_maintenance_query(sd_event_source *s, uint64_t usec, void *userd
         return 0;
 }
 
-int dns_add_new_service(DnsServiceBrowser *sb, DnsResourceRecord *rr, int owner_family, int ifindex, usec_t until) {
+static int dns_add_new_service(DnsServiceBrowser *sb, DnsResourceRecord *rr, int owner_family, int ifindex, usec_t until) {
         _cleanup_(dnssd_discovered_service_unrefp) DnssdDiscoveredService *s = NULL;
         int r;
 
         assert(sb);
+        assert(sb->manager);
         assert(rr);
 
         s = new(DnssdDiscoveredService, 1);
@@ -170,12 +173,10 @@ int dns_add_new_service(DnsServiceBrowser *sb, DnsResourceRecord *rr, int owner_
 
         *s = (DnssdDiscoveredService) {
                 .n_ref = 1,
-                .service_browser = sb,
                 .rr = dns_resource_record_copy(rr),
                 .family = owner_family,
                 .ifindex = ifindex,
                 .until = until,
-                .query = NULL,
                 .rr_ttl_state = DNS_RECORD_TTL_STATE_80_PERCENT,
         };
         if (!s->rr)
@@ -220,36 +221,41 @@ int dns_add_new_service(DnsServiceBrowser *sb, DnsResourceRecord *rr, int owner_
                                 "Failed to schedule mDNS maintenance query for DNS service: %m");
 
         LIST_PREPEND(dns_services, sb->dns_services, s);
-
+        s->service_browser = sb;
         TAKE_PTR(s);
         return 0;
 }
 
-void dns_remove_service(DnsServiceBrowser *sb, DnssdDiscoveredService *service) {
-        assert(sb);
+static DnssdDiscoveredService* dnssd_discovered_service_detach_impl(DnssdDiscoveredService *service) {
         assert(service);
 
-        LIST_REMOVE(dns_services, sb->dns_services, service);
-        dnssd_discovered_service_unref(service);
-}
-
-DnssdDiscoveredService *dns_service_free(DnssdDiscoveredService *service) {
-        if (!service)
-                return NULL;
-
         service->schedule_event = sd_event_source_disable_unref(service->schedule_event);
 
-        if (service->query && DNS_TRANSACTION_IS_LIVE(service->query->state))
-                dns_query_complete(service->query, DNS_TRANSACTION_ABORTED);
+        if (!service->service_browser)
+                return NULL; /* already detached */
 
+        LIST_REMOVE(dns_services, service->service_browser->dns_services, service);
+        service->service_browser = NULL;
+        return service; /* indicate that the service is detached. */
+}
+
+static void dnssd_discovered_service_detach(DnssdDiscoveredService *service) {
+        dnssd_discovered_service_unref(dnssd_discovered_service_detach_impl(service));
+}
+
+static DnssdDiscoveredService* dnssd_discovered_service_free(DnssdDiscoveredService *service) {
+        if (!service)
+                return NULL;
+
+        dnssd_discovered_service_detach_impl(service);
         service->rr = dns_resource_record_unref(service->rr);
 
         return mfree(service);
 }
 
-DEFINE_TRIVIAL_REF_UNREF_FUNC(DnssdDiscoveredService, dnssd_discovered_service, dns_service_free);
+DEFINE_TRIVIAL_REF_UNREF_FUNC(DnssdDiscoveredService, dnssd_discovered_service, dnssd_discovered_service_free);
 
-int mdns_service_update(DnssdDiscoveredService *service, DnsResourceRecord *rr, usec_t t, usec_t until) {
+static int mdns_service_update(DnssdDiscoveredService *service, DnsResourceRecord *rr, usec_t until) {
         assert(service);
         assert(rr);
 
@@ -301,7 +307,6 @@ int dns_service_match_and_update(
                 int ifindex,
                 usec_t until) {
 
-        usec_t t = now(CLOCK_BOOTTIME);
         int r;
 
         /* Check if a discovered service matching the given resource record, owner family, and ifindex exists
@@ -319,7 +324,7 @@ int dns_service_match_and_update(
                         return 1;
 
                 if (service->until < until)
-                        mdns_service_update(service, rr, t, until);
+                        mdns_service_update(service, rr, until);
 
                 return 1;
         }
@@ -381,7 +386,7 @@ void dns_browse_services_purge(Manager *m, int family) {
         }
 }
 
-int mdns_manage_services_answer(DnsServiceBrowser *sb, DnsAnswer *answer, int owner_family) {
+static int mdns_manage_services_answer(DnsServiceBrowser *sb, DnsAnswer *answer, int owner_family) {
         DnsAnswerItem *item;
         _cleanup_(sd_json_variant_unrefp) sd_json_variant *array = NULL;
         int r;
@@ -497,7 +502,7 @@ int mdns_manage_services_answer(DnsServiceBrowser *sb, DnsAnswer *answer, int ow
                 /* Capture ifindex before removing the service */
                 ifindex = service->ifindex;
 
-                dns_remove_service(sb, service);
+                dnssd_discovered_service_detach(service);
 
                 log_debug("Remove from the list %s, %s, %s, %s, %d",
                           strna(name),
@@ -725,6 +730,7 @@ static int mdns_next_query_schedule(sd_event_source *s, uint64_t usec, void *use
 
         assert(userdata);
         assert_se(sb = dns_service_browser_ref(userdata));
+        assert(sb->manager);
 
         /* If the varlink connection has a userdata, then that means the previous query has not been finished. */
         if (!sd_varlink_get_userdata(sb->link)) {
@@ -804,6 +810,14 @@ void dns_browse_services_restart(Manager *m) {
         }
 }
 
+DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(
+        dns_service_browser_hash_ops,
+        void,
+        trivial_hash_func,
+        trivial_compare_func,
+        DnsServiceBrowser,
+        dns_service_browser_detach);
+
 int dns_subscribe_browse_service(
                 Manager *m, sd_varlink *link, const char *domain, const char *type, int ifindex, uint64_t flags) {
 
@@ -855,8 +869,6 @@ int dns_subscribe_browse_service(
 
         *sb = (DnsServiceBrowser) {
                 .n_ref = 1,
-                .manager = m,
-                .link = sd_varlink_ref(link),
                 .question_utf8 = dns_question_ref(question_utf8),
                 .question_idna = dns_question_ref(question_idna),
                 .key = dns_question_first_key(question_utf8),
@@ -880,35 +892,51 @@ int dns_subscribe_browse_service(
         if (r < 0)
                 return r;
 
-        r = hashmap_ensure_put(&m->dns_service_browsers, NULL, link, sb);
+        r = hashmap_ensure_put(&m->dns_service_browsers, &dns_service_browser_hash_ops, link, sb);
         if (r < 0)
                 return log_error_errno(r, "Failed to add service browser to the hashmap: %m");
 
+        sb->manager = m;
+        sb->link = sd_varlink_ref(link);
         TAKE_PTR(sb);
 
         return 0;
 }
 
-DnsServiceBrowser *dns_service_browser_free(DnsServiceBrowser *sb) {
-        DnsQuery *q;
-
-        if (!sb)
-                return NULL;
+static DnsServiceBrowser* dns_service_browser_detach_impl(DnsServiceBrowser *sb) {
+        assert(sb);
+        assert(!sb->manager == !sb->link);
 
         while (sb->dns_services)
-                dns_remove_service(sb, sb->dns_services);
+                dnssd_discovered_service_detach(sb->dns_services);
 
         sb->schedule_event = sd_event_source_disable_unref(sb->schedule_event);
 
-        q = sd_varlink_get_userdata(sb->link);
+        if (!sb->manager)
+                return NULL; /* already detached */
+
+        DnsQuery *q = sd_varlink_get_userdata(sb->link);
         if (q && DNS_TRANSACTION_IS_LIVE(q->state))
                 dns_query_complete(q, DNS_TRANSACTION_ABORTED);
 
+        hashmap_remove(sb->manager->dns_service_browsers, sb->link);
+        sb->link = sd_varlink_unref(sb->link);
+        sb->manager = NULL;
+        return sb; /* indicate that the object is detached */
+}
+
+void dns_service_browser_detach(DnsServiceBrowser *sb) {
+        dns_service_browser_unref(dns_service_browser_detach_impl(sb));
+}
+
+static DnsServiceBrowser* dns_service_browser_free(DnsServiceBrowser *sb) {
+        if (!sb)
+                return NULL;
+
+        dns_service_browser_detach_impl(sb);
         sb->question_idna = dns_question_unref(sb->question_idna);
         sb->question_utf8 = dns_question_unref(sb->question_utf8);
 
-        sb->link = sd_varlink_unref(sb->link);
-
         return mfree(sb);
 }
 

src/resolve/resolved-dns-browse-services.h

@@ -33,7 +33,6 @@ struct DnssdDiscoveredService {
         int ifindex;
         usec_t until;
         DnsRecordTTLState rr_ttl_state;
-        DnsQuery *query;
         LIST_FIELDS(DnssdDiscoveredService, dns_services);
 };
 
@@ -52,10 +51,7 @@ struct DnsServiceBrowser {
         LIST_HEAD(DnssdDiscoveredService, dns_services);
 };
 
-DnsServiceBrowser *dns_service_browser_free(DnsServiceBrowser *sb);
-void dns_remove_service(DnsServiceBrowser *sb, DnssdDiscoveredService *service);
-DnssdDiscoveredService *dns_service_free(DnssdDiscoveredService *service);
-
+void dns_service_browser_detach(DnsServiceBrowser *sb);
 DECLARE_TRIVIAL_REF_UNREF_FUNC(DnsServiceBrowser, dns_service_browser);
 DECLARE_TRIVIAL_REF_UNREF_FUNC(DnssdDiscoveredService, dnssd_discovered_service);
 
@@ -75,9 +71,6 @@ int mdns_answer_contains_service(
                 DnsServiceBrowser *sb,
                 DnsAnswer *answer,
                 DnssdDiscoveredService *service);
-int mdns_manage_services_answer(DnsServiceBrowser *sb, DnsAnswer *answer, int owner_family);
-int dns_add_new_service(DnsServiceBrowser *sb, DnsResourceRecord *rr, int owner_family, int ifindex, usec_t until);
-int mdns_service_update(DnssdDiscoveredService *service, DnsResourceRecord *rr, usec_t t, usec_t until);
 int mdns_browser_revisit_cache(DnsServiceBrowser *sb, int owner_family);
 int dns_subscribe_browse_service(
                 Manager *m,

src/resolve/resolved-dnssd.c

@@ -27,7 +27,7 @@
 
 #define DNSSD_SERVICE_DIRS ((const char* const*) CONF_PATHS_STRV("systemd/dnssd"))
 
-DnssdTxtData *dnssd_txtdata_free(DnssdTxtData *txt_data) {
+DnssdTxtData* dnssd_txtdata_free(DnssdTxtData *txt_data) {
         if (!txt_data)
                 return NULL;
 
@@ -37,7 +37,7 @@ DnssdTxtData *dnssd_txtdata_free(DnssdTxtData *txt_data) {
         return mfree(txt_data);
 }
 
-DnssdTxtData *dnssd_txtdata_free_all(DnssdTxtData *txt_data) {
+static DnssdTxtData* dnssd_txtdata_free_all(DnssdTxtData *txt_data) {
         DnssdTxtData *next;
 
         if (!txt_data)
@@ -50,7 +50,7 @@ DnssdTxtData *dnssd_txtdata_free_all(DnssdTxtData *txt_data) {
         return dnssd_txtdata_free_all(next);
 }
 
-DnssdRegisteredService *dnssd_registered_service_free(DnssdRegisteredService *service) {
+DnssdRegisteredService* dnssd_registered_service_free(DnssdRegisteredService *service) {
         if (!service)
                 return NULL;
 
@@ -74,10 +74,19 @@ DnssdRegisteredService *dnssd_registered_service_free(DnssdRegisteredService *se
         return mfree(service);
 }
 
+DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR(
+        dnssd_registered_service_hash_ops,
+        char,
+        string_hash_func,
+        string_compare_func,
+        DnssdRegisteredService,
+        dnssd_registered_service_free);
+
 static int dnssd_registered_service_make_goodbye(
                 DnssdRegisteredService *service,
                 DnsScope *scope,
                 DnsAnswer **ret) {
+
         _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL;
         size_t n_rrs = 3;
         int r;
@@ -263,7 +272,7 @@ static int dnssd_registered_service_load(Manager *manager, const char *path) {
                 TAKE_PTR(txt_data);
         }
 
-        r = hashmap_ensure_put(&manager->dnssd_registered_services, &string_hash_ops, service->id, service);
+        r = hashmap_ensure_put(&manager->dnssd_registered_services, &dnssd_registered_service_hash_ops, service->id, service);
         if (r < 0)
                 return r;
 
@@ -433,10 +442,11 @@ oom:
         return -ENOMEM;
 }
 
-int dnssd_txt_item_new_from_string(const char *key, const char *value, DnsTxtItem **ret_item) {
+static int dnssd_txt_item_new_from_string(const char *key, const char *value, DnsTxtItem **ret_item) {
         size_t length;
         DnsTxtItem *i;
 
+        assert(key);
         assert(ret_item);
 
         length = strlen(key);
@@ -464,6 +474,8 @@ int dnssd_txt_item_new_from_data(const char *key, const void *data, const size_t
         size_t length;
         DnsTxtItem *i;
 
+        assert(key);
+        assert(data || size == 0);
         assert(ret_item);
 
         length = strlen(key);
@@ -491,6 +503,8 @@ int dnssd_signal_conflict(Manager *manager, const char *name) {
         DnssdRegisteredService *s;
         int r;
 
+        assert(manager);
+
         if (sd_bus_is_ready(manager->bus) <= 0)
                 return 0;
 

src/resolve/resolved-dnssd.h

@@ -50,18 +50,19 @@ typedef struct DnssdRegisteredService {
         uid_t originator;
 } DnssdRegisteredService;
 
-DnssdRegisteredService *dnssd_registered_service_free(DnssdRegisteredService *service);
-void dnssd_registered_service_unregister(DnssdRegisteredService *service);
-DnssdTxtData *dnssd_txtdata_free(DnssdTxtData *txt_data);
-DnssdTxtData *dnssd_txtdata_free_all(DnssdTxtData *txt_data);
-void dnssd_registered_service_clear_on_reload(Hashmap *services);
+extern const struct hash_ops dnssd_registered_service_hash_ops;
 
-DEFINE_TRIVIAL_CLEANUP_FUNC(DnssdRegisteredService*, dnssd_registered_service_free);
+DnssdTxtData* dnssd_txtdata_free(DnssdTxtData *txt_data);
 DEFINE_TRIVIAL_CLEANUP_FUNC(DnssdTxtData*, dnssd_txtdata_free);
 
+DnssdRegisteredService* dnssd_registered_service_free(DnssdRegisteredService *service);
+DEFINE_TRIVIAL_CLEANUP_FUNC(DnssdRegisteredService*, dnssd_registered_service_free);
+
+void dnssd_registered_service_unregister(DnssdRegisteredService *service);
+void dnssd_registered_service_clear_on_reload(Hashmap *services);
+
 int dnssd_render_instance_name(Manager *m, DnssdRegisteredService *s, char **ret);
 int dnssd_load(Manager *manager);
-int dnssd_txt_item_new_from_string(const char *key, const char *value, DnsTxtItem **ret_item);
 int dnssd_txt_item_new_from_data(const char *key, const void *data, size_t size, DnsTxtItem **ret_item);
 int dnssd_update_rrs(DnssdRegisteredService *s);
 int dnssd_signal_conflict(Manager *manager, const char *name);

src/resolve/resolved-link.c

@@ -34,6 +34,14 @@
 #include "strv.h"
 #include "tmpfile-util.h"
 
+DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(
+        link_hash_ops,
+        void,
+        trivial_hash_func,
+        trivial_compare_func,
+        Link,
+        link_free);
+
 int link_new(Manager *m, Link **ret, int ifindex) {
         _cleanup_(link_freep) Link *l = NULL;
         int r;
@@ -58,7 +66,7 @@ int link_new(Manager *m, Link **ret, int ifindex) {
         if (asprintf(&l->state_file, "/run/systemd/resolve/netif/%i", ifindex) < 0)
                 return -ENOMEM;
 
-        r = hashmap_ensure_put(&m->links, NULL, INT_TO_PTR(ifindex), l);
+        r = hashmap_ensure_put(&m->links, &link_hash_ops, INT_TO_PTR(ifindex), l);
         if (r < 0)
                 return r;
 

src/resolve/resolved-manager.c

@@ -839,10 +839,6 @@ int manager_start(Manager *m) {
 }
 
 Manager* manager_free(Manager *m) {
-        Link *l;
-        DnssdRegisteredService *s;
-        DnsServiceBrowser *sb;
-
         if (!m)
                 return NULL;
 
@@ -850,9 +846,7 @@ Manager* manager_free(Manager *m) {
         dns_server_unlink_all(m->fallback_dns_servers);
         dns_search_domain_unlink_all(m->search_domains);
 
-        while ((l = hashmap_first(m->links)))
-               link_free(l);
-
+        m->links = hashmap_free(m->links);
         m->delegates = hashmap_free(m->delegates);
 
         while (m->dns_queries)
@@ -871,7 +865,6 @@ Manager* manager_free(Manager *m) {
 #endif
 
         set_free(m->refuse_record_types);
-        hashmap_free(m->links);
         hashmap_free(m->dns_transactions);
 
         sd_event_source_unref(m->network_event_source);
@@ -915,16 +908,12 @@ Manager* manager_free(Manager *m) {
         free(m->llmnr_hostname);
         free(m->mdns_hostname);
 
-        while ((s = hashmap_first(m->dnssd_registered_services)))
-               dnssd_registered_service_free(s);
         hashmap_free(m->dnssd_registered_services);
 
         dns_trust_anchor_flush(&m->trust_anchor);
         manager_etc_hosts_flush(m);
         manager_static_records_flush(m);
 
-        while ((sb = hashmap_first(m->dns_service_browsers)))
-                dns_service_browser_free(sb);
         hashmap_free(m->dns_service_browsers);
 
         hashmap_free(m->hooks);

src/resolve/resolved-varlink.c

@@ -157,28 +157,19 @@ static int reply_query_state(DnsQuery *q) {
 }
 
 static void vl_on_disconnect(sd_varlink_server *s, sd_varlink *link, void *userdata) {
-        DnsQuery *q;
-        Manager *m;
-
         assert(s);
         assert(link);
 
-        m = sd_varlink_server_get_userdata(s);
-        if (!m)
-                return;
+        DnsQuery *q = sd_varlink_get_userdata(link);
+        if (q && DNS_TRANSACTION_IS_LIVE(q->state)) {
+                log_debug("Client of active query vanished, aborting query.");
+                dns_query_complete(q, DNS_TRANSACTION_ABORTED);
+        }
 
-        DnsServiceBrowser *sb = hashmap_remove(m->dns_service_browsers, link);
-        dns_service_browser_unref(sb);
-
-        q = sd_varlink_get_userdata(link);
-        if (!q)
-                return;
-
-        if (!DNS_TRANSACTION_IS_LIVE(q->state))
-                return;
-
-        log_debug("Client of active query vanished, aborting query.");
-        dns_query_complete(q, DNS_TRANSACTION_ABORTED);
+        Manager *m = ASSERT_PTR(sd_varlink_server_get_userdata(s));
+        DnsServiceBrowser *sb = hashmap_get(m->dns_service_browsers, link);
+        if (sb)
+                dns_service_browser_detach(sb);
 }
 
 static void vl_on_notification_disconnect(sd_varlink_server *s, sd_varlink *link, void *userdata) {