Merge pull request #1010 from poettering/resolved-question-key

only maintain one question RR key per transaction and other fixes

10 files changed, 90 insertions, 174 deletions

diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c
index eb51b4b895..efc407dbc6 100644
--- a/src/resolve/resolved-dns-cache.c
+++ b/src/resolve/resolved-dns-cache.c
@@ -458,6 +458,10 @@ int dns_cache_put(
                 if (r > 0)
                         continue;
 
+                /* See https://tools.ietf.org/html/rfc2308, which
+                 * say that a matching SOA record in the packet
+                 * is used to to enable negative caching. */
+
                 r = dns_answer_find_soa(answer, q->keys[i], &soa);
                 if (r < 0)
                         goto fail;
@@ -483,72 +487,65 @@ 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) {
-                *ret = NULL;
-                *rcode = 0;
-                return 0;
-        }
+        if (key->type == DNS_TYPE_ANY ||
+            key->class == DNS_CLASS_ANY) {
 
-        for (i = 0; i < q->n_keys; i++) {
-                _cleanup_free_ char *key_str = NULL;
-                DnsCacheItem *j;
+                /* If we have ANY lookups we simply 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("Ignoring cache for ANY lookup: %s", key_str);
 
-                        log_debug("Ignoring cache for ANY lookup: %s", key_str);
+                *ret = NULL;
+                *rcode = DNS_RCODE_SUCCESS;
+                return 0;
+        }
 
-                        *ret = NULL;
-                        *rcode = 0;
-                        return 0;
-                }
+        first = hashmap_get(c->by_key, key);
+        if (!first) {
+                /* If one question cannot be answered we need to refresh */
 
-                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(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("Cache miss for %s", key_str);
+                *ret = NULL;
+                *rcode = DNS_RCODE_SUCCESS;
+                return 0;
+        }
 
-                        *ret = NULL;
-                        *rcode = 0;
-                        return 0;
-                } else {
-                        r = dns_resource_key_to_string(j->key, &key_str);
-                        if (r < 0)
-                                return r;
+        LIST_FOREACH(by_key, j, first) {
+                if (j->rr)
+                        n++;
+                else if (j->type == DNS_CACHE_NXDOMAIN)
+                        nxdomain = true;
+        }
 
-                        log_debug("%s cache hit for %s",
-                                   j->type == DNS_CACHE_POSITIVE ? "Positive" :
-                                                                  (j->type == DNS_CACHE_NODATA ? "NODATA" : "NXDOMAIN"), key_str);
-                }
+        r = dns_resource_key_to_string(key, &key_str);
+        if (r < 0)
+                return r;
 
-                LIST_FOREACH(by_key, j, j) {
-                        if (j->rr)
-                                n++;
-                        else if (j->type == DNS_CACHE_NXDOMAIN)
-                                nxdomain = true;
-                }
-        }
+        log_debug("%s cache hit for %s",
+                  nxdomain ? "NXDOMAIN" :
+                     n > 0 ? "Positive" : "NODATA",
+                  key_str);
 
         if (n <= 0) {
                 *ret = NULL;
@@ -560,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;
-
-                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;
-                        }
-                }
+        LIST_FOREACH(by_key, j, first) {
+                if (!j->rr)
+                        continue;
+
+                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..d9f5b342b2 100644
--- a/src/resolve/resolved-dns-query.c
+++ b/src/resolve/resolved-dns-query.c
@@ -138,31 +138,20 @@ 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;
 
         assert(q);
         assert(s);
+        assert(key);
 
         r = set_ensure_allocated(&q->transactions, NULL);
         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;
         }
@@ -189,27 +178,18 @@ gc:
 }
 
 static int dns_query_add_transaction_split(DnsQuery *q, DnsScope *s) {
+        unsigned i;
         int r;
 
         assert(q);
         assert(s);
 
-        if (s->protocol == DNS_PROTOCOL_MDNS) {
-                r = dns_query_add_transaction(q, s, NULL);
+        /* Create one transaction per question key */
+
+        for (i = 0; i < q->question->n_keys; i++) {
+                r = dns_query_add_transaction(q, s, q->question->keys[i]);
                 if (r < 0)
                         return r;
-        } else {
-                unsigned i;
-
-                /* On DNS and LLMNR we can only send a single
-                 * question per datagram, hence issue multiple
-                 * transactions. */
-
-                for (i = 0; i < q->question->n_keys; i++) {
-                        r = dns_query_add_transaction(q, s, q->question->keys[i]);
-                        if (r < 0)
-                                return r;
-                }
         }
 
         return 0;
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 73bfbb7ed4..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,11 +267,14 @@ 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)
                                 return -EINVAL;
+                        if (family != t->scope->family)
+                                return -EAFNOSUPPORT;
 
                         fd = dns_scope_tcp_socket(t->scope, family, &address, LLMNR_PORT, NULL);
                 }
@@ -426,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;
         }
@@ -516,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);
@@ -528,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;
@@ -619,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) {
@@ -672,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: