Skip to content

Commit f52e61d

Browse files
poetteringpoettering
committed
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.
1 parent 9e08a6e commit f52e61d

10 files changed

+78
-160
lines changed

‎src/resolve/resolved-dns-cache.c‎

Lines changed: 47 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -487,72 +487,65 @@ int dns_cache_put(
487487
return r;
488488
}
489489

490-
int dns_cache_lookup(DnsCache *c, DnsQuestion *q, int *rcode, DnsAnswer **ret) {
490+
int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **ret) {
491491
_cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL;
492-
unsigned i, n = 0;
492+
unsigned n = 0;
493493
int r;
494494
bool nxdomain = false;
495+
_cleanup_free_ char *key_str = NULL;
496+
DnsCacheItem *j, *first;
495497

496498
assert(c);
497-
assert(q);
499+
assert(key);
498500
assert(rcode);
499501
assert(ret);
500502

501-
if (q->n_keys <= 0) {
502-
*ret = NULL;
503-
*rcode = 0;
504-
return 0;
505-
}
503+
if (key->type == DNS_TYPE_ANY ||
504+
key->class == DNS_CLASS_ANY) {
506505

507-
for (i = 0; i < q->n_keys; i++) {
508-
_cleanup_free_ char *key_str = NULL;
509-
DnsCacheItem *j;
506+
/* If we have ANY lookups we simply refresh */
510507

511-
if (q->keys[i]->type == DNS_TYPE_ANY ||
512-
q->keys[i]->class == DNS_CLASS_ANY) {
513-
/* If we have ANY lookups we simply refresh */
508+
r = dns_resource_key_to_string(key, &key_str);
509+
if (r < 0)
510+
return r;
514511

515-
r = dns_resource_key_to_string(q->keys[i], &key_str);
516-
if (r < 0)
517-
return r;
512+
log_debug("Ignoring cache for ANY lookup: %s", key_str);
518513

519-
log_debug("Ignoring cache for ANY lookup: %s", key_str);
514+
*ret = NULL;
515+
*rcode = DNS_RCODE_SUCCESS;
516+
return 0;
517+
}
520518

521-
*ret = NULL;
522-
*rcode = 0;
523-
return 0;
524-
}
519+
first = hashmap_get(c->by_key, key);
520+
if (!first) {
521+
/* If one question cannot be answered we need to refresh */
525522

526-
j = hashmap_get(c->by_key, q->keys[i]);
527-
if (!j) {
528-
/* If one question cannot be answered we need to refresh */
523+
r = dns_resource_key_to_string(key, &key_str);
524+
if (r < 0)
525+
return r;
529526

530-
r = dns_resource_key_to_string(q->keys[i], &key_str);
531-
if (r < 0)
532-
return r;
527+
log_debug("Cache miss for %s", key_str);
533528

534-
log_debug("Cache miss for %s", key_str);
529+
*ret = NULL;
530+
*rcode = DNS_RCODE_SUCCESS;
531+
return 0;
532+
}
535533

536-
*ret = NULL;
537-
*rcode = 0;
538-
return 0;
539-
} else {
540-
r = dns_resource_key_to_string(j->key, &key_str);
541-
if (r < 0)
542-
return r;
534+
LIST_FOREACH(by_key, j, first) {
535+
if (j->rr)
536+
n++;
537+
else if (j->type == DNS_CACHE_NXDOMAIN)
538+
nxdomain = true;
539+
}
543540

544-
log_debug("%s cache hit for %s",
545-
j->type == DNS_CACHE_POSITIVE ? "Positive" :
546-
(j->type == DNS_CACHE_NODATA ? "NODATA" : "NXDOMAIN"), key_str);
547-
}
541+
r = dns_resource_key_to_string(key, &key_str);
542+
if (r < 0)
543+
return r;
548544

549-
LIST_FOREACH(by_key, j, j) {
550-
if (j->rr)
551-
n++;
552-
else if (j->type == DNS_CACHE_NXDOMAIN)
553-
nxdomain = true;
554-
}
555-
}
545+
log_debug("%s cache hit for %s",
546+
nxdomain ? "NXDOMAIN" :
547+
n > 0 ? "Positive" : "NODATA",
548+
key_str);
556549

557550
if (n <= 0) {
558551
*ret = NULL;
@@ -564,17 +557,13 @@ int dns_cache_lookup(DnsCache *c, DnsQuestion *q, int *rcode, DnsAnswer **ret) {
564557
if (!answer)
565558
return -ENOMEM;
566559

567-
for (i = 0; i < q->n_keys; i++) {
568-
DnsCacheItem *j;
569-
570-
j = hashmap_get(c->by_key, q->keys[i]);
571-
LIST_FOREACH(by_key, j, j) {
572-
if (j->rr) {
573-
r = dns_answer_add(answer, j->rr, 0);
574-
if (r < 0)
575-
return r;
576-
}
577-
}
560+
LIST_FOREACH(by_key, j, first) {
561+
if (!j->rr)
562+
continue;
563+
564+
r = dns_answer_add(answer, j->rr, 0);
565+
if (r < 0)
566+
return r;
578567
}
579568

580569
*ret = answer;

‎src/resolve/resolved-dns-cache.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,6 @@ void dns_cache_flush(DnsCache *c);
4040
void dns_cache_prune(DnsCache *c);
4141

4242
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);
43-
int dns_cache_lookup(DnsCache *c, DnsQuestion *q, int *rcode, DnsAnswer **answer);
43+
int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **answer);
4444

4545
int dns_cache_check_conflicts(DnsCache *cache, DnsResourceRecord *rr, int owner_family, const union in_addr_union *owner_address);

‎src/resolve/resolved-dns-query.c‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,6 @@ static int on_query_timeout(sd_event_source *s, usec_t usec, void *userdata) {
138138
}
139139

140140
static int dns_query_add_transaction(DnsQuery *q, DnsScope *s, DnsResourceKey *key) {
141-
_cleanup_(dns_question_unrefp) DnsQuestion *question = NULL;
142141
DnsTransaction *t;
143142
int r;
144143

@@ -149,20 +148,9 @@ static int dns_query_add_transaction(DnsQuery *q, DnsScope *s, DnsResourceKey *k
149148
if (r < 0)
150149
return r;
151150

152-
if (key) {
153-
question = dns_question_new(1);
154-
if (!question)
155-
return -ENOMEM;
156-
157-
r = dns_question_add(question, key);
158-
if (r < 0)
159-
return r;
160-
} else
161-
question = dns_question_ref(q->question);
162-
163-
t = dns_scope_find_transaction(s, question, true);
151+
t = dns_scope_find_transaction(s, key, true);
164152
if (!t) {
165-
r = dns_transaction_new(&t, s, question);
153+
r = dns_transaction_new(&t, s, key);
166154
if (r < 0)
167155
return r;
168156
}

‎src/resolve/resolved-dns-question.c‎

Lines changed: 0 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -300,42 +300,3 @@ int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion **
300300

301301
return 1;
302302
}
303-
304-
int dns_question_endswith(DnsQuestion *q, const char *suffix) {
305-
unsigned i;
306-
307-
assert(suffix);
308-
309-
if (!q)
310-
return 1;
311-
312-
for (i = 0; i < q->n_keys; i++) {
313-
int k;
314-
315-
k = dns_name_endswith(DNS_RESOURCE_KEY_NAME(q->keys[i]), suffix);
316-
if (k <= 0)
317-
return k;
318-
}
319-
320-
return 1;
321-
}
322-
323-
int dns_question_extract_reverse_address(DnsQuestion *q, int *family, union in_addr_union *address) {
324-
unsigned i;
325-
326-
assert(family);
327-
assert(address);
328-
329-
if (!q)
330-
return 0;
331-
332-
for (i = 0; i < q->n_keys; i++) {
333-
int k;
334-
335-
k = dns_name_address(DNS_RESOURCE_KEY_NAME(q->keys[i]), family, address);
336-
if (k != 0)
337-
return k;
338-
}
339-
340-
return 0;
341-
}

‎src/resolve/resolved-dns-question.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,4 @@ int dns_question_is_equal(DnsQuestion *a, DnsQuestion *b);
4848

4949
int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion **ret);
5050

51-
int dns_question_endswith(DnsQuestion *q, const char *suffix);
52-
int dns_question_extract_reverse_address(DnsQuestion *q, int *family, union in_addr_union *address);
53-
5451
DEFINE_TRIVIAL_CLEANUP_FUNC(DnsQuestion*, dns_question_unref);

‎src/resolve/resolved-dns-scope.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -617,11 +617,11 @@ void dns_scope_process_query(DnsScope *s, DnsStream *stream, DnsPacket *p) {
617617
}
618618
}
619619

620-
DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsQuestion *question, bool cache_ok) {
620+
DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsResourceKey *key, bool cache_ok) {
621621
DnsTransaction *t;
622622

623623
assert(scope);
624-
assert(question);
624+
assert(key);
625625

626626
/* Try to find an ongoing transaction that is a equal or a
627627
* superset of the specified question */
@@ -636,7 +636,7 @@ DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsQuestion *questio
636636
!t->received)
637637
continue;
638638

639-
if (dns_question_is_superset(t->question, question) > 0)
639+
if (dns_resource_key_equal(t->key, key) > 0)
640640
return t;
641641
}
642642

‎src/resolve/resolved-dns-scope.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ int dns_scope_llmnr_membership(DnsScope *s, bool b);
8585

8686
void dns_scope_process_query(DnsScope *s, DnsStream *stream, DnsPacket *p);
8787

88-
DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsQuestion *question, bool cache_ok);
88+
DnsTransaction *dns_scope_find_transaction(DnsScope *scope, DnsResourceKey *key, bool cache_ok);
8989

9090
int dns_scope_notify_conflict(DnsScope *scope, DnsResourceRecord *rr);
9191
void dns_scope_check_conflicts(DnsScope *scope, DnsPacket *p);

‎src/resolve/resolved-dns-transaction.c‎

Lines changed: 20 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "resolved-llmnr.h"
2525
#include "resolved-dns-transaction.h"
2626
#include "random-util.h"
27+
#include "dns-domain.h"
2728

2829
DnsTransaction* dns_transaction_free(DnsTransaction *t) {
2930
DnsQuery *q;
@@ -34,7 +35,7 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) {
3435

3536
sd_event_source_unref(t->timeout_event_source);
3637

37-
dns_question_unref(t->question);
38+
dns_resource_key_unref(t->key);
3839
dns_packet_unref(t->sent);
3940
dns_packet_unref(t->received);
4041
dns_answer_unref(t->cached);
@@ -76,13 +77,13 @@ void dns_transaction_gc(DnsTransaction *t) {
7677
dns_transaction_free(t);
7778
}
7879

79-
int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsQuestion *q) {
80+
int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsResourceKey *key) {
8081
_cleanup_(dns_transaction_freep) DnsTransaction *t = NULL;
8182
int r;
8283

8384
assert(ret);
8485
assert(s);
85-
assert(q);
86+
assert(key);
8687

8788
r = hashmap_ensure_allocated(&s->manager->dns_transactions, NULL);
8889
if (r < 0)
@@ -94,7 +95,7 @@ int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsQuestion *q) {
9495

9596
t->dns_fd = -1;
9697

97-
t->question = dns_question_ref(q);
98+
t->key = dns_resource_key_ref(key);
9899

99100
do
100101
random_bytes(&t->id, sizeof(t->id));
@@ -266,7 +267,8 @@ static int dns_transaction_open_tcp(DnsTransaction *t) {
266267
/* Otherwise, try to talk to the owner of a
267268
* the IP address, in case this is a reverse
268269
* PTR lookup */
269-
r = dns_question_extract_reverse_address(t->question, &family, &address);
270+
271+
r = dns_name_address(DNS_RESOURCE_KEY_NAME(t->key), &family, &address);
270272
if (r < 0)
271273
return r;
272274
if (r == 0)
@@ -428,8 +430,7 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) {
428430
}
429431

430432
/* Only consider responses with equivalent query section to the request */
431-
r = dns_question_is_equal(p->question, t->question);
432-
if (r <= 0) {
433+
if (p->question->n_keys != 1 || dns_resource_key_equal(p->question->keys[0], t->key) <= 0) {
433434
dns_transaction_complete(t, DNS_TRANSACTION_INVALID_REPLY);
434435
return;
435436
}
@@ -518,7 +519,6 @@ static int on_transaction_timeout(sd_event_source *s, usec_t usec, void *userdat
518519

519520
static int dns_transaction_make_packet(DnsTransaction *t) {
520521
_cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
521-
unsigned n, added = 0;
522522
int r;
523523

524524
assert(t);
@@ -530,24 +530,17 @@ static int dns_transaction_make_packet(DnsTransaction *t) {
530530
if (r < 0)
531531
return r;
532532

533-
for (n = 0; n < t->question->n_keys; n++) {
534-
r = dns_scope_good_key(t->scope, t->question->keys[n]);
535-
if (r < 0)
536-
return r;
537-
if (r == 0)
538-
continue;
539-
540-
r = dns_packet_append_key(p, t->question->keys[n], NULL);
541-
if (r < 0)
542-
return r;
543-
544-
added++;
545-
}
546-
547-
if (added <= 0)
533+
r = dns_scope_good_key(t->scope, t->key);
534+
if (r < 0)
535+
return r;
536+
if (r == 0)
548537
return -EDOM;
549538

550-
DNS_PACKET_HEADER(p)->qdcount = htobe16(added);
539+
r = dns_packet_append_key(p, t->key, NULL);
540+
if (r < 0)
541+
return r;
542+
543+
DNS_PACKET_HEADER(p)->qdcount = htobe16(1);
551544
DNS_PACKET_HEADER(p)->id = t->id;
552545

553546
t->sent = p;
@@ -621,7 +614,7 @@ int dns_transaction_go(DnsTransaction *t) {
621614
/* Let's then prune all outdated entries */
622615
dns_cache_prune(&t->scope->cache);
623616

624-
r = dns_cache_lookup(&t->scope->cache, t->question, &t->cached_rcode, &t->cached);
617+
r = dns_cache_lookup(&t->scope->cache, t->key, &t->cached_rcode, &t->cached);
625618
if (r < 0)
626619
return r;
627620
if (r > 0) {
@@ -674,8 +667,8 @@ int dns_transaction_go(DnsTransaction *t) {
674667
return r;
675668

676669
if (t->scope->protocol == DNS_PROTOCOL_LLMNR &&
677-
(dns_question_endswith(t->question, "in-addr.arpa") > 0 ||
678-
dns_question_endswith(t->question, "ip6.arpa") > 0)) {
670+
(dns_name_endswith(DNS_RESOURCE_KEY_NAME(t->key), "in-addr.arpa") > 0 ||
671+
dns_name_endswith(DNS_RESOURCE_KEY_NAME(t->key), "ip6.arpa") > 0)) {
679672

680673
/* RFC 4795, Section 2.4. says reverse lookups shall
681674
* always be made via TCP on LLMNR */

0 commit comments

Comments
 (0)