From c805014a350da7c32eb2bdb408a9fa643c191dee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 29 Jan 2021 16:18:12 +0100 Subject: [PATCH 1/2] resolved: minor cleanups --- src/resolve/resolved-dns-query.c | 7 +++---- src/resolve/resolved-dns-query.h | 16 ++++++++-------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 757fbedf99..d10a2db9c2 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -50,7 +50,6 @@ static void dns_query_candidate_stop(DnsQueryCandidate *c) { } DnsQueryCandidate* dns_query_candidate_free(DnsQueryCandidate *c) { - if (!c) return NULL; @@ -69,7 +68,7 @@ DnsQueryCandidate* dns_query_candidate_free(DnsQueryCandidate *c) { } static int dns_query_candidate_next_search_domain(DnsQueryCandidate *c) { - DnsSearchDomain *next = NULL; + DnsSearchDomain *next; assert(c); @@ -602,8 +601,8 @@ static int dns_query_try_etc_hosts(DnsQuery *q) { assert(q); - /* Looks in /etc/hosts for matching entries. Note that this is done *before* the normal lookup is done. The - * data from /etc/hosts hence takes precedence over the network. */ + /* Looks in /etc/hosts for matching entries. Note that this is done *before* the normal lookup is + * done. The data from /etc/hosts hence takes precedence over the network. */ r = manager_etc_hosts_lookup( q->manager, diff --git a/src/resolve/resolved-dns-query.h b/src/resolve/resolved-dns-query.h index 8056aa3727..6aadf28a15 100644 --- a/src/resolve/resolved-dns-query.h +++ b/src/resolve/resolved-dns-query.h @@ -31,19 +31,19 @@ struct DnsQueryCandidate { struct DnsQuery { Manager *manager; - /* When resolving a service, we first create a TXT+SRV query, - * and then for the hostnames we discover auxiliary A+AAAA - * queries. This pointer always points from the auxiliary - * queries back to the TXT+SRV query. */ + /* When resolving a service, we first create a TXT+SRV query, and then for the hostnames we discover + * auxiliary A+AAAA queries. This pointer always points from the auxiliary queries back to the + * TXT+SRV query. */ DnsQuery *auxiliary_for; LIST_HEAD(DnsQuery, auxiliary_queries); unsigned n_auxiliary_queries; int auxiliary_result; - /* The question, formatted in IDNA for use on classic DNS, and as UTF8 for use in LLMNR or mDNS. Note that even - * on classic DNS some labels might use UTF8 encoding. Specifically, DNS-SD service names (in contrast to their - * domain suffixes) use UTF-8 encoding even on DNS. Thus, the difference between these two fields is mostly - * relevant only for explicit *hostname* lookups as well as the domain suffixes of service lookups. */ + /* The question, formatted in IDNA for use on classic DNS, and as UTF8 for use in LLMNR or mDNS. Note + * that even on classic DNS some labels might use UTF8 encoding. Specifically, DNS-SD service names + * (in contrast to their domain suffixes) use UTF-8 encoding even on DNS. Thus, the difference + * between these two fields is mostly relevant only for explicit *hostname* lookups as well as the + * domain suffixes of service lookups. */ DnsQuestion *question_idna; DnsQuestion *question_utf8; From 0e0fd08fc832b8f42e567d722d388eba086da5ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 29 Jan 2021 16:21:08 +0100 Subject: [PATCH 2/2] resolved: use reference counting for DnsQueryCandidate objects Follow-up for 4ea8b443de. The logic that manages DnsQueryCandidate is rather complicated: a calls to dns_query_complete() that wants to destroy a DnsQueryCandidate can be nested inside a deep chain of calls. Using reference counts seems like the simplest approach. DnsSearchDomain already uses reference counting. This patch effectively brings dns_query_candidate_go() to the state before 4ea8b443de, but wraps the iteration over DnsQueryCandidate.transactions in dns_query_candidate_ref+dns_query_candidate_unref. --- src/resolve/resolved-dns-query.c | 31 ++++++++++++++----------------- src/resolve/resolved-dns-query.h | 9 ++++++--- src/resolve/resolved-dns-scope.c | 2 +- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index d10a2db9c2..8ee4fd8309 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -26,6 +26,7 @@ static int dns_query_candidate_new(DnsQueryCandidate **ret, DnsQuery *q, DnsScop return -ENOMEM; *c = (DnsQueryCandidate) { + .n_ref = 1, .query = q, .scope = s, }; @@ -49,7 +50,7 @@ static void dns_query_candidate_stop(DnsQueryCandidate *c) { } } -DnsQueryCandidate* dns_query_candidate_free(DnsQueryCandidate *c) { +static DnsQueryCandidate* dns_query_candidate_free(DnsQueryCandidate *c) { if (!c) return NULL; @@ -67,6 +68,8 @@ DnsQueryCandidate* dns_query_candidate_free(DnsQueryCandidate *c) { return mfree(c); } +DEFINE_PUBLIC_TRIVIAL_REF_UNREF_FUNC(DnsQueryCandidate, dns_query_candidate, dns_query_candidate_free); + static int dns_query_candidate_next_search_domain(DnsQueryCandidate *c) { DnsSearchDomain *next; @@ -129,14 +132,15 @@ static int dns_query_candidate_add_transaction(DnsQueryCandidate *c, DnsResource } static int dns_query_candidate_go(DnsQueryCandidate *c) { + _cleanup_(dns_query_candidate_unrefp) DnsQueryCandidate *keep_c = NULL; DnsTransaction *t; int r; unsigned n = 0; - bool notify = false; assert(c); - c->query->block_ready++; + /* Let's keep a reference to the query while we're operating */ + keep_c = dns_query_candidate_ref(c); /* Start the transactions that are not started yet */ SET_FOREACH(t, c->transactions) { @@ -144,21 +148,14 @@ static int dns_query_candidate_go(DnsQueryCandidate *c) { continue; r = dns_transaction_go(t); - if (r < 0) { - c->query->block_ready--; + if (r < 0) return r; - } - if (r == 0) - /* A transaction is complete. */ - notify = true; n++; } - c->query->block_ready--; - /* If there was nothing to start, then let's proceed immediately */ - if (n == 0 || notify) + if (n == 0) dns_query_candidate_notify(c); return 0; @@ -306,11 +303,11 @@ static void dns_query_stop(DnsQuery *q) { dns_query_candidate_stop(c); } -static void dns_query_free_candidates(DnsQuery *q) { +static void dns_query_unref_candidates(DnsQuery *q) { assert(q); while (q->candidates) - dns_query_candidate_free(q->candidates); + dns_query_candidate_unref(q->candidates); } static void dns_query_reset_answer(DnsQuery *q) { @@ -339,7 +336,7 @@ DnsQuery *dns_query_free(DnsQuery *q) { LIST_REMOVE(auxiliary_queries, q->auxiliary_for->auxiliary_queries, q); } - dns_query_free_candidates(q); + dns_query_unref_candidates(q); dns_question_unref(q->question_idna); dns_question_unref(q->question_utf8); @@ -514,7 +511,7 @@ static int on_query_timeout(sd_event_source *s, usec_t usec, void *userdata) { } static int dns_query_add_candidate(DnsQuery *q, DnsScope *s) { - _cleanup_(dns_query_candidate_freep) DnsQueryCandidate *c = NULL; + _cleanup_(dns_query_candidate_unrefp) DnsQueryCandidate *c = NULL; int r; assert(q); @@ -935,7 +932,7 @@ static int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname) dns_question_unref(q->question_utf8); q->question_utf8 = TAKE_PTR(nq_utf8); - dns_query_free_candidates(q); + dns_query_unref_candidates(q); dns_query_reset_answer(q); q->state = DNS_TRANSACTION_NULL; diff --git a/src/resolve/resolved-dns-query.h b/src/resolve/resolved-dns-query.h index 6aadf28a15..133076dbf0 100644 --- a/src/resolve/resolved-dns-query.h +++ b/src/resolve/resolved-dns-query.h @@ -16,12 +16,14 @@ typedef struct DnsStubListenerExtra DnsStubListenerExtra; #include "resolved-dns-transaction.h" struct DnsQueryCandidate { + unsigned n_ref; + int error_code; + DnsQuery *query; DnsScope *scope; DnsSearchDomain *search_domain; - int error_code; Set *transactions; LIST_FIELDS(DnsQueryCandidate, candidates_by_query); @@ -101,8 +103,9 @@ enum { DNS_QUERY_RESTARTED, }; -DnsQueryCandidate* dns_query_candidate_free(DnsQueryCandidate *c); -DEFINE_TRIVIAL_CLEANUP_FUNC(DnsQueryCandidate*, dns_query_candidate_free); +DnsQueryCandidate* dns_query_candidate_ref(DnsQueryCandidate*); +DnsQueryCandidate* dns_query_candidate_unref(DnsQueryCandidate*); +DEFINE_TRIVIAL_CLEANUP_FUNC(DnsQueryCandidate*, dns_query_candidate_unref); void dns_query_candidate_notify(DnsQueryCandidate *c); diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index 298ce21ae3..d77e81ae39 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -105,7 +105,7 @@ DnsScope* dns_scope_free(DnsScope *s) { dns_scope_abort_transactions(s); while (s->query_candidates) - dns_query_candidate_free(s->query_candidates); + dns_query_candidate_unref(s->query_candidates); hashmap_free(s->transactions_by_key);