From f52e61da047d7fc74e83f12dbbf87e0cbcc51c73 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 Aug 2015 22:55:01 +0200 Subject: [PATCH] resolved: only maintain one question RR key per transaction Let's simplify things and only maintain a single RR key per transaction object, instead of a full DnsQuestion. Unicast DNS and LLMNR don't support multiple questions per packet anway, and Multicast DNS suggests coalescing questions beyond a single dns query, across the whole system. --- src/resolve/resolved-dns-cache.c | 109 +++++++++++-------------- src/resolve/resolved-dns-cache.h | 2 +- src/resolve/resolved-dns-query.c | 16 +--- src/resolve/resolved-dns-question.c | 39 --------- src/resolve/resolved-dns-question.h | 3 - src/resolve/resolved-dns-scope.c | 6 +- src/resolve/resolved-dns-scope.h | 2 +- src/resolve/resolved-dns-transaction.c | 47 +++++------ src/resolve/resolved-dns-transaction.h | 4 +- src/resolve/resolved-dns-zone.c | 14 +--- 10 files changed, 80 insertions(+), 162 deletions(-) diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c index 751e961351..efc407dbc6 100644 --- a/src/resolve/resolved-dns-cache.c +++ b/src/resolve/resolved-dns-cache.c @@ -487,73 +487,66 @@ fail: return r; } -int dns_cache_lookup(DnsCache *c, DnsQuestion *q, int *rcode, DnsAnswer **ret) { +int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **ret) { _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL; - unsigned i, n = 0; + unsigned n = 0; int r; bool nxdomain = false; + _cleanup_free_ char *key_str = NULL; + DnsCacheItem *j, *first; assert(c); - assert(q); + assert(key); assert(rcode); assert(ret); - if (q->n_keys <= 0) { + if (key->type == DNS_TYPE_ANY || + key->class == DNS_CLASS_ANY) { + + /* If we have ANY lookups we simply refresh */ + + r = dns_resource_key_to_string(key, &key_str); + if (r < 0) + return r; + + log_debug("Ignoring cache for ANY lookup: %s", key_str); + *ret = NULL; - *rcode = 0; + *rcode = DNS_RCODE_SUCCESS; return 0; } - for (i = 0; i < q->n_keys; i++) { - _cleanup_free_ char *key_str = NULL; - DnsCacheItem *j; + first = hashmap_get(c->by_key, key); + if (!first) { + /* If one question cannot be answered we need to refresh */ - if (q->keys[i]->type == DNS_TYPE_ANY || - q->keys[i]->class == DNS_CLASS_ANY) { - /* If we have ANY lookups we simply refresh */ + r = dns_resource_key_to_string(key, &key_str); + if (r < 0) + return r; - r = dns_resource_key_to_string(q->keys[i], &key_str); - if (r < 0) - return r; + log_debug("Cache miss for %s", key_str); - log_debug("Ignoring cache for ANY lookup: %s", key_str); - - *ret = NULL; - *rcode = 0; - return 0; - } - - j = hashmap_get(c->by_key, q->keys[i]); - if (!j) { - /* If one question cannot be answered we need to refresh */ - - r = dns_resource_key_to_string(q->keys[i], &key_str); - if (r < 0) - return r; - - log_debug("Cache miss for %s", key_str); - - *ret = NULL; - *rcode = 0; - return 0; - } else { - r = dns_resource_key_to_string(j->key, &key_str); - if (r < 0) - return r; - - log_debug("%s cache hit for %s", - j->type == DNS_CACHE_POSITIVE ? "Positive" : - (j->type == DNS_CACHE_NODATA ? "NODATA" : "NXDOMAIN"), key_str); - } - - LIST_FOREACH(by_key, j, j) { - if (j->rr) - n++; - else if (j->type == DNS_CACHE_NXDOMAIN) - nxdomain = true; - } + *ret = NULL; + *rcode = DNS_RCODE_SUCCESS; + return 0; } + LIST_FOREACH(by_key, j, first) { + if (j->rr) + n++; + else if (j->type == DNS_CACHE_NXDOMAIN) + nxdomain = true; + } + + r = dns_resource_key_to_string(key, &key_str); + if (r < 0) + return r; + + log_debug("%s cache hit for %s", + nxdomain ? "NXDOMAIN" : + n > 0 ? "Positive" : "NODATA", + key_str); + if (n <= 0) { *ret = NULL; *rcode = nxdomain ? DNS_RCODE_NXDOMAIN : DNS_RCODE_SUCCESS; @@ -564,17 +557,13 @@ int dns_cache_lookup(DnsCache *c, DnsQuestion *q, int *rcode, DnsAnswer **ret) { if (!answer) return -ENOMEM; - for (i = 0; i < q->n_keys; i++) { - DnsCacheItem *j; + LIST_FOREACH(by_key, j, first) { + if (!j->rr) + continue; - j = hashmap_get(c->by_key, q->keys[i]); - LIST_FOREACH(by_key, j, j) { - if (j->rr) { - r = dns_answer_add(answer, j->rr, 0); - if (r < 0) - return r; - } - } + r = dns_answer_add(answer, j->rr, 0); + if (r < 0) + return r; } *ret = answer; diff --git a/src/resolve/resolved-dns-cache.h b/src/resolve/resolved-dns-cache.h index 8a9b3d459d..fd583a775f 100644 --- a/src/resolve/resolved-dns-cache.h +++ b/src/resolve/resolved-dns-cache.h @@ -40,6 +40,6 @@ void dns_cache_flush(DnsCache *c); void dns_cache_prune(DnsCache *c); int dns_cache_put(DnsCache *c, DnsQuestion *q, int rcode, DnsAnswer *answer, unsigned max_rrs, usec_t timestamp, int owner_family, const union in_addr_union *owner_address); -int dns_cache_lookup(DnsCache *c, DnsQuestion *q, int *rcode, DnsAnswer **answer); +int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **answer); int dns_cache_check_conflicts(DnsCache *cache, DnsResourceRecord *rr, int owner_family, const union in_addr_union *owner_address); diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 4f3f903548..bfa5a08ab3 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -138,7 +138,6 @@ static int on_query_timeout(sd_event_source *s, usec_t usec, void *userdata) { } static int dns_query_add_transaction(DnsQuery *q, DnsScope *s, DnsResourceKey *key) { - _cleanup_(dns_question_unrefp) DnsQuestion *question = NULL; DnsTransaction *t; int r; @@ -149,20 +148,9 @@ static int dns_query_add_transaction(DnsQuery *q, DnsScope *s, DnsResourceKey *k if (r < 0) return r; - if (key) { - question = dns_question_new(1); - if (!question) - return -ENOMEM; - - r = dns_question_add(question, key); - if (r < 0) - return r; - } else - question = dns_question_ref(q->question); - - t = dns_scope_find_transaction(s, question, true); + t = dns_scope_find_transaction(s, key, true); if (!t) { - r = dns_transaction_new(&t, s, question); + r = dns_transaction_new(&t, s, key); if (r < 0) return r; } diff --git a/src/resolve/resolved-dns-question.c b/src/resolve/resolved-dns-question.c index 7590bc5418..c94928d725 100644 --- a/src/resolve/resolved-dns-question.c +++ b/src/resolve/resolved-dns-question.c @@ -300,42 +300,3 @@ int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion ** return 1; } - -int dns_question_endswith(DnsQuestion *q, const char *suffix) { - unsigned i; - - assert(suffix); - - if (!q) - return 1; - - for (i = 0; i < q->n_keys; i++) { - int k; - - k = dns_name_endswith(DNS_RESOURCE_KEY_NAME(q->keys[i]), suffix); - if (k <= 0) - return k; - } - - return 1; -} - -int dns_question_extract_reverse_address(DnsQuestion *q, int *family, union in_addr_union *address) { - unsigned i; - - assert(family); - assert(address); - - if (!q) - return 0; - - for (i = 0; i < q->n_keys; i++) { - int k; - - k = dns_name_address(DNS_RESOURCE_KEY_NAME(q->keys[i]), family, address); - if (k != 0) - return k; - } - - return 0; -} diff --git a/src/resolve/resolved-dns-question.h b/src/resolve/resolved-dns-question.h index fc98677798..77de0c7a2c 100644 --- a/src/resolve/resolved-dns-question.h +++ b/src/resolve/resolved-dns-question.h @@ -48,7 +48,4 @@ int dns_question_is_equal(DnsQuestion *a, DnsQuestion *b); int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion **ret); -int dns_question_endswith(DnsQuestion *q, const char *suffix); -int dns_question_extract_reverse_address(DnsQuestion *q, int *family, union in_addr_union *address); - DEFINE_TRIVIAL_CLEANUP_FUNC(DnsQuestion*, dns_question_unref); diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index b1e5855a6f..57a2c7d6c1 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -617,11 +617,11 @@ void dns_scope_process_query(DnsScope *s, DnsStream *stream, DnsPacket *p) { } } -DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsQuestion *question, bool cache_ok) { +DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsResourceKey *key, bool cache_ok) { DnsTransaction *t; assert(scope); - assert(question); + assert(key); /* Try to find an ongoing transaction that is a equal or a * superset of the specified question */ @@ -636,7 +636,7 @@ DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsQuestion *questio !t->received) continue; - if (dns_question_is_superset(t->question, question) > 0) + if (dns_resource_key_equal(t->key, key) > 0) return t; } diff --git a/src/resolve/resolved-dns-scope.h b/src/resolve/resolved-dns-scope.h index b2dac86b44..47f285db47 100644 --- a/src/resolve/resolved-dns-scope.h +++ b/src/resolve/resolved-dns-scope.h @@ -85,7 +85,7 @@ int dns_scope_llmnr_membership(DnsScope *s, bool b); void dns_scope_process_query(DnsScope *s, DnsStream *stream, DnsPacket *p); -DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsQuestion *question, bool cache_ok); +DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsResourceKey *key, bool cache_ok); int dns_scope_notify_conflict(DnsScope *scope, DnsResourceRecord *rr); void dns_scope_check_conflicts(DnsScope *scope, DnsPacket *p); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 3fc4063917..81156dfa45 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -24,6 +24,7 @@ #include "resolved-llmnr.h" #include "resolved-dns-transaction.h" #include "random-util.h" +#include "dns-domain.h" DnsTransaction* dns_transaction_free(DnsTransaction *t) { DnsQuery *q; @@ -34,7 +35,7 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) { sd_event_source_unref(t->timeout_event_source); - dns_question_unref(t->question); + dns_resource_key_unref(t->key); dns_packet_unref(t->sent); dns_packet_unref(t->received); dns_answer_unref(t->cached); @@ -76,13 +77,13 @@ void dns_transaction_gc(DnsTransaction *t) { dns_transaction_free(t); } -int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsQuestion *q) { +int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsResourceKey *key) { _cleanup_(dns_transaction_freep) DnsTransaction *t = NULL; int r; assert(ret); assert(s); - assert(q); + assert(key); r = hashmap_ensure_allocated(&s->manager->dns_transactions, NULL); if (r < 0) @@ -94,7 +95,7 @@ int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsQuestion *q) { t->dns_fd = -1; - t->question = dns_question_ref(q); + t->key = dns_resource_key_ref(key); do random_bytes(&t->id, sizeof(t->id)); @@ -266,7 +267,8 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { /* Otherwise, try to talk to the owner of a * the IP address, in case this is a reverse * PTR lookup */ - r = dns_question_extract_reverse_address(t->question, &family, &address); + + r = dns_name_address(DNS_RESOURCE_KEY_NAME(t->key), &family, &address); if (r < 0) return r; if (r == 0) @@ -428,8 +430,7 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) { } /* Only consider responses with equivalent query section to the request */ - r = dns_question_is_equal(p->question, t->question); - if (r <= 0) { + if (p->question->n_keys != 1 || dns_resource_key_equal(p->question->keys[0], t->key) <= 0) { dns_transaction_complete(t, DNS_TRANSACTION_INVALID_REPLY); return; } @@ -518,7 +519,6 @@ static int on_transaction_timeout(sd_event_source *s, usec_t usec, void *userdat static int dns_transaction_make_packet(DnsTransaction *t) { _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; - unsigned n, added = 0; int r; assert(t); @@ -530,24 +530,17 @@ static int dns_transaction_make_packet(DnsTransaction *t) { if (r < 0) return r; - for (n = 0; n < t->question->n_keys; n++) { - r = dns_scope_good_key(t->scope, t->question->keys[n]); - if (r < 0) - return r; - if (r == 0) - continue; - - r = dns_packet_append_key(p, t->question->keys[n], NULL); - if (r < 0) - return r; - - added++; - } - - if (added <= 0) + r = dns_scope_good_key(t->scope, t->key); + if (r < 0) + return r; + if (r == 0) return -EDOM; - DNS_PACKET_HEADER(p)->qdcount = htobe16(added); + r = dns_packet_append_key(p, t->key, NULL); + if (r < 0) + return r; + + DNS_PACKET_HEADER(p)->qdcount = htobe16(1); DNS_PACKET_HEADER(p)->id = t->id; t->sent = p; @@ -621,7 +614,7 @@ int dns_transaction_go(DnsTransaction *t) { /* Let's then prune all outdated entries */ dns_cache_prune(&t->scope->cache); - r = dns_cache_lookup(&t->scope->cache, t->question, &t->cached_rcode, &t->cached); + r = dns_cache_lookup(&t->scope->cache, t->key, &t->cached_rcode, &t->cached); if (r < 0) return r; if (r > 0) { @@ -674,8 +667,8 @@ int dns_transaction_go(DnsTransaction *t) { return r; if (t->scope->protocol == DNS_PROTOCOL_LLMNR && - (dns_question_endswith(t->question, "in-addr.arpa") > 0 || - dns_question_endswith(t->question, "ip6.arpa") > 0)) { + (dns_name_endswith(DNS_RESOURCE_KEY_NAME(t->key), "in-addr.arpa") > 0 || + dns_name_endswith(DNS_RESOURCE_KEY_NAME(t->key), "ip6.arpa") > 0)) { /* RFC 4795, Section 2.4. says reverse lookups shall * always be made via TCP on LLMNR */ diff --git a/src/resolve/resolved-dns-transaction.h b/src/resolve/resolved-dns-transaction.h index d8a5647609..4db9352a04 100644 --- a/src/resolve/resolved-dns-transaction.h +++ b/src/resolve/resolved-dns-transaction.h @@ -47,7 +47,7 @@ enum DnsTransactionState { struct DnsTransaction { DnsScope *scope; - DnsQuestion *question; + DnsResourceKey *key; DnsTransactionState state; uint16_t id; @@ -84,7 +84,7 @@ struct DnsTransaction { LIST_FIELDS(DnsTransaction, transactions_by_scope); }; -int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsQuestion *q); +int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsResourceKey *key); DnsTransaction* dns_transaction_free(DnsTransaction *t); void dns_transaction_gc(DnsTransaction *t); diff --git a/src/resolve/resolved-dns-zone.c b/src/resolve/resolved-dns-zone.c index 99d96c3f40..fc212f48f4 100644 --- a/src/resolve/resolved-dns-zone.c +++ b/src/resolve/resolved-dns-zone.c @@ -166,7 +166,6 @@ static int dns_zone_link_item(DnsZone *z, DnsZoneItem *i) { static int dns_zone_item_probe_start(DnsZoneItem *i) { _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; - _cleanup_(dns_question_unrefp) DnsQuestion *question = NULL; DnsTransaction *t; int r; @@ -179,17 +178,9 @@ static int dns_zone_item_probe_start(DnsZoneItem *i) { if (!key) return -ENOMEM; - question = dns_question_new(1); - if (!question) - return -ENOMEM; - - r = dns_question_add(question, key); - if (r < 0) - return r; - - t = dns_scope_find_transaction(i->scope, question, false); + t = dns_scope_find_transaction(i->scope, key, false); if (!t) { - r = dns_transaction_new(&t, i->scope, question); + r = dns_transaction_new(&t, i->scope, key); if (r < 0) return r; } @@ -217,7 +208,6 @@ static int dns_zone_item_probe_start(DnsZoneItem *i) { } dns_zone_item_ready(i); - return 0; gc: